[GR-45009] Fix missing reflection registration tests

PullRequest: graal/14135
oracle · Mar 29, 2023 · da53c0d · da53c0d
2 parents 57a4514 + 2d0da81
commit da53c0d
Show file tree

Hide file tree

Showing 8 changed files with 108 additions and 102 deletions.
diff --git a/...le.graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/StandaloneHost.java b/...le.graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/StandaloneHost.java
@@ -26,20 +26,22 @@
 
 package com.oracle.graal.pointsto.standalone;
 
-import com.oracle.graal.pointsto.api.HostVM;
-import com.oracle.graal.pointsto.meta.AnalysisType;
-import com.oracle.graal.pointsto.meta.HostedProviders;
-import com.oracle.graal.pointsto.standalone.plugins.StandaloneGraphBuilderPhase;
-import com.oracle.graal.pointsto.util.AnalysisError;
-import jdk.vm.ci.meta.ResolvedJavaType;
+import java.util.Comparator;
+import java.util.concurrent.ConcurrentHashMap;
+
 import org.graalvm.compiler.java.GraphBuilderPhase;
 import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
 import org.graalvm.compiler.nodes.graphbuilderconf.IntrinsicContext;
 import org.graalvm.compiler.options.OptionValues;
 import org.graalvm.compiler.phases.OptimisticOptimizations;
 
-import java.util.Comparator;
-import java.util.concurrent.ConcurrentHashMap;
+import com.oracle.graal.pointsto.api.HostVM;
+import com.oracle.graal.pointsto.meta.AnalysisType;
+import com.oracle.graal.pointsto.meta.HostedProviders;
+import com.oracle.graal.pointsto.standalone.plugins.StandaloneGraphBuilderPhase;
+import com.oracle.graal.pointsto.util.AnalysisError;
+
+import jdk.vm.ci.meta.ResolvedJavaType;
 
 public class StandaloneHost extends HostVM {
     private final ConcurrentHashMap<AnalysisType, Class<?>> typeToClass = new ConcurrentHashMap<>();
@@ -80,10 +82,6 @@ public void onTypeReachable(AnalysisType type) {
          */
     }
 
-    @Override
-    public void onTypeInstantiated(AnalysisType newValue) {
-    }
-
     @Override
     public GraphBuilderPhase.Instance createGraphBuilderPhase(HostedProviders builderProviders, GraphBuilderConfiguration graphBuilderConfig, OptimisticOptimizations optimisticOpts,
                     IntrinsicContext initialIntrinsicContext) {

diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/api/HostVM.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/api/HostVM.java
@@ -154,13 +154,6 @@ public void checkType(ResolvedJavaType type, AnalysisUniverse universe) {
      */
     public abstract void onTypeReachable(AnalysisType newValue);
 
-    /**
-     * Run initialization tasks for a newly instantiated {@link AnalysisType}.
-     *
-     * @param newValue the type to initialize
-     */
-    public abstract void onTypeInstantiated(AnalysisType newValue);
-
     /**
      * Check if an {@link AnalysisType} is initialized.
      */

diff --git a/...vm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisUniverse.java b/...vm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisUniverse.java
@@ -692,7 +692,6 @@ public void onFieldAccessed(AnalysisField field) {
     }
 
     public void onTypeInstantiated(AnalysisType type, UsageKind usage) {
-        hostVM.onTypeInstantiated(type);
         bb.onTypeInstantiated(type, usage);
     }
 

diff --git a/.../com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionConfigurationParser.java b/.../com.oracle.svm.core/src/com/oracle/svm/core/configure/ReflectionConfigurationParser.java
@@ -84,7 +84,11 @@ private void parseClass(EconomicMap<String, Object> data) {
         }
         ConfigurationCondition condition = conditionResult.get();
 
-        TypeResult<T> result = delegate.resolveType(condition, className, false);
+        /*
+         * Even if primitives cannot be queried through Class.forName, they can be registered to
+         * allow getDeclaredMethods() and similar bulk queries at run time.
+         */
+        TypeResult<T> result = delegate.resolveType(condition, className, true);
         if (!result.isPresent()) {
             handleError("Could not resolve class " + className + " for reflection configuration.", result.getException());
             return;

diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/ClassForNameSupport.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/ClassForNameSupport.java
@@ -55,8 +55,14 @@ public static void registerClass(Class<?> clazz) {
             return; // must be defined at runtime before it can be looked up
         }
         String name = clazz.getName();
-        VMError.guarantee(!singleton().knownClasses.containsKey(name) || singleton().knownClasses.get(name) == clazz);
-        singleton().knownClasses.put(name, clazz);
+        if (!singleton().knownClasses.containsKey(name) || !(singleton().knownClasses.get(name) instanceof Throwable)) {
+            /*
+             * If the class has already been seen as throwing an error, we don't overwrite this
+             * error
+             */
+            VMError.guarantee(!singleton().knownClasses.containsKey(name) || singleton().knownClasses.get(name) == clazz);
+            singleton().knownClasses.put(name, clazz);
+        }
     }
 
     @Platforms(Platform.HOSTED_ONLY.class)

diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java
@@ -992,41 +992,28 @@ public Field getField(String fieldName) throws NoSuchFieldException, SecurityExc
 
     private void checkField(String fieldName, Field field, boolean publicOnly) throws NoSuchFieldException {
         boolean throwMissingErrors = throwMissingRegistrationErrors();
-        boolean noSuchField = false;
-        boolean missingRegistration = false;
+        Class<?> clazz = DynamicHub.toClass(this);
         if (field == null) {
-            if (throwMissingErrors) {
-                if (isClassFlagSet(ALL_DECLARED_FIELDS_FLAG) || (publicOnly && isClassFlagSet(ALL_FIELDS_FLAG))) {
-                    /*
-                     * If getDeclaredFields (or getFields for a public field) is registered, we know
-                     * for sure that the field does indeed not exist if we don't find it.
-                     */
-                    noSuchField = true;
-                } else {
-                    missingRegistration = true;
-                }
+            if (throwMissingErrors && !allElementsRegistered(publicOnly, ALL_DECLARED_FIELDS_FLAG, ALL_FIELDS_FLAG)) {
+                throw MissingReflectionRegistrationUtils.forField(clazz, fieldName);
             } else {
-                noSuchField = true;
+                /*
+                 * If getDeclaredFields (or getFields for a public field) is registered, we know for
+                 * sure that the field does indeed not exist if we don't find it.
+                 */
+                throw new NoSuchFieldException(fieldName);
             }
         } else {
             ReflectionMetadataDecoder decoder = ImageSingletons.lookup(ReflectionMetadataDecoder.class);
             int fieldModifiers = field.getModifiers();
-            if (decoder.isNegative(fieldModifiers)) {
-                noSuchField = true;
-            } else if (decoder.isHiding(fieldModifiers)) {
-                if (throwMissingErrors) {
-                    missingRegistration = true;
-                } else {
-                    noSuchField = true;
-                }
+            boolean negative = decoder.isNegative(fieldModifiers);
+            boolean hiding = decoder.isHiding(fieldModifiers);
+            if (throwMissingErrors && hiding) {
+                throw MissingReflectionRegistrationUtils.forField(clazz, fieldName);
+            } else if (negative || hiding) {
+                throw new NoSuchFieldException(fieldName);
             }
         }
-        VMError.guarantee(!missingRegistration || !noSuchField, "Either a MissingRegistrationError or a NoSuchFieldException should be thrown, not both");
-        if (missingRegistration) {
-            throw MissingReflectionRegistrationUtils.forField(DynamicHub.toClass(this), fieldName);
-        } else if (noSuchField) {
-            throw new NoSuchFieldException(fieldName);
-        }
     }
 
     @Substitute
@@ -1039,41 +1026,32 @@ private Method getMethod(String methodName, Class<?>... parameterTypes) throws N
 
     private void checkMethod(String methodName, Class<?>[] parameterTypes, Executable method, boolean publicOnly) throws NoSuchMethodException {
         boolean throwMissingErrors = throwMissingRegistrationErrors();
-        boolean noSuchMethod = false;
-        boolean missingRegistration = false;
+        Class<?> clazz = DynamicHub.toClass(this);
         if (method == null) {
-            if (throwMissingErrors) {
-                if (isClassFlagSet(ALL_DECLARED_METHODS_FLAG) || (publicOnly && isClassFlagSet(ALL_METHODS_FLAG))) {
-                    /*
-                     * If getDeclaredMethods (or getMethods for a public method) is registered, we
-                     * know for sure that the method does indeed not exist if we don't find it.
-                     */
-                    noSuchMethod = true;
-                } else {
-                    missingRegistration = true;
-                }
+            if (throwMissingErrors && !allElementsRegistered(publicOnly, ALL_DECLARED_METHODS_FLAG, ALL_METHODS_FLAG)) {
+                throw MissingReflectionRegistrationUtils.forMethod(clazz, methodName, parameterTypes);
             } else {
-                noSuchMethod = true;
+                /*
+                 * If getDeclaredMethods (or getMethods for a public method) is registered, we know
+                 * for sure that the method does indeed not exist if we don't find it.
+                 */
+                throw new NoSuchMethodException(methodToString(methodName, parameterTypes));
             }
         } else {
             ReflectionMetadataDecoder decoder = ImageSingletons.lookup(ReflectionMetadataDecoder.class);
             int methodModifiers = method.getModifiers();
-            if (decoder.isNegative(methodModifiers)) {
-                noSuchMethod = true;
-            } else if (decoder.isHiding(methodModifiers)) {
-                if (throwMissingErrors) {
-                    missingRegistration = true;
-                } else {
-                    noSuchMethod = true;
-                }
+            boolean negative = decoder.isNegative(methodModifiers);
+            boolean hiding = decoder.isHiding(methodModifiers);
+            if (throwMissingErrors && hiding) {
+                throw MissingReflectionRegistrationUtils.forMethod(clazz, methodName, parameterTypes);
+            } else if (negative || hiding) {
+                throw new NoSuchMethodException(methodToString(methodName, parameterTypes));
             }
         }
-        VMError.guarantee(!missingRegistration || !noSuchMethod, "Either a MissingRegistrationError or a NoSuchMethodException should be thrown, not both");
-        if (missingRegistration) {
-            throw MissingReflectionRegistrationUtils.forMethod(DynamicHub.toClass(this), methodName, parameterTypes);
-        } else if (noSuchMethod) {
-            throw new NoSuchMethodException(methodToString(methodName, parameterTypes));
-        }
+    }
+
+    private boolean allElementsRegistered(boolean publicOnly, int allDeclaredElementsFlag, int allPublicElementsFlag) {
+        return isClassFlagSet(allDeclaredElementsFlag) || (publicOnly && isClassFlagSet(allPublicElementsFlag));
     }
 
     @KeepOriginal

diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SVMHost.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/SVMHost.java
@@ -70,8 +70,6 @@
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
 import org.graalvm.nativeimage.c.function.RelocatedPointer;
-import org.graalvm.nativeimage.impl.ConfigurationCondition;
-import org.graalvm.nativeimage.impl.RuntimeReflectionSupport;
 
 import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.PointsToAnalysis;
@@ -150,7 +148,6 @@ public class SVMHost extends HostVM {
     private final LinkAtBuildTimeSupport linkAtBuildTimeSupport;
     private final HostedStringDeduplication stringTable;
     private final UnsafeAutomaticSubstitutionProcessor automaticSubstitutions;
-    private final RuntimeReflectionSupport reflectionSupport;
 
     /**
      * Optionally keep the Graal graphs alive during analysis. This increases the memory footprint
@@ -188,7 +185,6 @@ public SVMHost(OptionValues options, ClassLoader classLoader, ClassInitializatio
             multiMethodAnalysisPolicy = DEFAULT_MULTIMETHOD_ANALYSIS_POLICY;
         }
         parsingSupport = ImageSingletons.contains(SVMParsingSupport.class) ? ImageSingletons.lookup(SVMParsingSupport.class) : null;
-        this.reflectionSupport = ImageSingletons.lookup(RuntimeReflectionSupport.class);
     }
 
     private static Map<String, EnumSet<AnalysisType.UsageKind>> setupForbiddenTypes(OptionValues options) {
@@ -316,14 +312,6 @@ public void onTypeReachable(AnalysisType analysisType) {
         automaticSubstitutions.computeSubstitutions(this, GraalAccess.getOriginalProviders().getMetaAccess().lookupJavaType(analysisType.getJavaClass()));
     }
 
-    @Override
-    public void onTypeInstantiated(AnalysisType newValue) {
-        if (newValue.isAnnotation()) {
-            /* getDeclaredMethods is called in the AnnotationType constructor */
-            reflectionSupport.registerAllDeclaredMethodsQuery(ConfigurationCondition.alwaysTrue(), true, newValue.getJavaClass());
-        }
-    }
-
     @Override
     public boolean isInitialized(AnalysisType type) {
         boolean shouldInitializeAtRuntime = classInitializationSupport.shouldInitializeAtRuntime(type);
@@ -647,7 +635,7 @@ public void methodBeforeTypeFlowCreationHook(BigBang bb, AnalysisMethod method,
             if (n instanceof StackValueNode) {
                 containsStackValueNode.put(method, true);
             } else if (n instanceof ReachabilityRegistrationNode node) {
-               bb.postTask(debug -> node.getRegistrationTask().ensureDone());
+                bb.postTask(debug -> node.getRegistrationTask().ensureDone());
             }
             checkClassInitializerSideEffect(method, n);
         }