8211320: Aarch64: unsafe.compareAndSetByte() and unsafe.compareAndSetShort() c2 intrinsics broken with negative expected value

Reviewed-by: adinn, aph

src/hotspot/cpu/aarch64/aarch64.ad

@@ -8341,8 +8341,7 @@ instruct compareAndExchangeB(iRegINoSp res, indirect mem, iRegI oldval, iRegI ne
     "cmpxchg $res = $mem, $oldval, $newval\t# (byte, weak) if $mem == $oldval then $mem <-- $newval"
   %}
   ins_encode %{
-    __ uxtbw(rscratch2, $oldval$$Register);
-    __ cmpxchg($mem$$Register, rscratch2, $newval$$Register,
+    __ cmpxchg($mem$$Register, $oldval$$Register, $newval$$Register,
                Assembler::byte, /*acquire*/ false, /*release*/ true,
                /*weak*/ false, $res$$Register);
     __ sxtbw($res$$Register, $res$$Register);
@@ -8358,8 +8357,7 @@ instruct compareAndExchangeS(iRegINoSp res, indirect mem, iRegI oldval, iRegI ne
     "cmpxchg $res = $mem, $oldval, $newval\t# (short, weak) if $mem == $oldval then $mem <-- $newval"
   %}
   ins_encode %{
-    __ uxthw(rscratch2, $oldval$$Register);
-    __ cmpxchg($mem$$Register, rscratch2, $newval$$Register,
+    __ cmpxchg($mem$$Register, $oldval$$Register, $newval$$Register,
                Assembler::halfword, /*acquire*/ false, /*release*/ true,
                /*weak*/ false, $res$$Register);
     __ sxthw($res$$Register, $res$$Register);
@@ -8436,8 +8434,7 @@ instruct weakCompareAndSwapB(iRegINoSp res, indirect mem, iRegI oldval, iRegI ne
     "csetw $res, EQ\t# $res <-- (EQ ? 1 : 0)"
   %}
   ins_encode %{
-    __ uxtbw(rscratch2, $oldval$$Register);
-    __ cmpxchg($mem$$Register, rscratch2, $newval$$Register,
+    __ cmpxchg($mem$$Register, $oldval$$Register, $newval$$Register,
                Assembler::byte, /*acquire*/ false, /*release*/ true,
                /*weak*/ true, noreg);
     __ csetw($res$$Register, Assembler::EQ);
@@ -8454,8 +8451,7 @@ instruct weakCompareAndSwapS(iRegINoSp res, indirect mem, iRegI oldval, iRegI ne
     "csetw $res, EQ\t# $res <-- (EQ ? 1 : 0)"
   %}
   ins_encode %{
-    __ uxthw(rscratch2, $oldval$$Register);
-    __ cmpxchg($mem$$Register, rscratch2, $newval$$Register,
+    __ cmpxchg($mem$$Register, $oldval$$Register, $newval$$Register,
                Assembler::halfword, /*acquire*/ false, /*release*/ true,
                /*weak*/ true, noreg);
     __ csetw($res$$Register, Assembler::EQ);

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp

@@ -2367,21 +2367,18 @@ void MacroAssembler::cmpxchg(Register addr, Register expected,
                              bool weak,
                              Register result) {
   if (result == noreg)  result = rscratch1;
+  BLOCK_COMMENT("cmpxchg {");
   if (UseLSE) {
     mov(result, expected);
     lse_cas(result, new_val, addr, size, acquire, release, /*not_pair*/ true);
-    cmp(result, expected);
+    compare_eq(result, expected, size);
   } else {
-    BLOCK_COMMENT("cmpxchg {");
     Label retry_load, done;
     if ((VM_Version::features() & VM_Version::CPU_STXR_PREFETCH))
       prfm(Address(addr), PSTL1STRM);
     bind(retry_load);
     load_exclusive(result, addr, size, acquire);
-    if (size == xword)
-      cmp(result, expected);
-    else
-      cmpw(result, expected);
+    compare_eq(result, expected, size);
     br(Assembler::NE, done);
     store_exclusive(rscratch1, new_val, addr, size, release);
     if (weak) {
@@ -2390,10 +2387,28 @@ void MacroAssembler::cmpxchg(Register addr, Register expected,
       cbnzw(rscratch1, retry_load);
     }
     bind(done);
-    BLOCK_COMMENT("} cmpxchg");
+  }
+  BLOCK_COMMENT("} cmpxchg");
+}
+
+// A generic comparison. Only compares for equality, clobbers rscratch1.
+void MacroAssembler::compare_eq(Register rm, Register rn, enum operand_size size) {
+  if (size == xword) {
+    cmp(rm, rn);
+  } else if (size == word) {
+    cmpw(rm, rn);
+  } else if (size == halfword) {
+    eorw(rscratch1, rm, rn);
+    ands(zr, rscratch1, 0xffff);
+  } else if (size == byte) {
+    eorw(rscratch1, rm, rn);
+    ands(zr, rscratch1, 0xff);
+  } else {
+    ShouldNotReachHere();
   }
 }
 
+
 static bool different(Register a, RegisterOrConstant b, Register c) {
   if (b.is_constant())
     return a != c;

src/hotspot/cpu/aarch64/macroAssembler_aarch64.hpp

@@ -1020,7 +1020,10 @@ public:
                enum operand_size size,
                bool acquire, bool release, bool weak,
                Register result);
+private:
+  void compare_eq(Register rn, Register rm, enum operand_size size);
 
+public:
   // Calls
 
   address trampoline_call(Address entry, CodeBuffer *cbuf = NULL);

test/hotspot/jtreg/compiler/unsafe/CASandCAEwithNegExpected.java

@@ -0,0 +1,143 @@
+/*
+ * Copyright (c) 2018, Red Hat, Inc. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8211320
+ * @summary Aarch64: unsafe.compareAndSetByte() and unsafe.compareAndSetShort() c2 intrinsics broken with negative expected value
+ *
+ * @modules java.base/jdk.internal.misc
+ * @run main/othervm -XX:-UseOnStackReplacement -XX:-BackgroundCompilation  CASandCAEwithNegExpected
+ */
+
+import java.lang.reflect.Field;
+import jdk.internal.misc.Unsafe;
+
+public class CASandCAEwithNegExpected {
+    public volatile int f_int = -1;
+    public volatile long f_long = -1;
+    public volatile byte f_byte = -1;
+    public volatile short f_short = -1;
+
+    public static Unsafe unsafe = Unsafe.getUnsafe();
+    public static Field f_int_field;
+    public static Field f_long_field;
+    public static Field f_byte_field;
+    public static Field f_short_field;
+    public static long f_int_off;
+    public static long f_long_off;
+    public static long f_byte_off;
+    public static long f_short_off;
+
+    static {
+        try {
+            f_int_field = CASandCAEwithNegExpected.class.getField("f_int");
+            f_long_field = CASandCAEwithNegExpected.class.getField("f_long");
+            f_byte_field = CASandCAEwithNegExpected.class.getField("f_byte");
+            f_short_field = CASandCAEwithNegExpected.class.getField("f_short");
+            f_int_off = unsafe.objectFieldOffset(f_int_field);
+            f_long_off = unsafe.objectFieldOffset(f_long_field);
+            f_byte_off = unsafe.objectFieldOffset(f_byte_field);
+            f_short_off = unsafe.objectFieldOffset(f_short_field);
+        } catch (Exception e) {
+            System.out.println("reflection failed " + e);
+            e.printStackTrace();
+        }
+    }
+
+    static public void main(String[] args) {
+        CASandCAEwithNegExpected t = new CASandCAEwithNegExpected();
+        for (int i = 0; i < 20_000; i++) {
+            t.test();
+        }
+    }
+
+    // check proper handling of sign extension of expected value in comparison
+    void test() {
+        f_int = -1;
+        f_long = -1;
+        f_byte = -1;
+        f_short = -1;
+
+        unsafe.compareAndSetInt(this, f_int_off, -1, 42);
+        if (f_int != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+        unsafe.compareAndSetLong(this, f_long_off, -1, 42);
+        if (f_long != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+        unsafe.compareAndSetByte(this, f_byte_off, (byte)-1, (byte)42);
+        if (f_byte != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+        unsafe.compareAndSetShort(this, f_short_off, (short)-1, (short)42);
+        if (f_short != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+
+        f_int = -1;
+        f_long = -1;
+        f_byte = -1;
+        f_short = -1;
+
+        unsafe.compareAndExchangeInt(this, f_int_off, -1, 42);
+        if (f_int != 42) {
+            throw new RuntimeException("CAE failed");
+        }
+        unsafe.compareAndExchangeLong(this, f_long_off, -1, 42);
+        if (f_long != 42) {
+            throw new RuntimeException("CAE failed");
+        }
+        unsafe.compareAndExchangeByte(this, f_byte_off, (byte)-1, (byte)42);
+        if (f_byte != 42) {
+            throw new RuntimeException("CAE failed");
+        }
+        unsafe.compareAndExchangeShort(this, f_short_off, (short)-1, (short)42);
+        if (f_short != 42) {
+            throw new RuntimeException("CAE failed");
+        }
+
+        f_int = -1;
+        f_long = -1;
+        f_byte = -1;
+        f_short = -1;
+
+        if (unsafe.weakCompareAndSetInt(this, f_int_off, -1, 42) && f_int != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+
+        if (unsafe.weakCompareAndSetLong(this, f_long_off, -1, 42) && f_long != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+
+        if (unsafe.weakCompareAndSetByte(this, f_byte_off, (byte)-1, (byte)42) && f_byte != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+
+        if (unsafe.weakCompareAndSetShort(this, f_short_off, (short)-1, (short)42) && f_short != 42) {
+            throw new RuntimeException("CAS failed");
+        }
+    }
+
+}