JDK-7170319 : Bug in GVN code in C1 breaks Java volatile semantics
  • Type: Bug
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 6
  • Priority: P2
  • Status: Closed
  • Resolution: Duplicate
  • OS: generic
  • CPU: generic
  • Submitted: 2012-05-21
  • Updated: 2012-05-21
  • Resolved: 2012-05-21
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
Other
hs24Resolved
Related Reports
Duplicate :  
Relates :  
Description
As reported on:

http://stackoverflow.com/questions/10620680/why-volatile-in-java-5-doesnt-synchronize-cached-copies-of-variables-with-main

and discussed on:

http://cs.oswego.edu/pipermail/concurrency-interest/2012-May/009440.html

and

http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2012-May/007704.html

The following test program reports "Error" where under the Java Memory Model rules it never should:

public class VolatileBug {
    volatile static private int a;
    static private int b;

    public static void main(String [] args) throws Exception {
        for (int i = 0; i < 100; i++) {
            new Thread() {

                @Override
                public void run() {
                    int tt = b; // makes the jvm cache the value of b

                    while (a==0) {

                    }

                    if (b == 0) {
                        System.out.println("error");
                    }
                }

            }.start();
        }

        b = 1;
        a = 1;
    }
}

Investigation shows the problem is in the GlobalValueNumbering code where the load of the volatile field doesn't kill the prevous load of a non-volatile field, resulting in the old value incorrectly being re-used after the volatile field is read.

Comments
WORK AROUND In a fastdebug build you can use: -XX:-UseGlobalOrderNumbering though it may impact performance significantly.
21-05-2012

EVALUATION Christian Thalinger writes: The fix is actually very simple: diff --git a/src/share/vm/c1/c1_ValueMap.hpp b/src/share/vm/c1/c1_ValueMap.hpp --- a/src/share/vm/c1/c1_ValueMap.hpp +++ b/src/share/vm/c1/c1_ValueMap.hpp @@ -160,8 +160,8 @@ void do_Local (Local* x) { /* nothing to do */ } void do_Constant (Constant* x) { /* nothing to do */ } void do_LoadField (LoadField* x) { - if (x->is_init_point()) { - // getstatic is an initialization point so treat it as a wide kill + if (x->is_init_point() || // getstatic is an initialization point so treat it as a wide kill + x->field()->is_volatile()) { // The JMM requires this. kill_memory(); } } This kills all loads after the volatile load.
21-05-2012