Missing membar for constructors

[Andrew Haley]

[Sorry, previous patch was wrong.]


I've been getting some weird crashes, and tracked it down to a missing
membar in constructors, where a final field can be published unsafely.
Patch appended.  I found this very surprising, but there doesn't seem
to be any code anywhere in Graal to handle this correctly.

Andrew.

diff --git a/graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java b/graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java
index e51910c..a01be9e 100644
--- a/graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java
+++ b/graal/com.oracle.graal.java/src/com/oracle/graal/java/BytecodeParser.java
@@ -239,6 +239,8 @@ import static com.oracle.graal.java.BytecodeParserOptions.TraceParserPlugins;
 import static com.oracle.graal.nodes.graphbuilderconf.IntrinsicContext.CompilationContext.INLINE_DURING_PARSING;
 import static com.oracle.graal.nodes.type.StampTool.isPointerNonNull;
 import static java.lang.String.format;
+import static jdk.vm.ci.code.MemoryBarriers.LOAD_STORE;
+import static jdk.vm.ci.code.MemoryBarriers.STORE_STORE;
 import static jdk.vm.ci.meta.DeoptimizationAction.InvalidateRecompile;
 import static jdk.vm.ci.meta.DeoptimizationAction.InvalidateReprofile;
 import static jdk.vm.ci.meta.DeoptimizationReason.JavaSubroutineMismatch;
@@ -360,6 +362,7 @@ import com.oracle.graal.nodes.extended.BytecodeExceptionNode;
 import com.oracle.graal.nodes.extended.GuardedNode;
 import com.oracle.graal.nodes.extended.GuardingNode;
 import com.oracle.graal.nodes.extended.IntegerSwitchNode;
+import com.oracle.graal.nodes.extended.MembarNode;
 import com.oracle.graal.nodes.extended.ValueAnchorNode;
 import com.oracle.graal.nodes.graphbuilderconf.GraphBuilderConfiguration;
 import com.oracle.graal.nodes.graphbuilderconf.GraphBuilderConfiguration.BytecodeExceptionMode;
@@ -587,6 +590,8 @@ public class BytecodeParser implements GraphBuilderContext {

     private int lastBCI; // BCI of lastInstr. This field is for resolving instrumentation target.

+    private boolean needsReturnMembar = false; // Constructor has write to final field.
+
     protected BytecodeParser(GraphBuilderPhase.Instance graphBuilderInstance, StructuredGraph graph, BytecodeParser parent, ResolvedJavaMethod method,
                     int entryBCI, IntrinsicContext intrinsicContext) {
         this.code = intrinsicContext == null ? new ResolvedJavaMethodBytecode(method) : intrinsicContext.getBytecodeProvider().getBytecode(method);
@@ -606,6 +611,7 @@ public class BytecodeParser implements GraphBuilderContext {
         this.entryBCI = entryBCI;
         this.parent = parent;
         this.lastBCI = -1;
+        this.needsReturnMembar = false;

         assert code.getCode() != null : "method must contain bytecodes: " + method;

@@ -1216,6 +1222,9 @@ public class BytecodeParser implements GraphBuilderContext {
         StoreFieldNode storeFieldNode = new StoreFieldNode(receiver, field, value);
         append(storeFieldNode);
         storeFieldNode.setStateAfter(this.createFrameState(stream.nextBCI(), storeFieldNode));
+        if (field.isFinal() && method.isConstructor()) {
+            needsReturnMembar = true;
+        }
     }

     /**
@@ -1794,20 +1803,27 @@ public class BytecodeParser implements GraphBuilderContext {
     }

     private void beforeReturn(ValueNode x, JavaKind kind) {
-        if (graph.method() != null && graph.method().isJavaLangObjectInit()) {
-            /*
-             * Get the receiver from the initial state since bytecode rewriting could do arbitrary
-             * things to the state of the locals.
-             */
-            ValueNode receiver = graph.start().stateAfter().localAt(0);
-            assert receiver != null && receiver.getStackKind() == JavaKind.Object;
-            if (RegisterFinalizerNode.mayHaveFinalizer(receiver, graph.getAssumptions())) {
-                append(new RegisterFinalizerNode(receiver));
+        if (graph.method() != null) {
+            if (needsReturnMembar) {
+                append(new MembarNode(LOAD_STORE | STORE_STORE));
             }
-        }
-        genInfoPointNode(InfopointReason.METHOD_END, x);

-        synchronizedEpilogue(BytecodeFrame.AFTER_BCI, x, kind);
+            if (graph.method().isJavaLangObjectInit()) {
+                /*
+                 * Get the receiver from the initial state since bytecode rewriting could do
+                 * arbitrary things to the state of the locals.
+                 */
+                ValueNode receiver = graph.start().stateAfter().localAt(0);
+                assert receiver != null && receiver.getStackKind() == JavaKind.Object;
+                if (RegisterFinalizerNode.mayHaveFinalizer(receiver, graph.getAssumptions())) {
+                    append(new RegisterFinalizerNode(receiver));
+                }
+
+                genInfoPointNode(InfopointReason.METHOD_END, x);
+
+                synchronizedEpilogue(BytecodeFrame.AFTER_BCI, x, kind);
+            }
+        }
     }

     protected MonitorEnterNode createMonitorEnterNode(ValueNode x, MonitorIdNode monitorId) {