Missing membar for constructors

[Doug Simon]

We should move discussion of this patch to the github pull request once it exists.

> On 10 Nov 2016, at 21:13, Tom Rodriguez <tom.rodriguez at oracle.com> wrote:
> 
> 
>> On Nov 10, 2016, at 12:02 PM, Doug Simon <doug.simon at oracle.com> wrote:
>> 
>> Thanks Andrew, this is indeed an unfortunate bug - thanks for tracking it down.
>> 
>> Could you please submit your patch as a pull request at https://github.com/graalvm/graal-core so we can review/discuss it there.
> 
> I suspect this isn’t sufficient.  We’ll also need a barrier when we materialize CommitAllocations that include stores to final fields.  And hopefully this new barrier won’t interfere with EA.  We might need a special node to represent this so we can optimize it properly.
> 
> tom
> 
>> 
>> -Doug
>> 
>>> On 10 Nov 2016, at 19:59, Andrew Haley <aph at redhat.com> wrote:
>>> 
>>> [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) {
>>> 
>> 
>