From 0f0a1c695330c713ec1281d3395fefb7a95cb0db Mon Sep 17 00:00:00 2001 From: Christian Wimmer Date: Wed, 12 Oct 2022 16:00:11 -0700 Subject: [PATCH] Class instanceOf and isAssignableFrom checks do need to make the checked type reachable --- substratevm/CHANGELOG.md | 1 + .../pointsto/flow/MethodTypeFlowBuilder.java | 63 ++++++++++++++++++- .../pointsto/results/StrengthenGraphs.java | 44 +++++++++++++ .../svm/core/snippets/ImplicitExceptions.java | 22 +++++-- .../svm/hosted/SubstrateStrengthenGraphs.java | 6 ++ .../AnalysisConstantReflectionProvider.java | 4 +- .../flow/SVMMethodTypeFlowBuilder.java | 17 ++++- 7 files changed, 146 insertions(+), 11 deletions(-) diff --git a/substratevm/CHANGELOG.md b/substratevm/CHANGELOG.md index 29d2c50545a7..7f15e4b07958 100644 --- a/substratevm/CHANGELOG.md +++ b/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. diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java index d9f1647704aa..25bdd2c1930a 100644 --- a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java +++ b/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)) { diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java index 87a389b2f493..c0beb04d0875 100644 --- a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java +++ b/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); diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/snippets/ImplicitExceptions.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/snippets/ImplicitExceptions.java index 51c5aaa1334b..64bef77ad594 100644 --- a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/snippets/ImplicitExceptions.java +++ b/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}. */ diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SubstrateStrengthenGraphs.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SubstrateStrengthenGraphs.java index 43884b6ead50..9d3d97442846 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SubstrateStrengthenGraphs.java +++ b/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); + } } diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ameta/AnalysisConstantReflectionProvider.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ameta/AnalysisConstantReflectionProvider.java index 7c7e255ca5a7..d5c0738c8fbc 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ameta/AnalysisConstantReflectionProvider.java +++ b/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) { diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/flow/SVMMethodTypeFlowBuilder.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/flow/SVMMethodTypeFlowBuilder.java index 091b0bbc30f8..7d75176d791c 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/analysis/flow/SVMMethodTypeFlowBuilder.java +++ b/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 {