Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8175887

C1 value numbering handling of Unsafe.get*Volatile is incorrect

    Details

    • Subcomponent:
    • Resolved In Build:
      b161
    • Verification:
      Verified

      Backports

        Description

        This looks eerily similar to JDK-7170145, but now for Unsafe.get*Volatile handling in C1. We seem to miss the case for volatile ops like this:

        $ hg diff src/share/vm/c1/c1_ValueMap.hpp
        diff -r dedf248e8e3e src/share/vm/c1/c1_ValueMap.hpp
        --- a/src/share/vm/c1/c1_ValueMap.hpp Mon Feb 13 10:37:33 2017 -0500
        +++ b/src/share/vm/c1/c1_ValueMap.hpp Mon Feb 27 10:33:01 2017 +0100
        @@ -198,7 +198,11 @@
           void do_ExceptionObject(ExceptionObject* x) { /* nothing to do */ }
           void do_RoundFP (RoundFP* x) { /* nothing to do */ }
           void do_UnsafeGetRaw (UnsafeGetRaw* x) { /* nothing to do */ }
        - void do_UnsafeGetObject(UnsafeGetObject* x) { /* nothing to do */ }
        + void do_UnsafeGetObject(UnsafeGetObject* x) {
        + if (x->is_volatile()) {
        + kill_memory();
        + }
        + }
           void do_ProfileCall (ProfileCall* x) { /* nothing to do */ }
           void do_ProfileReturnType (ProfileReturnType* x) { /* nothing to do */ }
           void do_ProfileInvoke (ProfileInvoke* x) { /* nothing to do */ };

        This might explain a few failures in jcstress, like this -- although not confirmed as the actual culprit:
         http://openjdk.linaro.org/jdk9/jcstress-nightly-runs/2017/056/results/org.openjdk.jcstress.tests.coherence.varHandles.fields.opaque.ShortTest.html

          Issue Links

            Activity

            Show
            shade Aleksey Shipilev added a comment - RFC: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-February/025731.html
            Hide
            shade Aleksey Shipilev added a comment - - edited
            Confirmed on current jdk9-hs Linux x86_64 with targeted jcstress test:
             http://hg.openjdk.java.net/code-tools/jcstress/file/0400ed6dd6a6/tests-custom/src/main/java/org/openjdk/jcstress/tests/unsafe/UnsafeReadTwiceOverVolatileReadTest.java

            Exception in thread "main" java.lang.AssertionError: TEST FAILURES:
            org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -XX:TieredStopAtLevel=1]: Observed forbidden state: 0, 1, 0
            org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -server]: Observed forbidden state: 0, 1, 0
            org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -client]: Observed forbidden state: 0, 1, 0
            org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: 0, 1, 0

            Seems fixed with the patch above.
            Show
            shade Aleksey Shipilev added a comment - - edited Confirmed on current jdk9-hs Linux x86_64 with targeted jcstress test:   http://hg.openjdk.java.net/code-tools/jcstress/file/0400ed6dd6a6/tests-custom/src/main/java/org/openjdk/jcstress/tests/unsafe/UnsafeReadTwiceOverVolatileReadTest.java Exception in thread "main" java.lang.AssertionError: TEST FAILURES: org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -XX:TieredStopAtLevel=1]: Observed forbidden state: 0, 1, 0 org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -server]: Observed forbidden state: 0, 1, 0 org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -client]: Observed forbidden state: 0, 1, 0 org.openjdk.jcstress.tests.unsafe.UnsafeReadTwiceOverVolatileReadTest [-XX:+UnlockDiagnosticVMOptions, -XX:-UseNewCode, -server, -XX:+UnlockDiagnosticVMOptions, -XX:+StressLCM, -XX:+StressGCM]: Observed forbidden state: 0, 1, 0 Seems fixed with the patch above.
            Hide
            shade Aleksey Shipilev added a comment -
            As in JDK-7170145, we should probably fix the put*Volatile too. jcstress would be amended to test memory effects of unsafe ops too (it does test volatile-keyword ops).
            Show
            shade Aleksey Shipilev added a comment - As in JDK-7170145 , we should probably fix the put*Volatile too. jcstress would be amended to test memory effects of unsafe ops too (it does test volatile-keyword ops).
            Show
            shade Aleksey Shipilev added a comment - RFR: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2017-February/025732.html
            Hide
            zmajo Zoltan Majo (Inactive) added a comment -
            ILW=breaking specification, so far observed with a number of jcstress tests, no workaround (or use -XX:-TieredCompilation) = HLH
            Show
            zmajo Zoltan Majo (Inactive) added a comment - ILW=breaking specification, so far observed with a number of jcstress tests, no workaround (or use -XX:-TieredCompilation) = HLH
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/2ff05d967fb2
            User: vlivanov
            Date: 2017-03-01 13:32:11 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/hs/hotspot/rev/2ff05d967fb2 User: vlivanov Date: 2017-03-01 13:32:11 +0000
            Hide
            hgupdate HG Updates added a comment -
            URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/2ff05d967fb2
            User: lana
            Date: 2017-03-15 14:49:20 +0000
            Show
            hgupdate HG Updates added a comment - URL: http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/2ff05d967fb2 User: lana Date: 2017-03-15 14:49:20 +0000
            Hide
            kvn Vladimir Kozlov added a comment -
            Verified with new added tests.
            Show
            kvn Vladimir Kozlov added a comment - Verified with new added tests.

              People

              • Assignee:
                shade Aleksey Shipilev
                Reporter:
                shade Aleksey Shipilev
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: