Canonicalize class initialization checks coming from invocation plugins during InlineBeforeAnalysis / SimulateClassInitializer

Christian Wimmer · Christian Wimmer · commit c60b247b052a · 2023-07-20T15:39:20.000-07:00
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/java/BytecodeParser.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/java/BytecodeParser.java
@@ -2861,7 +2861,7 @@ protected ConstantNode appendConstant(JavaConstant constant) {
     }
 
     @Override
-    public <T extends ValueNode> T append(T v) {
+    public <T extends Node> T append(T v) {
         assert !graph.trackNodeSourcePosition() || graph.currentNodeSourcePosition() != null || currentBlock == blockMap.getUnwindBlock() || currentBlock instanceof ExceptionDispatchBlock;
         if (v.graph() != null) {
             return v;
@@ -2873,7 +2873,7 @@ public <T extends ValueNode> T append(T v) {
         return added;
     }
 
-    private <T extends ValueNode> void updateLastInstruction(T v) {
+    private <T extends Node> void updateLastInstruction(T v) {
         if (v instanceof FixedNode) {
             FixedNode fixedNode = (FixedNode) v;
             if (lastInstr != null) {
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/graphbuilderconf/GraphBuilderContext.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/graphbuilderconf/GraphBuilderContext.java
@@ -35,6 +35,7 @@
 import org.graalvm.compiler.core.common.type.StampFactory;
 import org.graalvm.compiler.core.common.type.StampPair;
 import org.graalvm.compiler.debug.GraalError;
+import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.nodes.AbstractBeginNode;
 import org.graalvm.compiler.nodes.AbstractMergeNode;
 import org.graalvm.compiler.nodes.BeginNode;
@@ -90,7 +91,7 @@ public interface GraphBuilderContext extends GraphBuilderTool {
      *
      * @param kind the kind to use when type checking this operation
      * @param value the value to push to the stack. The value must already have been
-     *            {@linkplain #append(ValueNode) appended}.
+     *            {@linkplain #append(Node) appended}.
      */
     void push(JavaKind kind, ValueNode value);
 
@@ -121,14 +122,24 @@ default ValueNode[] popArguments(int argSize) {
      *            type checking this operation.
      * @return a node equivalent to {@code value} in the graph
      */
-    default <T extends ValueNode> T add(T value) {
+    default <T extends Node> T add(T value) {
         if (value.graph() != null) {
             assert !(value instanceof StateSplit) || ((StateSplit) value).stateAfter() != null;
             return value;
         }
         return setStateAfterIfNecessary(this, append(value));
     }
 
+    /**
+     * Maybe performs canonicalization on the provided node. Either the result of the
+     * canonicalization, or the original node if canonicalization is not possible, is added to the
+     * graph and returned. Note that the return value can be null when canonicalization determines
+     * that the node can be deleted.
+     */
+    default Node canonicalizeAndAdd(Node value) {
+        return add(value);
+    }
+
     default ValueNode addNonNullCast(ValueNode value) {
         AbstractPointerStamp valueStamp = (AbstractPointerStamp) value.stamp(NodeView.DEFAULT);
         if (valueStamp.nonNull()) {
@@ -536,7 +547,7 @@ default void replacePluginWithException(GeneratedInvocationPlugin plugin, Resolv
         throw GraalError.unimplementedParent(); // ExcludeFromJacocoGeneratedReport
     }
 
-    static <T extends ValueNode> T setStateAfterIfNecessary(GraphBuilderContext b, T value) {
+    static <T extends Node> T setStateAfterIfNecessary(GraphBuilderContext b, T value) {
         if (value instanceof StateSplit) {
             StateSplit stateSplit = (StateSplit) value;
             FrameState oldState = stateSplit.stateAfter();
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/graphbuilderconf/GraphBuilderTool.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/graphbuilderconf/GraphBuilderTool.java
@@ -25,8 +25,8 @@
 package org.graalvm.compiler.nodes.graphbuilderconf;
 
 import org.graalvm.compiler.debug.DebugContext;
+import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.nodes.StructuredGraph;
-import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.spi.CoreProviders;
 import org.graalvm.compiler.options.OptionValues;
 
@@ -43,7 +43,7 @@ public interface GraphBuilderTool extends CoreProviders {
      * @param value the node to be added to the graph
      * @return either the node added or an equivalent node
      */
-    <T extends ValueNode> T append(T value);
+    <T extends Node> T append(T value);
 
     default Assumptions getAssumptions() {
         return getGraph().getAssumptions();
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/GraphKit.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/GraphKit.java
@@ -41,6 +41,7 @@
 import org.graalvm.compiler.debug.DebugContext;
 import org.graalvm.compiler.debug.GraalError;
 import org.graalvm.compiler.graph.Graph;
+import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.Node.ValueNumberable;
 import org.graalvm.compiler.graph.NodeSourcePosition;
 import org.graalvm.compiler.java.FrameStateBuilder;
@@ -153,9 +154,9 @@ public <T extends ValueNode> T add(T node) {
         return graph.add(changeToWord(node));
     }
 
-    public <T extends ValueNode> T changeToWord(T node) {
-        if (wordTypes != null && wordTypes.isWord(node)) {
-            node.setStamp(wordTypes.getWordStamp(StampTool.typeOrNull(node)));
+    public <T extends Node> T changeToWord(T node) {
+        if (node instanceof ValueNode valueNode && wordTypes != null && wordTypes.isWord(valueNode)) {
+            valueNode.setStamp(wordTypes.getWordStamp(StampTool.typeOrNull(valueNode)));
         }
         return node;
     }
@@ -170,7 +171,7 @@ public final JavaKind asKind(JavaType type) {
     }
 
     @Override
-    public <T extends ValueNode> T append(T node) {
+    public <T extends Node> T append(T node) {
         if (node.graph() != null) {
             return node;
         }
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/IntrinsicGraphBuilder.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/IntrinsicGraphBuilder.java
@@ -34,6 +34,7 @@
 import org.graalvm.compiler.debug.DebugCloseable;
 import org.graalvm.compiler.debug.DebugContext;
 import org.graalvm.compiler.debug.GraalError;
+import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeSourcePosition;
 import org.graalvm.compiler.java.FrameStateBuilder;
 import org.graalvm.compiler.nodes.AbstractBeginNode;
@@ -163,7 +164,7 @@ protected IntrinsicGraphBuilder(OptionValues options,
         }
     }
 
-    private <T extends ValueNode> void updateLastInstruction(T v) {
+    private <T extends Node> void updateLastInstruction(T v) {
         if (v instanceof FixedNode) {
             FixedNode fixedNode = (FixedNode) v;
             if (lastInstr != null) {
@@ -222,7 +223,7 @@ protected void mergeUnwinds() {
     }
 
     @Override
-    public <T extends ValueNode> T append(T v) {
+    public <T extends Node> T append(T v) {
         if (v.graph() != null) {
             return v;
         }
diff --git a/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/PEGraphDecoder.java b/compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/replacements/PEGraphDecoder.java
@@ -94,6 +94,7 @@
 import org.graalvm.compiler.nodes.UnwindNode;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.WithExceptionNode;
+import org.graalvm.compiler.nodes.calc.FloatingNode;
 import org.graalvm.compiler.nodes.cfg.ControlFlowGraph;
 import org.graalvm.compiler.nodes.extended.AnchoringNode;
 import org.graalvm.compiler.nodes.extended.BytecodeExceptionNode;
@@ -421,7 +422,7 @@ public IntrinsicContext getIntrinsic() {
         }
 
         @Override
-        public <T extends ValueNode> T append(T value) {
+        public <T extends Node> T append(T value) {
             throw unimplementedOverride(); // ExcludeFromJacocoGeneratedReport
         }
 
@@ -550,7 +551,7 @@ public void setStateAfter(StateSplit stateSplit) {
 
         @SuppressWarnings("try")
         @Override
-        public <T extends ValueNode> T append(T v) {
+        public <T extends Node> T append(T v) {
             if (v.graph() != null) {
                 return v;
             }
@@ -563,6 +564,21 @@ public <T extends ValueNode> T append(T v) {
             }
         }
 
+        @Override
+        public Node canonicalizeAndAdd(Node node) {
+            Node canonicalized = node;
+            if (canonicalized instanceof FixedNode fixedNode) {
+                canonicalized = canonicalizeFixedNode(methodScope, null, fixedNode);
+            } else if (canonicalized instanceof FloatingNode floatingNode) {
+                canonicalized = handleFloatingNodeBeforeAdd(methodScope, null, floatingNode);
+            }
+
+            if (canonicalized == null) {
+                return null;
+            }
+            return super.canonicalizeAndAdd(canonicalized);
+        }
+
         private DebugCloseable withNodeSourcePosition() {
             if (getGraph().trackNodeSourcePosition()) {
                 NodeSourcePosition callerBytecodePosition = methodScope.getCallerNodeSourcePosition();
@@ -573,7 +589,7 @@ private DebugCloseable withNodeSourcePosition() {
             return null;
         }
 
-        private <T extends ValueNode> void updateLastInstruction(T v) {
+        private <T extends Node> void updateLastInstruction(T v) {
             if (v instanceof FixedNode) {
                 FixedNode fixedNode = (FixedNode) v;
                 if (lastInstr != null) {
@@ -721,7 +737,7 @@ public void setStateAfter(StateSplit sideEffect) {
 
         @SuppressWarnings("try")
         @Override
-        public <T extends ValueNode> T append(T v) {
+        public <T extends Node> T append(T v) {
             if (v.graph() != null) {
                 return v;
             }
@@ -744,7 +760,7 @@ private DebugCloseable withNodeSourcePosition() {
             return null;
         }
 
-        private <T extends ValueNode> void updateLastInstruction(T value) {
+        private <T extends Node> void updateLastInstruction(T value) {
             if (value instanceof FixedWithNextNode) {
                 FixedWithNextNode fixed = (FixedWithNextNode) value;
                 graph.addBeforeFixed(insertBefore, fixed);
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SubstrateClassInitializationPlugin.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/phases/SubstrateClassInitializationPlugin.java
@@ -78,6 +78,6 @@ public boolean apply(GraphBuilderContext builder, ResolvedJavaType type, Supplie
     private static void emitEnsureClassInitialized(GraphBuilderContext builder, JavaConstant hubConstant, FrameState frameState) {
         ValueNode hub = ConstantNode.forConstant(hubConstant, builder.getMetaAccess(), builder.getGraph());
         EnsureClassInitializedNode node = new EnsureClassInitializedNode(hub, frameState);
-        builder.add(node);
+        builder.canonicalizeAndAdd(node);
     }
 }
diff --git a/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/clinit/TestClassInitialization.java b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/clinit/TestClassInitialization.java
@@ -425,10 +425,22 @@ class ReflectionMustBeSafeEarly {
             m1 = c1Local.getDeclaredMethod("foo", int.class);
             f2 = c2Local.getDeclaredField("field");
 
+            /*
+             * Check that reflective class lookup and the elimination of the class initialization
+             * check also works when the class name is not constant yet during bytecode parsing.
+             */
+            if (c1Local != Class.forName(forNameMustBeSafeEarly(), true, ReflectionMustBeSafeEarly.class.getClassLoader())) {
+                throw new Error("wrong class");
+            }
+
         } catch (ReflectiveOperationException ex) {
             throw new Error(ex);
         }
     }
+
+    private static String forNameMustBeSafeEarly() {
+        return "com.oracle.svm.test.clinit.ForNameMustBeSafeEarly";
+    }
 }
 
 @SuppressWarnings("unused")

Original file line number	Diff line number	Diff line change
`@@ -41,6 +41,7 @@`
`41`	`41`	`import org.graalvm.compiler.debug.DebugContext;`
`42`	`42`	`import org.graalvm.compiler.debug.GraalError;`
`43`	`43`	`import org.graalvm.compiler.graph.Graph;`
	`44`	`+import org.graalvm.compiler.graph.Node;`
`44`	`45`	`import org.graalvm.compiler.graph.Node.ValueNumberable;`
`45`	`46`	`import org.graalvm.compiler.graph.NodeSourcePosition;`
`46`	`47`	`import org.graalvm.compiler.java.FrameStateBuilder;`
`@@ -153,9 +154,9 @@ public <T extends ValueNode> T add(T node) {`
`153`	`154`	`return graph.add(changeToWord(node));`
`154`	`155`	`}`
`155`	`156`
`156`		`- public <T extends ValueNode> T changeToWord(T node) {`
`157`		`- if (wordTypes != null && wordTypes.isWord(node)) {`
`158`		`- node.setStamp(wordTypes.getWordStamp(StampTool.typeOrNull(node)));`
	`157`	`+ public <T extends Node> T changeToWord(T node) {`
	`158`	`+ if (node instanceof ValueNode valueNode && wordTypes != null && wordTypes.isWord(valueNode)) {`
	`159`	`+ valueNode.setStamp(wordTypes.getWordStamp(StampTool.typeOrNull(valueNode)));`
`159`	`160`	`}`
`160`	`161`	`return node;`
`161`	`162`	`}`
`@@ -170,7 +171,7 @@ public final JavaKind asKind(JavaType type) {`
`170`	`171`	`}`
`171`	`172`
`172`	`173`	`@Override`
`173`		`- public <T extends ValueNode> T append(T node) {`
	`174`	`+ public <T extends Node> T append(T node) {`
`174`	`175`	`if (node.graph() != null) {`
`175`	`176`	`return node;`
`176`	`177`	`}`
Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,7 @@`
`34`	`34`	`import org.graalvm.compiler.debug.DebugCloseable;`
`35`	`35`	`import org.graalvm.compiler.debug.DebugContext;`
`36`	`36`	`import org.graalvm.compiler.debug.GraalError;`
	`37`	`+import org.graalvm.compiler.graph.Node;`
`37`	`38`	`import org.graalvm.compiler.graph.NodeSourcePosition;`
`38`	`39`	`import org.graalvm.compiler.java.FrameStateBuilder;`
`39`	`40`	`import org.graalvm.compiler.nodes.AbstractBeginNode;`
`@@ -163,7 +164,7 @@ protected IntrinsicGraphBuilder(OptionValues options,`
`163`	`164`	`}`
`164`	`165`	`}`
`165`	`166`
`166`		`- private <T extends ValueNode> void updateLastInstruction(T v) {`
	`167`	`+ private <T extends Node> void updateLastInstruction(T v) {`
`167`	`168`	`if (v instanceof FixedNode) {`
`168`	`169`	`FixedNode fixedNode = (FixedNode) v;`
`169`	`170`	`if (lastInstr != null) {`
`@@ -222,7 +223,7 @@ protected void mergeUnwinds() {`
`222`	`223`	`}`
`223`	`224`
`224`	`225`	`@Override`
`225`		`- public <T extends ValueNode> T append(T v) {`
	`226`	`+ public <T extends Node> T append(T v) {`
`226`	`227`	`if (v.graph() != null) {`
`227`	`228`	`return v;`
`228`	`229`	`}`
Original file line number	Diff line number	Diff line change
`@@ -78,6 +78,6 @@ public boolean apply(GraphBuilderContext builder, ResolvedJavaType type, Supplie`
`78`	`78`	`private static void emitEnsureClassInitialized(GraphBuilderContext builder, JavaConstant hubConstant, FrameState frameState) {`
`79`	`79`	`ValueNode hub = ConstantNode.forConstant(hubConstant, builder.getMetaAccess(), builder.getGraph());`
`80`	`80`	`EnsureClassInitializedNode node = new EnsureClassInitializedNode(hub, frameState);`
`81`		`- builder.add(node);`
	`81`	`+ builder.canonicalizeAndAdd(node);`
`82`	`82`	`}`
`83`	`83`	`}`