Register reflectively-accessed types as unsafe allocated

loicottet · loicottet · commit 4ba5326dd336 · 2024-11-29T16:53:09.000+01:00
diff --git a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java
@@ -46,10 +46,10 @@
 
 public interface ReflectionRegistry {
     default void register(ConfigurationCondition condition, Class<?>... classes) {
-        Arrays.stream(classes).forEach(clazz -> register(condition, false, clazz));
+        Arrays.stream(classes).forEach(clazz -> register(condition, clazz));
     }
 
-    void register(ConfigurationCondition condition, boolean unsafeAllocated, Class<?> clazz);
+    void register(ConfigurationCondition condition, Class<?> clazz);
 
     void register(ConfigurationCondition condition, boolean queriedOnly, Executable... methods);
 
diff --git a/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/config/ConfigurationType.java b/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/config/ConfigurationType.java
@@ -100,7 +100,6 @@ static ConfigurationType copyAndMerge(ConfigurationType type, ConfigurationType
     private boolean allPublicClasses;
     private ConfigurationMemberAccessibility allDeclaredFieldsAccess = ConfigurationMemberAccessibility.NONE;
     private ConfigurationMemberAccessibility allPublicFieldsAccess = ConfigurationMemberAccessibility.NONE;
-    private boolean unsafeAllocated;
     private ConfigurationMemberAccessibility allDeclaredMethodsAccess = ConfigurationMemberAccessibility.NONE;
     private ConfigurationMemberAccessibility allPublicMethodsAccess = ConfigurationMemberAccessibility.NONE;
     private ConfigurationMemberAccessibility allDeclaredConstructorsAccess = ConfigurationMemberAccessibility.NONE;
@@ -281,7 +280,6 @@ private void setFlagsFromOther(ConfigurationType other, BiPredicate<Boolean, Boo
         allPublicClasses = flagPredicate.test(allPublicClasses, other.allPublicClasses);
         allDeclaredFieldsAccess = accessCombiner.apply(allDeclaredFieldsAccess, other.allDeclaredFieldsAccess);
         allPublicFieldsAccess = accessCombiner.apply(allPublicFieldsAccess, other.allPublicFieldsAccess);
-        unsafeAllocated = flagPredicate.test(unsafeAllocated, other.unsafeAllocated);
         allDeclaredMethodsAccess = accessCombiner.apply(allDeclaredMethodsAccess, other.allDeclaredMethodsAccess);
         allPublicMethodsAccess = accessCombiner.apply(allPublicMethodsAccess, other.allPublicMethodsAccess);
         allDeclaredConstructorsAccess = accessCombiner.apply(allDeclaredConstructorsAccess, other.allDeclaredConstructorsAccess);
@@ -411,10 +409,6 @@ public synchronized void setAllPublicClasses() {
         allPublicClasses = true;
     }
 
-    public void setUnsafeAllocated() {
-        this.unsafeAllocated = true;
-    }
-
     public synchronized void setAllDeclaredFields(ConfigurationMemberAccessibility accessibility) {
         if (!allDeclaredFieldsAccess.includes(accessibility)) {
             allDeclaredFieldsAccess = accessibility;
@@ -470,7 +464,6 @@ public synchronized void printJson(JsonWriter writer) throws IOException {
         printJsonBooleanIfSet(writer, allPublicMethodsAccess == ConfigurationMemberAccessibility.ACCESSED, "allPublicMethods");
         printJsonBooleanIfSet(writer, allDeclaredConstructorsAccess == ConfigurationMemberAccessibility.ACCESSED, "allDeclaredConstructors");
         printJsonBooleanIfSet(writer, allPublicConstructorsAccess == ConfigurationMemberAccessibility.ACCESSED, "allPublicConstructors");
-        printJsonBooleanIfSet(writer, unsafeAllocated, "unsafeAllocated");
 
         if (fields != null) {
             writer.appendSeparator().quote("fields").appendFieldSeparator();
diff --git a/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/config/ParserConfigurationAdapter.java b/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/config/ParserConfigurationAdapter.java
@@ -81,12 +81,6 @@ public boolean registerAllConstructors(UnresolvedConfigurationCondition conditio
         return true;
     }
 
-    @Override
-    public void registerUnsafeAllocated(UnresolvedConfigurationCondition condition, ConfigurationType type) {
-        VMError.guarantee(condition.equals(type.getCondition()), "condition is here part of the type");
-        type.setUnsafeAllocated();
-    }
-
     @Override
     public void registerMethod(UnresolvedConfigurationCondition condition, boolean queriedOnly, ConfigurationType type, String methodName, List<ConfigurationType> methodParameterTypes) {
         VMError.guarantee(condition.equals(type.getCondition()), "condition is already a part of the type");
diff --git a/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/trace/JniProcessor.java b/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/trace/JniProcessor.java
@@ -89,10 +89,10 @@ void processEntry(EconomicMap<String, ?> entry, ConfigurationSet configurationSe
             case "AllocObject":
                 expectSize(args, 0);
                 /*
-                 * AllocObject is implemented via Unsafe.allocateInstance, so we need to set the
-                 * "unsafe allocated" flag in the reflection configuration file.
+                 * AllocObject is implemented via Unsafe.allocateInstance, so we need to add the
+                 * type to the reflection configuration file.
                  */
-                configurationSet.getReflectionConfiguration().getOrCreateType(condition, clazz).setUnsafeAllocated();
+                configurationSet.getReflectionConfiguration().getOrCreateType(condition, clazz);
                 break;
             case "GetStaticMethodID":
             case "GetMethodID": {
diff --git a/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/trace/ReflectionProcessor.java b/substratevm/src/com.oracle.svm.configure/src/com/oracle/svm/configure/trace/ReflectionProcessor.java
@@ -283,7 +283,8 @@ public void processEntry(EconomicMap<String, ?> entry, ConfigurationSet configur
                 break;
             }
             case "allocateInstance": {
-                configuration.getOrCreateType(condition, clazz).setUnsafeAllocated();
+                /* Reflectively-accessed types are unsafe-allocatable by default */
+                configuration.getOrCreateType(condition, clazz);
                 break;
             }
             default:
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/LegacyReflectionConfigurationParser.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/LegacyReflectionConfigurationParser.java
@@ -29,6 +29,7 @@
 import java.util.List;
 import java.util.Optional;
 
+import com.oracle.svm.util.LogUtils;
 import org.graalvm.collections.EconomicMap;
 import org.graalvm.collections.MapCursor;
 import org.graalvm.nativeimage.impl.UnresolvedConfigurationCondition;
@@ -56,6 +57,8 @@ public void parseAndRegister(Object json, URI origin) {
         parseClassArray(asList(json, "first level of document must be an array of class descriptors"));
     }
 
+    private boolean unsafeAllocatedWarningTriggered = false;
+
     @Override
     protected void parseClass(EconomicMap<String, Object> data) {
         checkAttributes(data, "reflection class descriptor object", List.of(NAME_KEY), OPTIONAL_REFLECT_CONFIG_OBJECT_ATTRS);
@@ -93,6 +96,11 @@ protected void parseClass(EconomicMap<String, Object> data) {
         T clazz = result.get();
         delegate.registerType(conditionResult.get(), clazz);
 
+        if (!unsafeAllocatedWarningTriggered && data.containsKey("unsafeAllocated")) {
+            unsafeAllocatedWarningTriggered = true;
+            LogUtils.warning("\"unsafeAllocated\" is deprecated in reflection-config.json. All reflectively-accessed classes can be instantiated through unsafe without the use of the flag.");
+        }
+
         registerIfNotDefault(data, false, clazz, "allDeclaredConstructors", () -> delegate.registerDeclaredConstructors(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allPublicConstructors", () -> delegate.registerPublicConstructors(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allDeclaredMethods", () -> delegate.registerDeclaredMethods(condition, false, clazz));
@@ -117,7 +125,6 @@ protected void parseClass(EconomicMap<String, Object> data) {
             delegate.registerDeclaredFields(queryCondition, true, clazz);
             delegate.registerPublicFields(queryCondition, true, clazz);
         }
-        registerIfNotDefault(data, false, clazz, "unsafeAllocated", () -> delegate.registerUnsafeAllocated(condition, clazz));
         MapCursor<String, Object> cursor = data.getEntries();
         while (cursor.advance()) {
             String name = cursor.getKey();
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionConfigurationParserDelegate.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionConfigurationParserDelegate.java
@@ -68,8 +68,6 @@ public interface ReflectionConfigurationParserDelegate<C, T> {
 
     boolean registerAllConstructors(C condition, boolean queriedOnly, T type);
 
-    void registerUnsafeAllocated(C condition, T clazz);
-
     String getTypeName(T type);
 
     String getSimpleName(T type);
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionMetadataParser.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionMetadataParser.java
@@ -34,6 +34,7 @@
 import org.graalvm.nativeimage.impl.UnresolvedConfigurationCondition;
 
 import com.oracle.svm.core.TypeResult;
+import com.oracle.svm.util.LogUtils;
 
 class ReflectionMetadataParser<C, T> extends ReflectionConfigurationParser<C, T> {
     private static final List<String> OPTIONAL_REFLECT_METADATA_ATTRS = Arrays.asList(CONDITIONAL_KEY,
@@ -56,6 +57,8 @@ public void parseAndRegister(Object json, URI origin) {
         }
     }
 
+    private boolean unsafeAllocatedWarningTriggered = false;
+
     @Override
     protected void parseClass(EconomicMap<String, Object> data) {
         checkAttributes(data, "reflection class descriptor object", List.of(TYPE_KEY), OPTIONAL_REFLECT_METADATA_ATTRS);
@@ -84,7 +87,7 @@ protected void parseClass(EconomicMap<String, Object> data) {
 
         C queryCondition = conditionResolver.alwaysTrue();
         T clazz = result.get();
-        delegate.registerType(conditionResult.get(), clazz);
+        delegate.registerType(condition, clazz);
 
         delegate.registerDeclaredClasses(queryCondition, clazz);
         delegate.registerRecordComponents(queryCondition, clazz);
@@ -98,14 +101,17 @@ protected void parseClass(EconomicMap<String, Object> data) {
         delegate.registerPublicMethods(queryCondition, true, clazz);
         delegate.registerDeclaredFields(queryCondition, true, clazz);
         delegate.registerPublicFields(queryCondition, true, clazz);
+        if (!unsafeAllocatedWarningTriggered && data.containsKey("unsafeAllocated")) {
+            unsafeAllocatedWarningTriggered = true;
+            LogUtils.warning("\"unsafeAllocated\" is deprecated in reachability-metadata.json. All reflectively-accessed classes can be instantiated through unsafe without the use of the flag.");
+        }
 
         registerIfNotDefault(data, false, clazz, "allDeclaredConstructors", () -> delegate.registerDeclaredConstructors(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allPublicConstructors", () -> delegate.registerPublicConstructors(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allDeclaredMethods", () -> delegate.registerDeclaredMethods(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allPublicMethods", () -> delegate.registerPublicMethods(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allDeclaredFields", () -> delegate.registerDeclaredFields(condition, false, clazz));
         registerIfNotDefault(data, false, clazz, "allPublicFields", () -> delegate.registerPublicFields(condition, false, clazz));
-        registerIfNotDefault(data, false, clazz, "unsafeAllocated", () -> delegate.registerUnsafeAllocated(condition, clazz));
 
         MapCursor<String, Object> cursor = data.getEntries();
         while (cursor.advance()) {
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/config/RegistryAdapter.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/config/RegistryAdapter.java
@@ -28,7 +28,6 @@
 
 import java.lang.reflect.Executable;
 import java.lang.reflect.Method;
-import java.lang.reflect.Modifier;
 import java.util.ArrayList;
 import java.util.List;
 
@@ -223,17 +222,6 @@ public boolean registerAllConstructors(ConfigurationCondition condition, boolean
         return methods.length > 0;
     }
 
-    @Override
-    public void registerUnsafeAllocated(ConfigurationCondition condition, Class<?> clazz) {
-        if (!clazz.isArray() && !clazz.isInterface() && !Modifier.isAbstract(clazz.getModifiers())) {
-            registry.register(condition, true, clazz);
-            /*
-             * Ignore otherwise as the implementation of allocateInstance will anyhow throw an
-             * exception.
-             */
-        }
-    }
-
     @Override
     public void registerMethod(ConfigurationCondition condition, boolean queriedOnly, Class<?> type, String methodName, List<Class<?>> methodParameterTypes) throws NoSuchMethodException {
         try {
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jni/JNIAccessFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jni/JNIAccessFeature.java
@@ -219,8 +219,7 @@ private class JNIRuntimeAccessibilitySupportImpl extends ConditionalConfiguratio
                     implements RuntimeJNIAccessSupport {
 
         @Override
-        public void register(ConfigurationCondition condition, boolean unsafeAllocated, Class<?> clazz) {
-            assert !unsafeAllocated : "unsafeAllocated can be only set via Unsafe.allocateInstance, not via JNI.";
+        public void register(ConfigurationCondition condition, Class<?> clazz) {
             Objects.requireNonNull(clazz, () -> nullErrorMessage("class"));
             abortIfSealed();
             registerConditionalConfiguration(condition, (cnd) -> newClasses.add(clazz));
@@ -252,7 +251,7 @@ private void registerFields(boolean finalIsWritable, Field[] fields) {
         @Override
         public void registerClassLookup(ConfigurationCondition condition, String typeName) {
             try {
-                register(condition, false, Class.forName(typeName));
+                register(condition, Class.forName(typeName));
             } catch (ClassNotFoundException e) {
                 newNegativeClassLookups.add(typeName);
             }
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java
@@ -197,9 +197,9 @@ private boolean isQueryFlagSet(Class<?> clazz, int flag) {
     }
 
     @Override
-    public void register(ConfigurationCondition condition, boolean unsafeInstantiated, Class<?> clazz) {
+    public void register(ConfigurationCondition condition, Class<?> clazz) {
         Objects.requireNonNull(clazz, () -> nullErrorMessage("class"));
-        runConditionalInAnalysisTask(condition, (cnd) -> registerClass(cnd, clazz, unsafeInstantiated, true));
+        runConditionalInAnalysisTask(condition, (cnd) -> registerClass(cnd, clazz, true));
     }
 
     @Override
@@ -213,7 +213,7 @@ public void registerAllClassesQuery(ConfigurationCondition condition, Class<?> c
                         /* Malformed inner classes can have no declaring class */
                         innerClasses.computeIfAbsent(innerClass.getDeclaringClass(), c -> ConcurrentHashMap.newKeySet()).add(innerClass);
                     }
-                    registerClass(cnd, innerClass, false, !MissingRegistrationUtils.throwMissingRegistrationErrors());
+                    registerClass(cnd, innerClass, !MissingRegistrationUtils.throwMissingRegistrationErrors());
                 }
             } catch (LinkageError e) {
                 registerLinkageError(clazz, e, classLookupExceptions);
@@ -239,22 +239,22 @@ public void registerAllDeclaredClassesQuery(ConfigurationCondition condition, Cl
             try {
                 for (Class<?> innerClass : clazz.getDeclaredClasses()) {
                     innerClasses.computeIfAbsent(clazz, c -> ConcurrentHashMap.newKeySet()).add(innerClass);
-                    registerClass(cnd, innerClass, false, !MissingRegistrationUtils.throwMissingRegistrationErrors());
+                    registerClass(cnd, innerClass, !MissingRegistrationUtils.throwMissingRegistrationErrors());
                 }
             } catch (LinkageError e) {
                 registerLinkageError(clazz, e, classLookupExceptions);
             }
         });
     }
 
-    private void registerClass(ConfigurationCondition condition, Class<?> clazz, boolean unsafeInstantiated, boolean allowForName) {
+    private void registerClass(ConfigurationCondition condition, Class<?> clazz, boolean allowForName) {
         if (shouldExcludeClass(clazz)) {
             return;
         }
 
         AnalysisType type = metaAccess.lookupJavaType(clazz);
         type.registerAsReachable("Is registered for reflection.");
-        if (unsafeInstantiated) {
+        if (type.isArray() || (type.isInstanceClass() && !type.isAbstract())) {
             type.registerAsUnsafeAllocated("Is registered via reflection metadata.");
             classForNameSupport.registerUnsafeAllocated(condition, clazz);
         }
@@ -291,7 +291,7 @@ public void registerClassLookupException(ConfigurationCondition condition, Strin
     public void registerClassLookup(ConfigurationCondition condition, String typeName) {
         runConditionalInAnalysisTask(condition, (cnd) -> {
             try {
-                registerClass(cnd, Class.forName(typeName, false, ClassLoader.getSystemClassLoader()), false, true);
+                registerClass(cnd, Class.forName(typeName, false, ClassLoader.getSystemClassLoader()), true);
             } catch (ClassNotFoundException e) {
                 classForNameSupport.registerNegativeQuery(cnd, typeName);
             } catch (Throwable t) {
@@ -316,7 +316,7 @@ public void registerAllPermittedSubclassesQuery(ConfigurationCondition condition
             setQueryFlag(clazz, ALL_PERMITTED_SUBCLASSES_FLAG);
             if (clazz.isSealed()) {
                 for (Class<?> permittedSubclass : clazz.getPermittedSubclasses()) {
-                    registerClass(condition, permittedSubclass, false, false);
+                    registerClass(condition, permittedSubclass, false);
                 }
             }
         });
@@ -329,7 +329,7 @@ public void registerAllNestMembersQuery(ConfigurationCondition condition, Class<
             setQueryFlag(clazz, ALL_NEST_MEMBERS_FLAG);
             for (Class<?> nestMember : clazz.getNestMembers()) {
                 if (nestMember != clazz) {
-                    registerClass(condition, nestMember, false, false);
+                    registerClass(condition, nestMember, false);
                 }
             }
         });

Original file line number	Diff line number	Diff line change
`@@ -46,10 +46,10 @@`
`46`	`46`
`47`	`47`	`public interface ReflectionRegistry {`
`48`	`48`	`default void register(ConfigurationCondition condition, Class<?>... classes) {`
`49`		`- Arrays.stream(classes).forEach(clazz -> register(condition, false, clazz));`
	`49`	`+ Arrays.stream(classes).forEach(clazz -> register(condition, clazz));`
`50`	`50`	`}`
`51`	`51`
`52`		`- void register(ConfigurationCondition condition, boolean unsafeAllocated, Class<?> clazz);`
	`52`	`+ void register(ConfigurationCondition condition, Class<?> clazz);`
`53`	`53`
`54`	`54`	`void register(ConfigurationCondition condition, boolean queriedOnly, Executable... methods);`
`55`	`55`
Original file line number	Diff line number	Diff line change
`@@ -283,7 +283,8 @@ public void processEntry(EconomicMap<String, ?> entry, ConfigurationSet configur`
`283`	`283`	`break;`
`284`	`284`	`}`
`285`	`285`	`case "allocateInstance": {`
`286`		`- configuration.getOrCreateType(condition, clazz).setUnsafeAllocated();`
	`286`	`+ /* Reflectively-accessed types are unsafe-allocatable by default */`
	`287`	`+ configuration.getOrCreateType(condition, clazz);`
`287`	`288`	`break;`
`288`	`289`	`}`
`289`	`290`	`default:`