Missing membar for constructors

[Doug Simon]

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.

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