Missing membar for constructors

[Tom Rodriguez]

> 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) {
>> 
>