[GR-48643] Prohibit recursion in compiler phases.

PullRequest: graal/15606

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/CheckGraalInvariants.java

@@ -344,6 +344,7 @@ public static void runTest(InvariantsTool tool) {
         verifiers.add(new VerifyPluginFrameState());
         verifiers.add(new VerifyGraphUniqueUsages());
         verifiers.add(new VerifyEndlessLoops());
+        verifiers.add(new VerifyPhaseNoDirectRecursion());
         VerifyAssertionUsage assertionUsages = null;
         boolean checkAssertions = tool.checkAssertions();
 

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/VerifyPhaseNoDirectRecursion.java

@@ -0,0 +1,77 @@
+/*
+ * Copyright (c) 2023, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.graal.compiler.core.test;
+
+import jdk.graal.compiler.nodes.StructuredGraph;
+import jdk.graal.compiler.nodes.java.MethodCallTargetNode;
+import jdk.graal.compiler.nodes.spi.CoreProviders;
+import jdk.graal.compiler.phases.BasePhase;
+import jdk.graal.compiler.phases.RecursivePhase;
+import jdk.graal.compiler.phases.VerifyPhase;
+import jdk.vm.ci.meta.ResolvedJavaMethod;
+import jdk.vm.ci.meta.ResolvedJavaType;
+
+/**
+ * Verify that no subclass of {@link BasePhase} uses direct recursion in its implementation. Direct
+ * recursion can easily cause stack overflows, endless recursions, etc. The graal optimizer code
+ * base must use iteration instead.
+ */
+public class VerifyPhaseNoDirectRecursion extends VerifyPhase<CoreProviders> {
+
+    @Override
+    public boolean checkContract() {
+        return false;
+    }
+
+    @Override
+    protected void verify(StructuredGraph graph, CoreProviders context) {
+        final ResolvedJavaType basePhaseType = context.getMetaAccess().lookupJavaType(BasePhase.class);
+        final ResolvedJavaType recursivePhaseType = context.getMetaAccess().lookupJavaType(RecursivePhase.class);
+        final ResolvedJavaMethod graphMethod = graph.method();
+        final ResolvedJavaType graphMethodType = graph.method().getDeclaringClass();
+
+        if (graphMethod.getDeclaringClass().getName().contains("truffle")) {
+            // We only enforce this code requirement for core compiler code and ignore truffle.
+            return;
+        }
+        if (!basePhaseType.isAssignableFrom(graphMethodType)) {
+            // Not a compiler phase.
+            return;
+        }
+        if (recursivePhaseType.isAssignableFrom(graphMethodType)) {
+            // Some exclude listed phases.
+            return;
+        }
+        for (MethodCallTargetNode m : graph.getNodes(MethodCallTargetNode.TYPE)) {
+            ResolvedJavaMethod callee = m.targetMethod();
+            if (callee.equals(graphMethod)) {
+                throw new VerificationError(
+                                "Call %s in %s is a self recursive call in a phase subclass. This is prohibited for performance and safety reasons. Replace with iteration. " +
+                                                "(Exceptions can be made for very stable well understood code, consider whitlisting in VerifyPhaseNoDirectRecursion.java).",
+                                m.invoke(), m.invoke().asNode().getNodeSourcePosition());
+            }
+        }
+    }
+}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/loop/phases/ConvertDeoptimizeToGuardPhase.java

@@ -71,6 +71,7 @@
 import jdk.graal.compiler.nodes.spi.Simplifiable;
 import jdk.graal.compiler.nodes.spi.SimplifierTool;
 import jdk.graal.compiler.nodes.util.GraphUtil;
+import jdk.graal.compiler.phases.RecursivePhase;
 import jdk.graal.compiler.phases.common.CanonicalizerPhase;
 import jdk.graal.compiler.phases.common.DeadCodeEliminationPhase;
 import jdk.graal.compiler.phases.common.LazyValue;
@@ -89,7 +90,7 @@
  * branch starting at an other kind of {@link ControlSplitNode}, it will only bring the
  * {@link DeoptimizeNode} as close to the {@link ControlSplitNode} as possible.
  */
-public class ConvertDeoptimizeToGuardPhase extends PostRunCanonicalizationPhase<CoreProviders> {
+public class ConvertDeoptimizeToGuardPhase extends PostRunCanonicalizationPhase<CoreProviders> implements RecursivePhase {
 
     public ConvertDeoptimizeToGuardPhase(CanonicalizerPhase canonicalizer) {
         super(canonicalizer);

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/PhaseSuite.java

@@ -46,7 +46,7 @@
 /**
  * A compiler phase that can apply an ordered collection of phases to a graph.
  */
-public class PhaseSuite<C> extends BasePhase<C> implements PhasePlan<BasePhase<? super C>> {
+public class PhaseSuite<C> extends BasePhase<C> implements PhasePlan<BasePhase<? super C>>, RecursivePhase {
 
     public static class Options {
         @Option(help = "Prints the difference in the graph state caused by each phase of the suite.") //

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/RecursivePhase.java

@@ -0,0 +1,32 @@
+/*
+ * Copyright (c) 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.graal.compiler.phases;
+
+/**
+ * Marker interface for phases that use recursion during processing.
+ */
+public interface RecursivePhase {
+
+}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/ExpandLogicPhase.java

@@ -32,6 +32,7 @@
 import jdk.graal.compiler.debug.GraalError;
 import jdk.graal.compiler.graph.Graph;
 import jdk.graal.compiler.graph.Node;
+import jdk.graal.compiler.graph.NodeStack;
 import jdk.graal.compiler.nodes.AbstractBeginNode;
 import jdk.graal.compiler.nodes.AbstractMergeNode;
 import jdk.graal.compiler.nodes.BeginNode;
@@ -103,22 +104,33 @@ private static void processNormalizeCompareNode(AbstractNormalizeCompareNode nor
     }
 
     @SuppressWarnings("try")
-    private static void expandBinary(ShortCircuitOrNode binary) {
-        while (binary.usages().isNotEmpty()) {
-            Node usage = binary.usages().first();
-            try (DebugCloseable nsp = usage.withNodeSourcePosition()) {
-                if (usage instanceof ShortCircuitOrNode) {
-                    expandBinary((ShortCircuitOrNode) usage);
-                } else if (usage instanceof IfNode) {
-                    processIf(binary.getX(), binary.isXNegated(), binary.getY(), binary.isYNegated(), (IfNode) usage, binary.getShortCircuitProbability().getDesignatedSuccessorProbability());
-                } else if (usage instanceof ConditionalNode) {
-                    processConditional(binary.getX(), binary.isXNegated(), binary.getY(), binary.isYNegated(), (ConditionalNode) usage);
-                } else {
-                    throw GraalError.shouldNotReachHereUnexpectedValue(usage); // ExcludeFromJacocoGeneratedReport
+    private static void expandBinary(ShortCircuitOrNode s) {
+        NodeStack toProcess = new NodeStack();
+        toProcess.push(s);
+
+        outer: while (!toProcess.isEmpty()) {
+            ShortCircuitOrNode binary = (ShortCircuitOrNode) toProcess.pop();
+
+            while (binary.usages().isNotEmpty()) {
+                Node usage = binary.usages().first();
+                try (DebugCloseable nsp = usage.withNodeSourcePosition()) {
+                    if (usage instanceof ShortCircuitOrNode) {
+                        toProcess.push(binary);
+                        toProcess.push(usage);
+                        continue outer;
+                    } else if (usage instanceof IfNode) {
+                        processIf(binary.getX(), binary.isXNegated(), binary.getY(), binary.isYNegated(), (IfNode) usage, binary.getShortCircuitProbability().getDesignatedSuccessorProbability());
+                    } else if (usage instanceof ConditionalNode) {
+                        processConditional(binary.getX(), binary.isXNegated(), binary.getY(), binary.isYNegated(), (ConditionalNode) usage);
+                    } else {
+                        throw GraalError.shouldNotReachHereUnexpectedValue(usage); // ExcludeFromJacocoGeneratedReport
+                    }
                 }
             }
+            binary.safeDelete();
+
         }
-        binary.safeDelete();
+
     }
 
     private static void processIf(LogicNode x, boolean xNegated, LogicNode y, boolean yNegated, IfNode ifNode, double shortCircuitProbability) {

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/FloatingReadPhase.java

@@ -79,10 +79,11 @@
 import jdk.graal.compiler.nodes.memory.address.AddressNode;
 import jdk.graal.compiler.nodes.spi.CoreProviders;
 import jdk.graal.compiler.nodes.util.GraphUtil;
+import jdk.graal.compiler.phases.RecursivePhase;
 import jdk.graal.compiler.phases.common.util.EconomicSetNodeEventListener;
 import jdk.graal.compiler.phases.graph.ReentrantNodeIterator;
 
-public class FloatingReadPhase extends PostRunCanonicalizationPhase<CoreProviders> {
+public class FloatingReadPhase extends PostRunCanonicalizationPhase<CoreProviders> implements RecursivePhase {
 
     private final boolean createMemoryMapNodes;
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/phases/RemoveUnwindPhase.java

@@ -70,7 +70,7 @@ protected void run(StructuredGraph graph) {
         List<WithExceptionNode> withExceptionNodes = new ArrayList<>();
         List<BytecodeExceptionNode> bytecodeExceptionNodes = new ArrayList<>();
         for (UnwindNode node : graph.getNodes(UnwindNode.TYPE)) {
-            walkBack(node.predecessor(), node, withExceptionNodes, bytecodeExceptionNodes, GraphUtil.unproxify(node.exception()), graph);
+            walkBack(node.predecessor(), node, GraphUtil.unproxify(node.exception()), withExceptionNodes, bytecodeExceptionNodes);
         }
 
         /*
@@ -91,47 +91,80 @@ protected void run(StructuredGraph graph) {
         }
     }
 
+    private record WorklistEntry(Node current, Node successor, ValueNode expectedException) {
+    }
+
     /**
      * We walk back from the {@link UnwindNode} to a {@link WithExceptionNode}. If the control flow
-     * path only contains nodes white-listed in this method, then we know that we have a node that
-     * just forwards the exception to the {@link UnwindNode}. Such nodes are rewritten to a variant
-     * without an exception edge, i.e., no exception handler entry is created for such invokes.
+     * path only contains nodes explicitly allowed in this method, then we know that we have a node
+     * that just forwards the exception to the {@link UnwindNode}. Such nodes are rewritten to a
+     * variant without an exception edge, i.e., no exception handler entry is created for such
+     * invokes.
      */
-    protected static void walkBack(Node n, Node successor, List<WithExceptionNode> withExceptionNodes, List<BytecodeExceptionNode> bytecodeExceptionNodes, ValueNode expectedExceptionNode,
-                    StructuredGraph graph) {
-        if (n instanceof WithExceptionNode) {
-            WithExceptionNode node = (WithExceptionNode) n;
-            if (node.exceptionEdge() == successor) {
-                withExceptionNodes.add(node);
-            }
-        } else if (n instanceof BytecodeExceptionNode || n instanceof ExceptionObjectNode) {
-            if (n == expectedExceptionNode) {
-                if (n instanceof BytecodeExceptionNode) {
-                    BytecodeExceptionNode node = (BytecodeExceptionNode) n;
-                    bytecodeExceptionNodes.add(node);
+    protected static void walkBack(Node initialNode, Node initialSuccessor, ValueNode initialExpectedException,
+                    List<WithExceptionNode> withExceptionNodes, List<BytecodeExceptionNode> bytecodeExceptionNodes) {
+        Node current = initialNode;
+        Node successor = initialSuccessor;
+        ValueNode expectedException = initialExpectedException;
+
+        /*
+         * The worklist is used when a node has multiple predecessors. The inner loop walks back
+         * single-predecessor nodes to avoid allocating unnecessary worklist entries.
+         */
+        List<WorklistEntry> worklist = new ArrayList<>();
+        while (true) {
+            nodeWalk: while (true) {
+                if (current instanceof WithExceptionNode node) {
+                    if (node.exceptionEdge() == successor) {
+                        withExceptionNodes.add(node);
+                    }
+                    break;
+
+                } else if (current instanceof BytecodeExceptionNode || current instanceof ExceptionObjectNode) {
+                    if (current == expectedException) {
+                        if (current instanceof BytecodeExceptionNode) {
+                            BytecodeExceptionNode node = (BytecodeExceptionNode) current;
+                            bytecodeExceptionNodes.add(node);
+                        } else {
+                            successor = current;
+                            current = current.predecessor();
+                            continue nodeWalk;
+                        }
+                    }
+                    // bailout here, the node does not flow into the corresponding unwind, its a
+                    // unrelated exception, but a different one than the unwind one
+                    break;
+                } else if (current instanceof MergeNode merge) {
+                    if (merge.isPhiAtMerge(expectedException)) {
+                        /* Propagate expected exception on each control path leading to the merge */
+                        PhiNode expectedExceptionForInput = (PhiNode) expectedException;
+                        for (int input = 0; input < merge.forwardEndCount(); input++) {
+                            Node predecessor = merge.forwardEndAt(input);
+                            worklist.add(new WorklistEntry(predecessor, merge, GraphUtil.unproxify(expectedExceptionForInput.valueAt(input))));
+                        }
+                    } else {
+                        for (ValueNode predecessor : merge.cfgPredecessors()) {
+                            worklist.add(new WorklistEntry(predecessor, merge, expectedException));
+                        }
+                    }
+                    break;
+
+                } else if (current instanceof AbstractBeginNode || current instanceof AbstractEndNode) {
+                    successor = current;
+                    current = current.predecessor();
+
                 } else {
-                    walkBack(n.predecessor(), n, withExceptionNodes, bytecodeExceptionNodes, expectedExceptionNode, graph);
-                }
-            } else {
-                graph.getDebug().log(DebugContext.VERY_DETAILED_LEVEL, "Node %s does not flow to the corresponding Unwind. Bailing out.", n);
-            }
-        } else if (n instanceof MergeNode) {
-            MergeNode merge = (MergeNode) n;
-            if (merge.isPhiAtMerge(expectedExceptionNode)) {
-                /* Propagate expected exception on each control path leading to the merge */
-                PhiNode expectedExceptionForInput = (PhiNode) expectedExceptionNode;
-                for (int input = 0; input < merge.forwardEndCount(); input++) {
-                    Node predecessor = merge.forwardEndAt(input);
-                    walkBack(predecessor, merge, withExceptionNodes, bytecodeExceptionNodes, GraphUtil.unproxify(expectedExceptionForInput.valueAt(input)), graph);
-                }
-            } else {
-                for (ValueNode predecessor : merge.cfgPredecessors()) {
-                    walkBack(predecessor, merge, withExceptionNodes, bytecodeExceptionNodes, expectedExceptionNode, graph);
+                    break;
                 }
             }
 
-        } else if (n instanceof AbstractBeginNode || n instanceof AbstractEndNode) {
-            walkBack(n.predecessor(), n, withExceptionNodes, bytecodeExceptionNodes, expectedExceptionNode, graph);
+            if (worklist.isEmpty()) {
+                break;
+            }
+            var entry = worklist.removeLast();
+            current = entry.current;
+            successor = entry.successor;
+            expectedException = entry.expectedException;
         }
     }