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-47449] Verify return types of substituted methods #7340

Merged
merged 7 commits into from
Sep 6, 2023
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 java-benchmarks/mx.java-benchmarks/mx_java_benchmarks.py
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,7 @@ def extra_image_build_argument(self, benchmark, args):
'-H:+AllowFoldMethods',
'-H:-UseServiceLoaderFeature',
'-H:+AllowDeprecatedBuilderClassesOnImageClasspath', # needs to be removed once GR-41746 is fixed
'-H:+DisableSubstitutionReturnTypeCheck', # remove once Quarkus fixed their substitutions (GR-48152)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zakkak I had to add this for our Quarkus jobs because in the (quite old) version we are using there were substitutions with return type mismatches. The offending substitution seems to be gone on master, but I have not checked all the others. Maybe things are fine, maybe not. Just a heads up. :)

Copy link
Collaborator

@zakkak zakkak Sep 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zapster, thanks for the heads up.

It looks like there are some failures on Quarkus main as well which I am working on in quarkusio/quarkus#35847 and quarkusio/quarkus#35956

]) + super(BaseQuarkusBenchmarkSuite, self).extra_image_build_argument(benchmark, args)


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -752,12 +752,12 @@ public int getClassAccessFlags() {
}

@Substitute
private Object getComponentType() {
private DynamicHub getComponentType() {
return componentType;
}

@Substitute
private Object getSuperclass() {
private DynamicHub getSuperclass() {
return superHub;
}

Expand Down Expand Up @@ -791,18 +791,18 @@ private boolean isEnum() {
* We do not do the check "this.getModifiers() & ENUM) != 0" because we do not have the full
* modifier bits.
*/
return this.getSuperclass() == java.lang.Enum.class;
return toClass(getSuperclass()) == java.lang.Enum.class;
}

@KeepOriginal
private native Enum<?>[] getEnumConstants();
private native Object[] getEnumConstants();

@Substitute
public Enum<?>[] getEnumConstantsShared() {
public Object[] getEnumConstantsShared() {
if (enumConstantsReference instanceof LazyFinalReference) {
return (Enum<?>[]) ((LazyFinalReference<?>) enumConstantsReference).get();
return (Object[]) ((LazyFinalReference<?>) enumConstantsReference).get();
}
return (Enum<?>[]) enumConstantsReference;
return (Object[]) enumConstantsReference;
}

@KeepOriginal
Expand Down Expand Up @@ -962,7 +962,7 @@ public <T extends Annotation> T[] getAnnotationsByType(Class<T> annotationClass)
T[] result = getDeclaredAnnotationsByType(annotationClass);

if (result.length == 0 && AnnotationAccess.isAnnotationPresent(annotationClass, Inherited.class)) {
DynamicHub superClass = (DynamicHub) this.getSuperclass();
DynamicHub superClass = this.getSuperclass();
if (superClass != null) {
/* Determine if the annotation is associated with the superclass. */
result = superClass.getAnnotationsByType(annotationClass);
Expand Down Expand Up @@ -1378,7 +1378,7 @@ String computePackageName() {
String pn = null;
DynamicHub me = this;
while (me.hubIsArray()) {
me = (DynamicHub) me.getComponentType();
me = me.getComponentType();
}
if (me.isPrimitive()) {
pn = "java.lang";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,15 @@ final class Target_jdk_internal_jimage_ImageReaderFactory_JRTEnabled {
@TargetClass(className = "jdk.internal.module.SystemModuleFinders", innerClass = "SystemImage", onlyWith = JRTDisabled.class)
final class Target_jdk_internal_module_SystemModuleFinders_SystemImage_JRTDisabled {
@Substitute
static Object reader() {
static Target_jdk_internal_jimage_ImageReader reader() {
throw VMError.unsupportedFeature("JRT file system is disabled");
}
}

@TargetClass(className = "jdk.internal.jimage.ImageReader", onlyWith = JRTDisabled.class)
final class Target_jdk_internal_jimage_ImageReader {
}

@TargetClass(className = "sun.net.www.protocol.jrt.Handler", onlyWith = JRTDisabled.class)
final class Target_sun_net_www_protocol_jrt_Handler_JRTDisabled {
@Substitute
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ final class Target_java_lang_Object {

@Substitute
@TargetElement(name = "getClass")
private Object getClassSubst() {
private DynamicHub getClassSubst() {
return readHub(this);
}

Expand Down Expand Up @@ -139,11 +139,12 @@ private static Enum<?> valueOf(Class<Enum<?>> enumType, String name) {
* The original implementation creates and caches a HashMap to make the lookup faster. For
* simplicity, we do a linear search for now.
*/
Enum<?>[] enumConstants = DynamicHub.fromClass(enumType).getEnumConstantsShared();
Object[] enumConstants = DynamicHub.fromClass(enumType).getEnumConstantsShared();
if (enumConstants == null) {
throw new IllegalArgumentException(enumType.getName() + " is not an enum type");
}
for (Enum<?> e : enumConstants) {
for (Object o : enumConstants) {
Enum<?> e = (Enum<?>) o;
if (e.name().equals(name)) {
return e;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,11 +169,15 @@ protected Class<?>[] getClassContext() {
@SuppressWarnings({"static-method", "unused"})
final class Target_javax_crypto_JceSecurityManager {
@Substitute
Object getCryptoPermission(String var1) {
return Target_javax_crypto_CryptoAllPermission.INSTANCE;
Target_javax_crypto_CryptoPermission getCryptoPermission(String var1) {
return SubstrateUtil.cast(Target_javax_crypto_CryptoAllPermission.INSTANCE, Target_javax_crypto_CryptoPermission.class);
}
}

@TargetClass(className = "javax.crypto.CryptoPermission")
final class Target_javax_crypto_CryptoPermission {
}

@TargetClass(className = "javax.crypto.CryptoAllPermission")
final class Target_javax_crypto_CryptoAllPermission {
@Alias //
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ public static ClassLoader getSystemClassLoader() {
}

@Delete
private static native void initSystemClassLoader();
private static native ClassLoader initSystemClassLoader();

@Alias
public native Enumeration<URL> findResources(String name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
package com.oracle.svm.core.jdk;

import java.io.FileDescriptor;
import java.nio.MappedByteBuffer;

import com.oracle.svm.core.SubstrateUtil;
import com.oracle.svm.core.annotate.Alias;
import com.oracle.svm.core.annotate.Substitute;
import com.oracle.svm.core.annotate.TargetClass;
Expand Down Expand Up @@ -66,13 +68,13 @@ protected Target_java_nio_DirectByteBufferR_JDK17(int cap, long addr, FileDescri
final class Target_sun_nio_ch_Util_JDK17 {

@Substitute
private static Target_java_nio_DirectByteBuffer_JDK17 newMappedByteBuffer(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return new Target_java_nio_DirectByteBuffer_JDK17(size, addr, fd, unmapper, isSync, null);
private static MappedByteBuffer newMappedByteBuffer(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return SubstrateUtil.cast(new Target_java_nio_DirectByteBuffer_JDK17(size, addr, fd, unmapper, isSync, null), MappedByteBuffer.class);
}

@Substitute
static Target_java_nio_DirectByteBufferR_JDK17 newMappedByteBufferR(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return new Target_java_nio_DirectByteBufferR_JDK17(size, addr, fd, unmapper, isSync, null);
static MappedByteBuffer newMappedByteBufferR(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return SubstrateUtil.cast(new Target_java_nio_DirectByteBufferR_JDK17(size, addr, fd, unmapper, isSync, null), MappedByteBuffer.class);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
package com.oracle.svm.core.jdk;

import java.io.FileDescriptor;
import java.nio.MappedByteBuffer;

import com.oracle.svm.core.SubstrateUtil;
import com.oracle.svm.core.annotate.Alias;
import com.oracle.svm.core.annotate.Substitute;
import com.oracle.svm.core.annotate.TargetClass;
Expand Down Expand Up @@ -61,13 +63,13 @@ protected Target_java_nio_DirectByteBufferR_JDK19OrLater(int cap, long addr, Fil
final class Target_sun_nio_ch_Util_JDK19OrLater {

@Substitute
private static Target_java_nio_DirectByteBuffer_JDK19OrLater newMappedByteBuffer(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return new Target_java_nio_DirectByteBuffer_JDK19OrLater(size, addr, fd, unmapper, isSync, null);
private static MappedByteBuffer newMappedByteBuffer(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return SubstrateUtil.cast(new Target_java_nio_DirectByteBuffer_JDK19OrLater(size, addr, fd, unmapper, isSync, null), MappedByteBuffer.class);
}

@Substitute
static Target_java_nio_DirectByteBufferR_JDK19OrLater newMappedByteBufferR(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return new Target_java_nio_DirectByteBufferR_JDK19OrLater(size, addr, fd, unmapper, isSync, null);
static MappedByteBuffer newMappedByteBufferR(int size, long addr, FileDescriptor fd, Runnable unmapper, boolean isSync) {
return SubstrateUtil.cast(new Target_java_nio_DirectByteBufferR_JDK19OrLater(size, addr, fd, unmapper, isSync, null), MappedByteBuffer.class);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ final class Target_java_util_Locale {
private static Locale defaultFormatLocale;

@Substitute
private static Object initDefault() {
private static Locale initDefault() {
throw VMError.shouldNotReachHere("The default Locale must be initialized during image generation");
}

@Substitute
private static Object initDefault(Locale.Category category) {
private static Locale initDefault(Locale.Category category) {
throw VMError.shouldNotReachHere("The default Locale must be initialized during image generation: " + category);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ public static long getTypeId(Class<?> clazz) {

/** See {@link JVM#getEventWriter}. */
@Substitute
public static Object getEventWriter() {
public static Target_jdk_jfr_internal_event_EventWriter getEventWriter() {
return SubstrateJVM.get().getEventWriter();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ final class BoundMethodHandleUtils {
/* Bound method handle constructor */
static Target_java_lang_invoke_BoundMethodHandle make(MethodType type, Target_java_lang_invoke_LambdaForm form, String species, Object... args) {
Target_java_lang_invoke_SimpleMethodHandle bmh = new Target_java_lang_invoke_SimpleMethodHandle(type, form);
bmh.speciesData = SubstrateUtil.cast(Target_java_lang_invoke_BoundMethodHandle.SPECIALIZER, Target_java_lang_invoke_ClassSpecializer.class).findSpecies(species);
var specializer = SubstrateUtil.cast(Target_java_lang_invoke_BoundMethodHandle.SPECIALIZER, Target_java_lang_invoke_ClassSpecializer.class);
bmh.speciesData = SubstrateUtil.cast(specializer.findSpecies(species), Target_java_lang_invoke_BoundMethodHandle_SpeciesData.class);
bmh.args = (args != null) ? Arrays.copyOf(args, args.length) : new Object[0];
return SubstrateUtil.cast(bmh, Target_java_lang_invoke_BoundMethodHandle.class);
}
Expand All @@ -168,6 +169,6 @@ static Object[] appendArgs(Object[] args, Object newArg) {
}

static String speciesKey(Target_java_lang_invoke_SimpleMethodHandle bmh) {
return SubstrateUtil.cast(bmh.speciesData(), Target_java_lang_invoke_ClassSpecializer_SpeciesData.class).key();
return (String) SubstrateUtil.cast(bmh.speciesData(), Target_java_lang_invoke_ClassSpecializer_SpeciesData.class).key();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@
@TargetClass(className = "java.lang.invoke.ClassSpecializer")
final class Target_java_lang_invoke_ClassSpecializer {
@Alias
native Target_java_lang_invoke_BoundMethodHandle_SpeciesData findSpecies(Object ll);
native Target_java_lang_invoke_ClassSpecializer_SpeciesData findSpecies(Object ll);
}

@TargetClass(className = "java.lang.invoke.ClassSpecializer", innerClass = "SpeciesData")
final class Target_java_lang_invoke_ClassSpecializer_SpeciesData {
@Alias
native String key();
native Object key();

@Alias
protected native String deriveClassName();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ private static Target_java_lang_VirtualThread cast(Thread thread) {

@Override
public ThreadFactory createFactory() {
return Target_java_lang_Thread.ofVirtual().factory();
return SubstrateUtil.cast(Target_java_lang_Thread.ofVirtual(), Target_java_lang_Thread_Builder.class).factory();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -668,11 +668,11 @@ private static void clearInterruptEvent() {

@Alias
@TargetElement(onlyWith = LoomJDK.class)
public static native Target_java_lang_Thread_Builder ofVirtual();
public static native Target_java_lang_Thread_Builder_OfVirtual ofVirtual();

@Substitute
@TargetElement(name = "ofVirtual", onlyWith = {JDK19OrLater.class, NotLoomJDK.class})
public static Target_java_lang_Thread_Builder ofVirtualWithoutLoom() {
public static Target_java_lang_Thread_Builder_OfVirtual ofVirtualWithoutLoom() {
if (Target_jdk_internal_misc_PreviewFeatures.isEnabled()) {
if (DeoptimizationSupport.enabled()) {
throw new UnsupportedOperationException("Virtual threads are not supported together with Truffle JIT compilation.");
Expand Down Expand Up @@ -810,6 +810,10 @@ interface Target_java_lang_Thread_Builder {
ThreadFactory factory();
}

@TargetClass(value = Thread.class, innerClass = {"Builder", "OfVirtual"}, onlyWith = JDK19OrLater.class)
interface Target_java_lang_Thread_Builder_OfVirtual {
}

@TargetClass(value = Thread.class, innerClass = "Constants", onlyWith = JDK19OrLater.class)
final class Target_java_lang_Thread_Constants {
// Checkstyle: stop
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean o
@Option(help = "Deprecated", type = User)//
static final HostedOptionKey<Boolean> AllowIncompleteClasspath = new HostedOptionKey<>(false);

@Option(help = "Disable substitution return type checking", type = Debug, deprecated = true, stability = OptionStability.EXPERIMENTAL, deprecationMessage = "This option will be removed soon and the return type check will be mandatory.")//
public static final HostedOptionKey<Boolean> DisableSubstitutionReturnTypeCheck = new HostedOptionKey<>(false);

@SuppressWarnings("all")
private static boolean areAssertionsEnabled() {
boolean assertsEnabled = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,9 @@ private ResolvedJavaMethod findOriginalMethod(Executable annotatedMethod, Class<
Method originalMethod = originalClass.getDeclaredMethod(originalName, originalParams);

guarantee(Modifier.isStatic(annotatedMethod.getModifiers()) == Modifier.isStatic(originalMethod.getModifiers()), "Static modifier mismatch: %s, %s", annotatedMethod, originalMethod);

guarantee(NativeImageOptions.DisableSubstitutionReturnTypeCheck.getValue() || getTargetClass(((Method) annotatedMethod).getReturnType()).equals(originalMethod.getReturnType()),
"Return type mismatch:%n %s%n %s", annotatedMethod, originalMethod);
return metaAccess.lookupJavaMethod(originalMethod);

} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@
*/
package com.oracle.svm.truffle.nfi;

import com.oracle.svm.core.c.CGlobalData;
import com.oracle.svm.core.c.CGlobalDataFactory;
import com.oracle.svm.core.c.function.CEntryPointErrors;
import org.graalvm.nativeimage.ImageSingletons;
import org.graalvm.nativeimage.c.function.CEntryPoint;
import org.graalvm.nativeimage.c.function.CEntryPoint.Publish;
Expand All @@ -36,7 +33,10 @@
import org.graalvm.word.WordFactory;

import com.oracle.svm.core.Uninterruptible;
import com.oracle.svm.core.c.CGlobalData;
import com.oracle.svm.core.c.CGlobalDataFactory;
import com.oracle.svm.core.c.function.CEntryPointActions;
import com.oracle.svm.core.c.function.CEntryPointErrors;
import com.oracle.svm.core.c.function.CEntryPointOptions;
import com.oracle.svm.core.c.function.CEntryPointOptions.ReturnNullPointer;
import com.oracle.svm.core.c.function.CEntryPointSetup.LeaveDetachThreadEpilogue;
Expand All @@ -53,7 +53,6 @@
import com.oracle.svm.truffle.nfi.NativeAPI.ReleaseAndReturnFunction;
import com.oracle.svm.truffle.nfi.NativeAPI.ReleaseClosureRefFunction;
import com.oracle.svm.truffle.nfi.NativeAPI.ReleaseObjectRefFunction;
import com.oracle.truffle.api.interop.TruffleObject;

/**
* Implementation of the TruffleEnv and TruffleContext native API functions.
Expand Down Expand Up @@ -138,7 +137,7 @@ static void releaseClosureRef(NativeTruffleEnv env, PointerBase closure) {
static TruffleObjectHandle getClosureObject(NativeTruffleEnv env, PointerBase closure) {
TruffleNFISupport support = ImageSingletons.lookup(TruffleNFISupport.class);
Target_com_oracle_truffle_nfi_backend_libffi_LibFFIContext context = lookupContext(env.context());
TruffleObject ret = context.getClosureObject(closure.rawValue());
Object ret = context.getClosureObject(closure.rawValue());
return support.createGlobalHandle(ret);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import com.oracle.svm.truffle.nfi.libffi.LibFFI.ffi_cif;
import com.oracle.truffle.api.CallTarget;
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
import com.oracle.truffle.api.interop.TruffleObject;

@TargetClass(className = "com.oracle.truffle.nfi.backend.libffi.LibFFIContext", onlyWith = TruffleNFIFeature.IsEnabled.class)
final class Target_com_oracle_truffle_nfi_backend_libffi_LibFFIContext {
Expand Down Expand Up @@ -80,7 +79,7 @@ native Target_com_oracle_truffle_nfi_backend_libffi_ClosureNativePointer createC
native void releaseClosureRef(long codePointer);

@Alias
native TruffleObject getClosureObject(long codePointer);
native Object getClosureObject(long codePointer);

@Alias
protected native void initializeSimpleType(Target_com_oracle_truffle_nfi_backend_spi_types_NativeSimpleType simpleType, int size, int alignment, long ffiType);
Expand Down Expand Up @@ -212,9 +211,7 @@ Object lookupSymbol(Target_com_oracle_truffle_nfi_backend_libffi_LibFFILibrary l
if (ImageSingletons.lookup(TruffleNFISupport.class).errnoGetterFunctionName.equals(name)) {
return new ErrnoMirror();
} else {
Target_com_oracle_truffle_nfi_backend_libffi_LibFFISymbol ret = Target_com_oracle_truffle_nfi_backend_libffi_LibFFISymbol.create(library, name,
lookup(nativeContext, library.handle, name));
return ret;
return Target_com_oracle_truffle_nfi_backend_libffi_LibFFISymbol.create(library, name, lookup(nativeContext, library.handle, name));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,5 @@
final class Target_com_oracle_truffle_nfi_backend_libffi_LibFFISymbol {

@Alias
static native Target_com_oracle_truffle_nfi_backend_libffi_LibFFISymbol create(Target_com_oracle_truffle_nfi_backend_libffi_LibFFILibrary library, String name, long address);
static native Object create(Target_com_oracle_truffle_nfi_backend_libffi_LibFFILibrary library, String name, long address);
}
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
import com.oracle.svm.core.NeverInline;
import com.oracle.svm.core.ParsingReason;
import com.oracle.svm.core.RuntimeAssertionsSupport;
import com.oracle.svm.core.SubstrateUtil;
import com.oracle.svm.core.annotate.Alias;
import com.oracle.svm.core.annotate.AnnotateOriginal;
import com.oracle.svm.core.annotate.Delete;
Expand Down Expand Up @@ -1140,7 +1141,7 @@ final class Target_com_oracle_truffle_api_staticobject_PodBasedShapeGenerator<T>

@Substitute
@SuppressWarnings("unchecked")
Target_com_oracle_truffle_api_staticobject_PodBasedStaticShape<T> generateShape(Target_com_oracle_truffle_api_staticobject_PodBasedStaticShape<T> parentShape,
StaticShape<T> generateShape(Target_com_oracle_truffle_api_staticobject_PodBasedStaticShape<T> parentShape,
Map<String, Target_com_oracle_truffle_api_staticobject_StaticProperty> staticProperties, boolean safetyChecks) {
Pod.Builder<T> builder;
if (parentShape == null) {
Expand All @@ -1162,7 +1163,7 @@ Target_com_oracle_truffle_api_staticobject_PodBasedStaticShape<T> generateShape(
for (var entry : propertyFields) {
entry.getLeft().initOffset(entry.getRight().getOffset());
}
return Target_com_oracle_truffle_api_staticobject_PodBasedStaticShape.create(storageSuperClass, pod.getFactory(), safetyChecks, pod);
return SubstrateUtil.cast(Target_com_oracle_truffle_api_staticobject_PodBasedStaticShape.create(storageSuperClass, pod.getFactory(), safetyChecks, pod), StaticShape.class);
}
}

Expand Down