From 2bdca2299320711557aef92bcbb06ea7d53bbe6d Mon Sep 17 00:00:00 2001 From: Raphael Mosaner Date: Tue, 17 Dec 2024 13:32:17 +0100 Subject: [PATCH 1/2] Cut off control flow at merges with incompatible lock stacks (i.e., unstructured locking). (cherry picked from commit 95f4e95f39566b51e6b55ce3128b8caa2a3c7c48) --- .../graal/compiler/java/BytecodeParser.java | 70 +++++++++++++++---- .../compiler/java/FrameStateBuilder.java | 13 ---- .../phases/SharedGraphBuilderPhase.java | 49 ++++--------- 3 files changed, 70 insertions(+), 62 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java index 683ec84de240..17a9834f0c19 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java +++ b/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; @@ -3456,6 +3479,11 @@ private FixedNode createTarget(BciBlock block, FrameStateBuilder state, boolean */ LoopBeginNode loopBegin = (LoopBeginNode) getFirstInstruction(block); LoopEndNode 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; /* @@ -3466,15 +3494,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. @@ -3500,15 +3543,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; } diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/FrameStateBuilder.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/FrameStateBuilder.java index c82ddaeb01db..ee039e1de619 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/FrameStateBuilder.java +++ b/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]; - } - } - } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java index b4e360ac9e0e..d1363b316abe 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SharedGraphBuilderPhase.java +++ b/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; From 3b0c36a2022f056962f2e6f04107172f8bf35616 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C3=B6=20Barany?= Date: Fri, 13 Dec 2024 18:08:05 +0100 Subject: [PATCH 2/2] Check for self-cycles on non-loop phis. (cherry picked from commit 3f8a6d50893f994f234bdab1f5bc8392b5ee5b17) --- .../src/jdk/graal/compiler/nodes/PhiNode.java | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/PhiNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/PhiNode.java index 0697d99e8757..80acfca3e478 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/PhiNode.java +++ b/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,6 +113,7 @@ 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); } @@ -105,6 +121,7 @@ public void initializeValueAt(int i, ValueNode 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); }