[riscv-port] RFR: 8277884: riscv: Fix cmpxchg_narrow_value that needs to sign-extend non-bool results
Hi team, According to https://bugs.openjdk.java.net/browse/JDK-8277884 and [the original patch](https://github.com/riscv-collab/riscv-openjdk/pull/15): A very simple program inherited from jcstress could reproduce this: import java.lang.invoke.MethodHandles; import java.lang.invoke.VarHandle; // run with: // build/linux-riscv64-server-release/images/jdk/bin/java -ea -XX:-TieredCompilation -XX:CompileCommand=compileonly,jdk.internal.misc.Unsafe::compareAndSetByte TestCASByte // assert will fail. // if we add '-XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_compareAndExchangeByte' this test will pass. public class TestCASByte { byte[] array = new byte[1]; static final VarHandle vh; static { try { vh = MethodHandles.arrayElementVarHandle(byte[].class); } catch (Exception e) { throw new RuntimeException(e); } } public void test() { for (int i = 0; i < 100_000; i++) { // To level 4 // clear array[0] = 0; byte res1 = (byte)vh.getAndSet(array, 0, (byte)-1); byte res2 = (byte)vh.getAndSet(array, 0, (byte)2); assert res1 == 0 && res2 == -1; } } public static void main(String[] args) { new TestCASByte().test(); } } In this case when we are getting the -1 value saved in memory, the shift is 0, the old is 0x00000000000000ff and the mask is 0x00000000000000ff at the end of cmpxchg_narrow_value. The final result is 0x00000000000000ff but it indeed should be 0xffffffffffffffff. BTW the shift is 0. Therefore, if the result is negative, we need to make a sign extension. Thanks, Xiaolin ------------- Commit messages: - Fix cmpxchg_narrow_value that needs to sign-extend non-bool results Changes: https://git.openjdk.java.net/riscv-port/pull/15/files Webrev: https://webrevs.openjdk.java.net/?repo=riscv-port&pr=15&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277884 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/riscv-port/pull/15.diff Fetch: git fetch https://git.openjdk.java.net/riscv-port pull/15/head:pull/15 PR: https://git.openjdk.java.net/riscv-port/pull/15
On Mon, 29 Nov 2021 03:40:43 GMT, zhengxiaolinX <duke@openjdk.java.net> wrote:
Hi team,
According to https://bugs.openjdk.java.net/browse/JDK-8277884 and [the original patch](https://github.com/riscv-collab/riscv-openjdk/pull/9):
A very simple program inherited from jcstress could reproduce this:
import java.lang.invoke.MethodHandles; import java.lang.invoke.VarHandle;
// run with: // build/linux-riscv64-server-release/images/jdk/bin/java -ea -XX:-TieredCompilation -XX:CompileCommand=compileonly,jdk.internal.misc.Unsafe::compareAndSetByte TestCASByte // assert will fail. // if we add '-XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_compareAndExchangeByte' this test will pass.
public class TestCASByte {
byte[] array = new byte[1];
static final VarHandle vh; static { try { vh = MethodHandles.arrayElementVarHandle(byte[].class); } catch (Exception e) { throw new RuntimeException(e); } }
public void test() { for (int i = 0; i < 100_000; i++) { // To level 4 // clear array[0] = 0;
byte res1 = (byte)vh.getAndSet(array, 0, (byte)-1); byte res2 = (byte)vh.getAndSet(array, 0, (byte)2);
assert res1 == 0 && res2 == -1; } }
public static void main(String[] args) { new TestCASByte().test(); }
}
In this case when we are getting the `-1` value saved in memory, the shift is `0`, the old is `0x00000000000000ff` and the mask is `0x00000000000000ff` at the end of cmpxchg_narrow_value. The final result is `0x00000000000000ff` but it indeed should be `0xffffffffffffffff`. Therefore, if the result is negative, we need to make a sign extension.
Thanks, Xiaolin
Looks good. I can also reproduce the bug with following three jcstress tests: org.openjdk.jcstress.tests.atomicity.varHandles.fields.GetAndSetTest.GetAndSetByte org.openjdk.jcstress.tests.atomicity.varHandles.fields.GetAndSetTest.GetAndSetShort org.openjdk.jcstress.tests.atomicity.varHandles.arrays.GetAndSetTest.GetAndSetByte ------------- Marked as reviewed by fyang (Lead). PR: https://git.openjdk.java.net/riscv-port/pull/15
On Mon, 29 Nov 2021 05:06:34 GMT, Fei Yang <fyang@openjdk.org> wrote:
Looks good. I can also reproduce the bug with following three jcstress tests: org.openjdk.jcstress.tests.atomicity.varHandles.fields.GetAndSetTest.GetAndSetByte org.openjdk.jcstress.tests.atomicity.varHandles.fields.GetAndSetTest.GetAndSetShort org.openjdk.jcstress.tests.atomicity.varHandles.arrays.GetAndSetTest.GetAndSetByte
Quite thanks for the supplementary information about the failed tests. A simple `test/hotspot/jtreg/compiler` on Qemu seems no error found. And these three jcstress tests seem no `FORBIDDEN` occurrences after this patch. ------------- PR: https://git.openjdk.java.net/riscv-port/pull/15
On Mon, 29 Nov 2021 03:40:43 GMT, zhengxiaolinX <duke@openjdk.java.net> wrote:
Hi team,
According to https://bugs.openjdk.java.net/browse/JDK-8277884 and [the original patch](https://github.com/riscv-collab/riscv-openjdk/pull/9):
A very simple program inherited from jcstress could reproduce this:
import java.lang.invoke.MethodHandles; import java.lang.invoke.VarHandle;
// run with: // build/linux-riscv64-server-release/images/jdk/bin/java -ea -XX:-TieredCompilation -XX:CompileCommand=compileonly,jdk.internal.misc.Unsafe::compareAndSetByte TestCASByte // assert will fail. // if we add '-XX:+UnlockDiagnosticVMOptions -XX:DisableIntrinsic=_compareAndExchangeByte' this test will pass.
public class TestCASByte {
byte[] array = new byte[1];
static final VarHandle vh; static { try { vh = MethodHandles.arrayElementVarHandle(byte[].class); } catch (Exception e) { throw new RuntimeException(e); } }
public void test() { for (int i = 0; i < 100_000; i++) { // To level 4 // clear array[0] = 0;
byte res1 = (byte)vh.getAndSet(array, 0, (byte)-1); byte res2 = (byte)vh.getAndSet(array, 0, (byte)2);
assert res1 == 0 && res2 == -1; } }
public static void main(String[] args) { new TestCASByte().test(); }
}
In this case when we are getting the `-1` value saved in memory, the shift is `0`, the old is `0x00000000000000ff` and the mask is `0x00000000000000ff` at the end of cmpxchg_narrow_value. The final result is `0x00000000000000ff` but it indeed should be `0xffffffffffffffff`. Therefore, if the result is negative, we need to make a sign extension.
Thanks, Xiaolin
This pull request has now been integrated. Changeset: 5c1cbe08 Author: yunyao.zxl <yunyao.zxl@alibaba-inc.com> Committer: Fei Yang <fyang@openjdk.org> URL: https://git.openjdk.java.net/riscv-port/commit/5c1cbe08946a940e46b91183ea7d2... Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod 8277884: riscv: Fix cmpxchg_narrow_value that needs to sign-extend non-bool results Reviewed-by: fyang ------------- PR: https://git.openjdk.java.net/riscv-port/pull/15
participants (2)
-
Fei Yang
-
zhengxiaolinX