[GR-60440] UnstructuredLocking: Cut off control flow at merges with incompatible lock stacks.

PullRequest: graal/19643

Author: rmosaner

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java

@@ -864,17 +864,24 @@ protected static final class Target {
         final FixedNode entry;
         final FixedNode originalEntry;
         final FrameStateBuilder state;
+        final boolean reachable;
 
         public Target(FixedNode entry, FrameStateBuilder state) {
+            this(entry, state, null);
+        }
+
+        public Target(FixedNode entry, FrameStateBuilder state, FixedNode originalEntry) {
             this.entry = entry;
             this.state = state;
-            this.originalEntry = null;
+            this.originalEntry = originalEntry;
+            this.reachable = true;
         }
 
-        public Target(FixedNode entry, FrameStateBuilder state, FixedNode originalEntry) {
+        public Target(FixedNode entry, FrameStateBuilder state, FixedNode originalEntry, boolean reachable) {
             this.entry = entry;
             this.state = state;
             this.originalEntry = originalEntry;
+            this.reachable = reachable;
         }
 
         public FixedNode getEntry() {
@@ -888,6 +895,15 @@ public FixedNode getOriginalEntry() {
         public FrameStateBuilder getState() {
             return state;
         }
+
+        /**
+         * Indicates whether the target block is actually reachable. If not, {@link #getEntry()}
+         * will lead to a dead end (e.g., exception). Thus, no ends must be added to the target
+         * block's merge.
+         */
+        public boolean isReachable() {
+            return reachable;
+        }
     }
 
     @SuppressWarnings("serial")
@@ -3438,6 +3454,13 @@ private FixedNode createTarget(BciBlock block, FrameStateBuilder state, boolean
                 } else {
                     setFirstInstruction(block, graph.add(new BeginNode()));
                 }
+                /*
+                 * The target is the block's first instruction which may be preceded by exits of
+                 * loops or exception handling that must be done before the jump. The target.entry
+                 * holds the start of this sequence of operations. As the block is seen the first
+                 * time as jump target, we cannot check for unstructured locking, as this requires
+                 * to compare the lock stacks of two framestates to be merged.
+                 */
                 Target target = checkLoopExit(checkUnwind(getFirstInstruction(block), block, state), block);
                 FixedNode result = target.entry;
                 FrameStateBuilder currentEntryState = target.state == state ? (canReuseState ? state : state.copy()) : target.state;
@@ -3461,6 +3484,11 @@ private FixedNode createTarget(BciBlock block, FrameStateBuilder state, boolean
                     // location instead of the destination.
                     loopEnd = graph.add(new LoopEndNode(loopBegin));
                 }
+                /*
+                 * The target is created from the loopEnd which may be preceded by exits of loops or
+                 * exception handling that must be done before the jump. The target.entry holds the
+                 * start of this sequence of operations.
+                 */
                 Target target = checkUnstructuredLocking(checkLoopExit(new Target(loopEnd, state.copy()), block), block, getEntryState(block));
                 FixedNode result = target.entry;
                 /*
@@ -3471,15 +3499,30 @@ private FixedNode createTarget(BciBlock block, FrameStateBuilder state, boolean
                 assert !(block instanceof ExceptionDispatchBlock) : block;
                 assert !getEntryState(block).rethrowException();
                 target.state.setRethrowException(false);
-                getEntryState(block).merge(loopBegin, target.state);
-                debug.log("createTarget %s: merging backward branch to loop header %s, result: %s", block, loopBegin, result);
+
+                if (target.isReachable()) {
+                    getEntryState(block).merge(loopBegin, target.state);
+                    debug.log("createTarget %s: merging backward branch to loop header %s, result: %s", block, loopBegin, result);
+                } else {
+                    debug.log("createTarget %s: Unreachable target. This could be due to an exception being thrown (e.g., unstructured locking).", block);
+                }
 
                 return result;
             }
             assert currentBlock == null || currentBlock.getId() < block.getId() : "must not be backward branch";
             assert getFirstInstruction(block).next() == null : "bytecodes already parsed for block";
 
-            if (getFirstInstruction(block) instanceof AbstractBeginNode && !(getFirstInstruction(block) instanceof AbstractMergeNode)) {
+            // The EndNode for the new edge to merge.
+            EndNode newEnd = graph.add(new EndNode());
+            /*
+             * The target is created from the newEnd which may be preceded by exits of loops or
+             * exception handling that must be done before the jump. The target.entry holds the
+             * start of this sequence of operations.
+             */
+            Target target = checkUnstructuredLocking(checkLoopExit(checkUnwind(newEnd, block, state), block), block, getEntryState(block));
+            FixedNode result = target.entry;
+
+            if (target.isReachable() && getFirstInstruction(block) instanceof AbstractBeginNode && !(getFirstInstruction(block) instanceof AbstractMergeNode)) {
                 /*
                  * This is the second time we see this block. Create the actual MergeNode and the
                  * End Node for the already existing edge.
@@ -3505,15 +3548,14 @@ private FixedNode createTarget(BciBlock block, FrameStateBuilder state, boolean
                 setFirstInstruction(block, mergeNode);
             }
 
-            AbstractMergeNode mergeNode = (AbstractMergeNode) getFirstInstruction(block);
-
-            // The EndNode for the newly merged edge.
-            EndNode newEnd = graph.add(new EndNode());
-            Target target = checkUnstructuredLocking(checkLoopExit(checkUnwind(newEnd, block, state), block), block, getEntryState(block));
-            FixedNode result = target.entry;
-            getEntryState(block).merge(mergeNode, target.state);
-            mergeNode.addForwardEnd(newEnd);
-            debug.log("createTarget %s: merging state, result: %s", block, result);
+            if (target.isReachable()) {
+                AbstractMergeNode mergeNode = (AbstractMergeNode) getFirstInstruction(block);
+                getEntryState(block).merge(mergeNode, target.state);
+                mergeNode.addForwardEnd(newEnd);
+                debug.log("createTarget %s: merging state, result: %s", block, result);
+            } else {
+                debug.log("createTarget %s: Unreachable target. This could be due to an exception being thrown (e.g., unstructured locking).", block);
+            }
 
             return result;
         }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/FrameStateBuilder.java

@@ -1210,17 +1210,4 @@ public FrameState createInitialIntrinsicFrameState(ResolvedJavaMethod original)
                         new FrameState(null, new ResolvedJavaMethodBytecode(original), 0, newLocals, newStack, stackSize, null, null, locks, Collections.emptyList(), FrameState.StackState.BeforePop));
         return stateAfterStart;
     }
-
-    /**
-     * Sets monitorIds and lockedObjects of this FrameStateBuilder to the values in {@code from}.
-     */
-    public void setLocks(FrameStateBuilder from) {
-        lockedObjects = new ValueNode[from.lockedObjects.length];
-        monitorIds = new MonitorIdNode[from.lockedObjects.length];
-        for (int i = 0; i < from.lockedObjects.length; i++) {
-            lockedObjects[i] = from.lockedObjects[i];
-            monitorIds[i] = from.monitorIds[i];
-        }
-    }
-
 }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/PhiNode.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -77,9 +77,24 @@ public void setMerge(AbstractMergeNode x) {
     public boolean verifyNode() {
         assertTrue(merge() != null, "missing merge");
         assertTrue(merge().phiPredecessorCount() == valueCount(), "mismatch between merge predecessor count and phi value count: %d != %d", merge().phiPredecessorCount(), valueCount());
+        verifyNoIllegalSelfLoops();
         return super.verifyNode();
     }
 
+    private void verifyNoIllegalSelfLoops() {
+        if (!(merge instanceof LoopBeginNode)) {
+            for (int i = 0; i < valueCount(); i++) {
+                GraalError.guarantee(valueAt(i) != this, "non-loop phi at merge %s must not have a cycle, but value at index %s is itself: %s", merge(), i, valueAt(i));
+            }
+        }
+    }
+
+    private void verifyNoIllegalSelfLoop(ValueNode value) {
+        if (!(merge instanceof LoopBeginNode)) {
+            GraalError.guarantee(value != this, "non-loop phi at merge %s must not have a cycle, but value to be added is itself: %s", merge(), value);
+        }
+    }
+
     /**
      * Get the instruction that produces the value associated with the i'th predecessor of the
      * merge.
@@ -98,13 +113,15 @@ public ValueNode valueAt(int i) {
      * @param x the new phi input value for the given location
      */
     public void initializeValueAt(int i, ValueNode x) {
+        verifyNoIllegalSelfLoop(x);
         while (values().size() <= i) {
             values().add(null);
         }
         values().set(i, x);
     }
 
     public void setValueAt(int i, ValueNode x) {
+        verifyNoIllegalSelfLoop(x);
         values().set(i, x);
     }
 
@@ -158,6 +175,7 @@ public void addInput(ValueNode x) {
                         "x",
                         x);
         assert !(this instanceof ValuePhiNode) || x.stamp(NodeView.DEFAULT).isCompatible(stamp(NodeView.DEFAULT)) : Assertions.errorMessageContext("this", this, "x", x);
+        verifyNoIllegalSelfLoop(x);
         values().add(x);
     }
 

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java

@@ -748,52 +748,31 @@ protected Target checkUnstructuredLocking(Target target, BciBlock targetBlock, F
             if (mergeState.areLocksMergeableWith(target.getState())) {
                 return target;
             }
-
-            /*
-             * If the current locks are not compatible with the target merge,
-             * the following code is created
-             *
-             * @formatter:off
-             *
-             * if(false)                // UnreachableBeginNode
-             *   goto originalTarget    // using adapted locks
-             * else
-             *   releaseMonitors()      // also methodSynchronizedObject
-             *   throw new UnsupportedFeatureError()
-             *
-             * @formatter:on
-             *
-             * Returns the newly created IfNode as updated target.
-             */
+            // Create an UnsupportedFeatureException and unwind.
             FixedWithNextNode originalLast = lastInstr;
             FrameStateBuilder originalState = frameState;
 
-            IfNode ifNode = graph.add(new IfNode(LogicConstantNode.contradiction(graph), graph.add(new UnreachableBeginNode()), graph.add(new BeginNode()), BranchProbabilityNode.NEVER_TAKEN_PROFILE));
-
-            /*
-             * Create an UnsupportedFeatureException in the always entered (false) branch and
-             * unwind.
-             */
-            lastInstr = ifNode.falseSuccessor();
+            BeginNode holder = graph.add(new BeginNode());
+            lastInstr = holder;
             frameState = target.getState().copy();
             genReleaseMonitors(true);
             genThrowUnsupportedFeatureError("Incompatible lock states at merge. Native Image enforces structured locking (JVMS 2.11.10)");
 
-            /*
-             * Update the never entered (true) branch to have a matching lock stack with the
-             * subsequent merge. This branch will fold away during canonicalization. Add an
-             * UnreachableNode as assurance.
-             */
+            FixedNode exceptionPath = holder.next();
+
             Target newTarget;
-            FrameStateBuilder newState = target.getState().copy();
-            newState.setLocks(mergeState);
             if (target.getOriginalEntry() == null) {
-                newTarget = new Target(ifNode, newState, target.getEntry());
+                newTarget = new Target(exceptionPath, frameState, null, false);
+                target.getEntry().replaceAtPredecessor(exceptionPath);
+                target.getEntry().safeDelete();
             } else {
-                target.getOriginalEntry().replaceAtPredecessor(ifNode);
-                newTarget = new Target(target.getEntry(), newState, target.getOriginalEntry());
+                newTarget = new Target(target.getEntry(), frameState, exceptionPath, false);
+                target.getOriginalEntry().replaceAtPredecessor(exceptionPath);
+                target.getOriginalEntry().safeDelete();
             }
-            ifNode.trueSuccessor().setNext(newTarget.getOriginalEntry());
+
+            holder.setNext(null);
+            holder.safeDelete();
 
             lastInstr = originalLast;
             frameState = originalState;