Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions substratevm/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -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)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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}. */
Expand Down Expand Up @@ -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}. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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 {

Expand Down