[GR-41674] Class instanceOf and isAssignableFrom checks do need to make the checked type reachable.

PullRequest: graal/12922

Author: Christian Wimmer

substratevm/CHANGELOG.md

@@ -6,6 +6,7 @@ This changelog summarizes major changes to GraalVM Native Image.
 * (GR-40187) Report invalid use of SVM specific classes on image class- or module-path as error. As a temporary workaround, `-H:+AllowDeprecatedBuilderClassesOnImageClasspath` allows turning the error into a warning.
 * (GR-41196) Provide `.debug.svm.imagebuild.*` sections that contain build options and properties used in the build of the image.
 * (GR-41978) Disallow `--initialize-at-build-time` without arguments. As a temporary workaround, `-H:+AllowDeprecatedInitializeAllClassesAtBuildTime` allows turning this error into a warning.
+* (GR-41674) Class instanceOf and isAssignableFrom checks do need to make the checked type reachable.
 
 ## Version 22.3.0
 * (GR-35721) Remove old build output style and the `-H:±BuildOutputUseNewStyle` option.

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java

@@ -70,11 +70,14 @@
 import org.graalvm.compiler.nodes.calc.IsNullNode;
 import org.graalvm.compiler.nodes.calc.ObjectEqualsNode;
 import org.graalvm.compiler.nodes.extended.BoxNode;
+import org.graalvm.compiler.nodes.extended.BytecodeExceptionNode;
+import org.graalvm.compiler.nodes.extended.BytecodeExceptionNode.BytecodeExceptionKind;
 import org.graalvm.compiler.nodes.extended.ForeignCall;
 import org.graalvm.compiler.nodes.extended.GetClassNode;
 import org.graalvm.compiler.nodes.extended.RawLoadNode;
 import org.graalvm.compiler.nodes.extended.RawStoreNode;
 import org.graalvm.compiler.nodes.java.AtomicReadAndWriteNode;
+import org.graalvm.compiler.nodes.java.ClassIsAssignableFromNode;
 import org.graalvm.compiler.nodes.java.DynamicNewArrayNode;
 import org.graalvm.compiler.nodes.java.DynamicNewInstanceNode;
 import org.graalvm.compiler.nodes.java.ExceptionObjectNode;
@@ -134,6 +137,7 @@
 import com.oracle.graal.pointsto.nodes.UnsafePartitionStoreNode;
 import com.oracle.graal.pointsto.phases.InlineBeforeAnalysis;
 import com.oracle.graal.pointsto.results.StaticAnalysisResultsBuilder;
+import com.oracle.graal.pointsto.results.StrengthenGraphs;
 import com.oracle.graal.pointsto.typestate.TypeState;
 import com.oracle.graal.pointsto.util.AnalysisError;
 
@@ -226,7 +230,9 @@ public void registerUsedElements(boolean registerEmbeddedRoots) {
             if (n instanceof InstanceOfNode) {
                 InstanceOfNode node = (InstanceOfNode) n;
                 AnalysisType type = (AnalysisType) node.type().getType();
-                type.registerAsReachable();
+                if (!ignoreInstanceOfType(type)) {
+                    type.registerAsReachable();
+                }
 
             } else if (n instanceof NewInstanceNode) {
                 NewInstanceNode node = (NewInstanceNode) n;
@@ -290,7 +296,7 @@ public void registerUsedElements(boolean registerEmbeddedRoots) {
                     assert StampTool.isExactType(cn);
                     AnalysisType type = (AnalysisType) StampTool.typeOrNull(cn);
                     type.registerAsInHeap();
-                    if (registerEmbeddedRoots) {
+                    if (registerEmbeddedRoots && !ignoreConstant(cn)) {
                         registerEmbeddedRoot(cn);
                     }
                 }
@@ -321,6 +327,59 @@ public void registerUsedElements(boolean registerEmbeddedRoots) {
         }
     }
 
+    /**
+     * This method filters constants, i.e., these constants are not seen as reachable on its own.
+     * This avoids making things reachable just because of that constant usage. For all cases where
+     * this method returns true, {@link StrengthenGraphs} must have a corresponding re-write of the
+     * constant in case nothing else in the application made that constant reachable.
+     *
+     * {@link Class#isAssignableFrom} is often used with a constant receiver class. In that case, we
+     * do not want to make the receiver class reachable, because as long as the receiver class is
+     * not reachable for any other "real" reason we know that isAssignableFrom will always return
+     * false. So in {@link StrengthenGraphs} we can then constant-fold the
+     * {@link ClassIsAssignableFromNode} to false.
+     *
+     * Similarly, a class should not be marked as reachable only so that we can add the class name
+     * to the error message of a {@link ClassCastException}. In {@link StrengthenGraphs} we can
+     * re-write the Class constant to a String constant, i.e., only embed the class name and not the
+     * full java.lang.Class object in the image.
+     */
+    protected boolean ignoreConstant(ConstantNode cn) {
+        if (!ignoreInstanceOfType((AnalysisType) bb.getProviders().getConstantReflection().asJavaType(cn.asConstant()))) {
+            return false;
+        }
+        for (var usage : cn.usages()) {
+            if (usage instanceof ClassIsAssignableFromNode) {
+                if (((ClassIsAssignableFromNode) usage).getThisClass() != cn) {
+                    return false;
+                }
+            } else if (usage instanceof BytecodeExceptionNode) {
+                if (((BytecodeExceptionNode) usage).getExceptionKind() != BytecodeExceptionKind.CLASS_CAST) {
+                    return false;
+                }
+            } else {
+                return false;
+            }
+        }
+        /* Success, the ConstantNode do not need to be seen as reachable. */
+        return true;
+    }
+
+    protected boolean ignoreInstanceOfType(AnalysisType type) {
+        if (type == null || !bb.strengthenGraalGraphs()) {
+            return false;
+        }
+        if (type.isArray()) {
+            /*
+             * There is no real overhead when making array types reachable (which automatically also
+             * makes them instantiated), and it avoids manual reflection configuration because
+             * casting to an array type then automatically marks the array type as instantiated.
+             */
+            return false;
+        }
+        return true;
+    }
+
     private void registerEmbeddedRoot(ConstantNode cn) {
         JavaConstant root = cn.asJavaConstant();
         if (bb.scanningPolicy().trackConstant(bb, root)) {

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java

@@ -42,6 +42,7 @@
 import org.graalvm.compiler.nodeinfo.InputType;
 import org.graalvm.compiler.nodes.AbstractBeginNode;
 import org.graalvm.compiler.nodes.CallTargetNode;
+import org.graalvm.compiler.nodes.ConstantNode;
 import org.graalvm.compiler.nodes.FixedGuardNode;
 import org.graalvm.compiler.nodes.FixedNode;
 import org.graalvm.compiler.nodes.FixedWithNextNode;
@@ -60,7 +61,9 @@
 import org.graalvm.compiler.nodes.StateSplit;
 import org.graalvm.compiler.nodes.StructuredGraph;
 import org.graalvm.compiler.nodes.ValueNode;
+import org.graalvm.compiler.nodes.extended.BytecodeExceptionNode;
 import org.graalvm.compiler.nodes.extended.ValueAnchorNode;
+import org.graalvm.compiler.nodes.java.ClassIsAssignableFromNode;
 import org.graalvm.compiler.nodes.java.InstanceOfNode;
 import org.graalvm.compiler.nodes.java.LoadFieldNode;
 import org.graalvm.compiler.nodes.java.LoadIndexedNode;
@@ -187,6 +190,8 @@ public JavaTypeProfile makeTypeProfile(AnalysisField field) {
 
     protected abstract void setInvokeProfiles(Invoke invoke, JavaTypeProfile typeProfile, JavaMethodProfile methodProfile);
 
+    protected abstract String getTypeName(AnalysisType type);
+
     class StrengthenSimplifier implements CustomSimplification {
 
         private final StructuredGraph graph;
@@ -307,6 +312,35 @@ public void simplify(Node n, SimplifierTool tool) {
                     tool.addToWorkList(replacement);
                 }
 
+            } else if (n instanceof ClassIsAssignableFromNode) {
+                ClassIsAssignableFromNode node = (ClassIsAssignableFromNode) n;
+                ValueNode thisClass = node.getThisClass();
+                if (thisClass.isConstant()) {
+                    AnalysisType thisType = (AnalysisType) tool.getConstantReflection().asJavaType(thisClass.asConstant());
+                    if (!thisType.isReachable()) {
+                        node.replaceAndDelete(LogicConstantNode.contradiction(graph));
+                    }
+                }
+
+            } else if (n instanceof BytecodeExceptionNode) {
+                /*
+                 * We do not want a type to be reachable only to be used for the error message of a
+                 * ClassCastException. Therefore, in that case we replace the java.lang.Class with a
+                 * java.lang.String that is then used directly in the error message.
+                 */
+                BytecodeExceptionNode node = (BytecodeExceptionNode) n;
+                if (node.getExceptionKind() == BytecodeExceptionNode.BytecodeExceptionKind.CLASS_CAST) {
+                    ValueNode expectedClass = node.getArguments().get(1);
+                    if (expectedClass.isConstant()) {
+                        AnalysisType expectedType = (AnalysisType) tool.getConstantReflection().asJavaType(expectedClass.asConstant());
+                        if (expectedType != null && !expectedType.isReachable()) {
+                            String expectedName = getTypeName(expectedType);
+                            ConstantNode expectedConstant = ConstantNode.forConstant(tool.getConstantReflection().forString(expectedName), tool.getMetaAccess(), graph);
+                            node.getArguments().set(1, expectedConstant);
+                        }
+                    }
+                }
+
             } else if (n instanceof PiNode) {
                 PiNode node = (PiNode) n;
                 Stamp oldStamp = node.piStamp();
@@ -630,6 +664,16 @@ private Stamp strengthenStamp(Stamp s) {
                 return null;
             }
 
+            if (!originalType.isReachable()) {
+                /* We must be in dead code. */
+                if (stamp.nonNull()) {
+                    /* We must be in dead code. */
+                    return StampFactory.empty(JavaKind.Object);
+                } else {
+                    return StampFactory.alwaysNull();
+                }
+            }
+
             AnalysisType singleImplementorType = getSingleImplementorType(originalType);
             if (singleImplementorType != null && (!stamp.isExactType() || !singleImplementorType.equals(originalType))) {
                 ResolvedJavaType targetType = toTarget(singleImplementorType);

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/snippets/ImplicitExceptions.java

@@ -27,8 +27,8 @@
 import java.lang.reflect.GenericSignatureFormatError;
 
 import com.oracle.svm.core.SubstrateDiagnostics;
-import com.oracle.svm.core.heap.RestrictHeapAccess;
 import com.oracle.svm.core.code.FactoryMethodMarker;
+import com.oracle.svm.core.heap.RestrictHeapAccess;
 import com.oracle.svm.core.jdk.InternalVMMethod;
 import com.oracle.svm.core.jdk.StackTraceUtils;
 import com.oracle.svm.core.snippets.SnippetRuntime.SubstrateForeignCallDescriptor;
@@ -173,10 +173,16 @@ private static ArrayIndexOutOfBoundsException createOutOfBoundsException(int ind
 
     /** Foreign call: {@link #CREATE_CLASS_CAST_EXCEPTION}. */
     @SubstrateForeignCallTarget(stubCallingConvention = true)
-    private static ClassCastException createClassCastException(Object object, Class<?> expectedClass) {
+    private static ClassCastException createClassCastException(Object object, Object expectedClass) {
         assert object != null : "null can be cast to any type, so it cannot show up as a source of a ClassCastException";
         vmErrorIfImplicitExceptionsAreFatal();
-        return new ClassCastException(object.getClass().getTypeName() + " cannot be cast to " + expectedClass.getTypeName());
+        String expectedClassName;
+        if (expectedClass instanceof Class) {
+            expectedClassName = ((Class<?>) expectedClass).getTypeName();
+        } else {
+            expectedClassName = String.valueOf(expectedClass);
+        }
+        return new ClassCastException(object.getClass().getTypeName() + " cannot be cast to " + expectedClassName);
     }
 
     /** Foreign call: {@link #CREATE_ARRAY_STORE_EXCEPTION}. */
@@ -256,10 +262,16 @@ private static void throwNewClassCastException() {
 
     /** Foreign call: {@link #THROW_NEW_CLASS_CAST_EXCEPTION_WITH_ARGS}. */
     @SubstrateForeignCallTarget(stubCallingConvention = true)
-    private static void throwNewClassCastExceptionWithArgs(Object object, Class<?> expectedClass) {
+    private static void throwNewClassCastExceptionWithArgs(Object object, Object expectedClass) {
         assert object != null : "null can be cast to any type, so it cannot show up as a source of a ClassCastException";
         vmErrorIfImplicitExceptionsAreFatal();
-        throw new ClassCastException(object.getClass().getTypeName() + " cannot be cast to " + expectedClass.getTypeName());
+        String expectedClassName;
+        if (expectedClass instanceof Class) {
+            expectedClassName = ((Class<?>) expectedClass).getTypeName();
+        } else {
+            expectedClassName = String.valueOf(expectedClass);
+        }
+        throw new ClassCastException(object.getClass().getTypeName() + " cannot be cast to " + expectedClassName);
     }
 
     /** Foreign call: {@link #THROW_NEW_ARRAY_STORE_EXCEPTION}. */

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SubstrateStrengthenGraphs.java

@@ -42,6 +42,7 @@
 import com.oracle.svm.core.graal.nodes.LoweredDeadEndNode;
 import com.oracle.svm.core.nodes.SubstrateMethodCallTargetNode;
 import com.oracle.svm.core.snippets.SnippetRuntime;
+import com.oracle.svm.core.util.HostedStringDeduplication;
 import com.oracle.svm.hosted.meta.HostedType;
 
 import jdk.vm.ci.meta.JavaMethodProfile;
@@ -94,4 +95,9 @@ protected void setInvokeProfiles(Invoke invoke, JavaTypeProfile typeProfile, Jav
     protected void setInvokeProfiles(Invoke invoke, JavaTypeProfile typeProfile, JavaMethodProfile methodProfile, JavaTypeProfile staticTypeProfile) {
         ((SubstrateMethodCallTargetNode) invoke.callTarget()).setProfiles(typeProfile, methodProfile, staticTypeProfile);
     }
+
+    @Override
+    protected String getTypeName(AnalysisType type) {
+        return HostedStringDeduplication.singleton().deduplicate(type.toJavaName(true), false);
+    }
 }

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ameta/AnalysisConstantReflectionProvider.java

@@ -305,9 +305,7 @@ public ResolvedJavaType asJavaType(Constant constant) {
 
     @Override
     public JavaConstant asJavaClass(ResolvedJavaType type) {
-        DynamicHub dynamicHub = getHostVM().dynamicHub(type);
-        registerAsReachable(getHostVM(), dynamicHub);
-        return SubstrateObjectConstant.forObject(dynamicHub);
+        return SubstrateObjectConstant.forObject(getHostVM().dynamicHub(type));
     }
 
     protected static void registerAsReachable(SVMHost hostVM, DynamicHub dynamicHub) {

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/flow/SVMMethodTypeFlowBuilder.java

@@ -51,6 +51,7 @@
 import com.oracle.svm.core.util.UserError.UserException;
 import com.oracle.svm.hosted.NativeImageOptions;
 import com.oracle.svm.hosted.SVMHost;
+import com.oracle.svm.hosted.classinitialization.ClassInitializationOptions;
 import com.oracle.svm.hosted.substitute.ComputedValueField;
 
 import jdk.vm.ci.code.BytecodePosition;
@@ -82,7 +83,7 @@ public void registerUsedElements(boolean registerEmbeddedRoots) {
                 if (cn.hasUsages() && cn.isJavaConstant() && constant.getJavaKind() == JavaKind.Object && constant.isNonNull()) {
                     if (constant instanceof ImageHeapConstant) {
                         /* No replacement for ImageHeapObject. */
-                    } else {
+                    } else if (!ignoreConstant(cn)) {
                         /*
                          * Constants that are embedded into graphs via constant folding of static
                          * fields have already been replaced. But constants embedded manually by
@@ -107,6 +108,20 @@ public void registerUsedElements(boolean registerEmbeddedRoots) {
         }
     }
 
+    @Override
+    protected boolean ignoreInstanceOfType(AnalysisType type) {
+        if (ClassInitializationOptions.AllowDeprecatedInitializeAllClassesAtBuildTime.getValue()) {
+            /*
+             * Compatibility mode for Helidon MP: It initializes all classes at build time, and
+             * relies on the side effect of a class initializer of a class that is only used in an
+             * instanceof. See https://github.com/oracle/graal/pull/5224#issuecomment-1279586997 for
+             * details.
+             */
+            return false;
+        }
+        return super.ignoreInstanceOfType(type);
+    }
+
     @SuppressWarnings("serial")
     public static class UnsafeOffsetError extends UserException {