[GR-68593] Include unresolved types in the method descriptor

PullRequest: graal/21924

Author: Zeavee

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/SVMImageLayerLoader.java

@@ -51,6 +51,7 @@
 import java.util.function.Supplier;
 import java.util.stream.IntStream;
 
+import com.oracle.svm.hosted.substitute.SubstitutionMethod;
 import org.graalvm.collections.EconomicMap;
 import org.graalvm.nativeimage.AnnotationAccess;
 import org.graalvm.nativeimage.ImageSingletons;
@@ -453,16 +454,32 @@ protected boolean delegateLoadType(PersistedAnalysisType.Reader typeData) {
         return false;
     }
 
+    /**
+     * The {@link SubstitutionMethod} contains less information than the original
+     * {@link ResolvedJavaMethod} and trying to access it can result in an exception.
+     */
+    private static ResolvedJavaMethod getOriginalWrapped(AnalysisMethod method) {
+        ResolvedJavaMethod wrapped = method.getWrapped();
+        if (wrapped instanceof SubstitutionMethod subst) {
+            return subst.getAnnotated();
+        }
+        return wrapped;
+    }
+
     /**
      * Load all lambda types of the given capturing class. Each method of the capturing class is
      * parsed (see {@link LambdaParser#createMethodGraph(ResolvedJavaMethod, OptionValues)}). The
      * lambda types can then be found in the constant nodes of the graphs.
      */
     private void loadLambdaTypes(Class<?> capturingClass) {
         capturingClasses.computeIfAbsent(capturingClass, key -> {
+            /*
+             * Getting the original wrapped method is important to avoid getting exceptions that
+             * would be ignored otherwise.
+             */
             LambdaParser.allExecutablesDeclaredInClass(universe.getBigbang().getMetaAccess().lookupJavaType(capturingClass))
                             .filter(m -> m.getCode() != null)
-                            .forEach(m -> loadLambdaTypes(((AnalysisMethod) m).getWrapped(), universe.getBigbang()));
+                            .forEach(m -> loadLambdaTypes(getOriginalWrapped((AnalysisMethod) m), universe.getBigbang()));
             return true;
         });
     }

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/SVMImageLayerSnapshotUtil.java

@@ -299,7 +299,13 @@ public String getMethodDescriptor(AnalysisMethod method) {
         if (originalMethod != null) {
             return addModuleName(originalMethod.toString(), moduleName);
         }
-        return addModuleName(getQualifiedName(method), moduleName);
+        /*
+         * The wrapped qualified method is needed here as the AnalysisMethod replaces unresolved
+         * parameter or return types with java.lang.Object, potentially causing method descriptor
+         * duplication. The wrapped method signature preserves the original type information,
+         * preventing this issue.
+         */
+        return addModuleName(getWrappedQualifiedName(method), moduleName);
     }
 
     /*
@@ -326,6 +332,10 @@ private static String getQualifiedName(AnalysisMethod method) {
         return method.getSignature().getReturnType().toJavaName(true) + " " + method.getQualifiedName();
     }
 
+    private static String getWrappedQualifiedName(AnalysisMethod method) {
+        return method.wrapped.format("%R %H.%n(%P)");
+    }
+
     public static void forcePersistConstant(ImageHeapConstant imageHeapConstant) {
         AnalysisUniverse universe = imageHeapConstant.getType().getUniverse();
         universe.getHeapScanner().markReachable(imageHeapConstant, ObjectScanner.OtherReason.PERSISTED);

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/imagelayer/SVMImageLayerWriter.java

@@ -204,7 +204,13 @@ public class SVMImageLayerWriter extends ImageLayerWriter {
     private final MessageBuilder snapshotFileBuilder = new MessageBuilder();
     private final SharedLayerSnapshot.Builder snapshotBuilder = this.snapshotFileBuilder.initRoot(SharedLayerSnapshot.factory);
     private Map<ImageHeapConstant, ConstantParent> constantsMap;
-    private final Map<String, MethodGraphsInfo> methodsMap = new ConcurrentHashMap<>();
+    private final Map<AnalysisMethod, MethodGraphsInfo> methodsMap = new ConcurrentHashMap<>();
+    /**
+     * This map is only used for validation, to ensure that all method descriptors are unique. A
+     * duplicate method descriptor would cause methods to be incorrectly matched across layers,
+     * which is really hard to debug and can have unexpected consequences.
+     */
+    private final Map<String, AnalysisMethod> methodDescriptors = new HashMap<>();
     private final Map<AnalysisMethod, Set<AnalysisMethod>> polymorphicSignatureCallers = new ConcurrentHashMap<>();
     private final GraphsOutput graphsOutput;
     private final boolean useSharedLayerGraphs;
@@ -231,13 +237,13 @@ private record MethodGraphsInfo(String analysisGraphLocation, boolean analysisGr
 
         static final MethodGraphsInfo NO_GRAPHS = new MethodGraphsInfo(null, false, null);
 
-        MethodGraphsInfo withAnalysisGraph(String location, boolean isIntrinsic) {
-            assert analysisGraphLocation == null && !analysisGraphIsIntrinsic;
+        MethodGraphsInfo withAnalysisGraph(AnalysisMethod method, String location, boolean isIntrinsic) {
+            assert analysisGraphLocation == null && !analysisGraphIsIntrinsic : "Only one analysis graph can be persisted for a given method: " + method;
             return new MethodGraphsInfo(location, isIntrinsic, strengthenedGraphLocation);
         }
 
-        MethodGraphsInfo withStrengthenedGraph(String location) {
-            assert strengthenedGraphLocation == null;
+        MethodGraphsInfo withStrengthenedGraph(AnalysisMethod method, String location) {
+            assert strengthenedGraphLocation == null : "Only one strengthened graph can be persisted for a given method: " + method;
             return new MethodGraphsInfo(analysisGraphLocation, analysisGraphIsIntrinsic, location);
         }
     }
@@ -379,6 +385,7 @@ public void persistAnalysisInfo() {
         AnalysisMethod[] methodsToPersist = aUniverse.getMethods().stream().filter(AnalysisMethod::isTrackedAcrossLayers).sorted(Comparator.comparingInt(AnalysisMethod::getId))
                         .toArray(AnalysisMethod[]::new);
         initSortedArray(snapshotBuilder::initMethods, methodsToPersist, this::persistMethod);
+        methodDescriptors.clear();
 
         AnalysisField[] fieldsToPersist = aUniverse.getFields().stream().filter(AnalysisField::isTrackedAcrossLayers).sorted(Comparator.comparingInt(AnalysisField::getId))
                         .toArray(AnalysisField[]::new);
@@ -582,18 +589,20 @@ private static void handleNameConflict(String message) {
 
     private void persistMethod(AnalysisMethod method, Supplier<PersistedAnalysisMethod.Builder> builderSupplier) {
         PersistedAnalysisMethod.Builder builder = builderSupplier.get();
-        MethodGraphsInfo graphsInfo = methodsMap.putIfAbsent(imageLayerSnapshotUtil.getMethodDescriptor(method), MethodGraphsInfo.NO_GRAPHS);
+        MethodGraphsInfo graphsInfo = methodsMap.putIfAbsent(method, MethodGraphsInfo.NO_GRAPHS);
         Executable executable = method.getJavaMethod();
 
-        if (builder.getId() != 0) {
-            throw GraalError.shouldNotReachHere("The method descriptor should be unique, but " + imageLayerSnapshotUtil.getMethodDescriptor(method) + " got added twice.");
-        }
         if (executable != null) {
             initStringList(builder::initArgumentClassNames, Arrays.stream(executable.getParameterTypes()).map(Class::getName));
             builder.setClassName(executable.getDeclaringClass().getName());
         }
 
-        builder.setDescriptor(imageLayerSnapshotUtil.getMethodDescriptor(method));
+        String methodDescriptor = imageLayerSnapshotUtil.getMethodDescriptor(method);
+        if (methodDescriptors.put(methodDescriptor, method) != null) {
+            throw GraalError.shouldNotReachHere("The method descriptor should be unique, but %s got added twice.\nThe first method is %s and the second is %s."
+                            .formatted(methodDescriptor, methodDescriptors.get(methodDescriptor), method));
+        }
+        builder.setDescriptor(methodDescriptor);
         builder.setDeclaringTypeId(method.getDeclaringClass().getId());
         initInts(builder::initArgumentTypeIds, method.getSignature().toParameterList(null).stream().mapToInt(AnalysisType::getId));
         builder.setId(method.getId());
@@ -1053,18 +1062,19 @@ private static AnalysisMethod getRelocatableConstantMethod(MethodRef methodRef)
 
     @Override
     public void persistAnalysisParsedGraph(AnalysisMethod method, AnalysisParsedGraph analysisParsedGraph) {
-        String name = imageLayerSnapshotUtil.getMethodDescriptor(method);
-        MethodGraphsInfo graphsInfo = methodsMap.get(name);
-        if (graphsInfo == null || graphsInfo.analysisGraphLocation == null) {
+        /*
+         * A copy of the encoded graph is needed here because the nodeStartOffsets can be
+         * concurrently updated otherwise, which causes the ObjectCopier to fail.
+         */
+        String location = persistGraph(method, new EncodedGraph(analysisParsedGraph.getEncodedGraph()));
+        if (location != null) {
             /*
-             * A copy of the encoded graph is needed here because the nodeStartOffsets can be
-             * concurrently updated otherwise, which causes the ObjectCopier to fail.
+             * This method should only be called once for each method. This check is performed by
+             * withAnalysisGraph as it will throw if the MethodGraphsInfo already has an analysis
+             * graph.
              */
-            String location = persistGraph(method, new EncodedGraph(analysisParsedGraph.getEncodedGraph()));
-            if (location != null) {
-                methodsMap.compute(name, (n, mgi) -> (mgi != null ? mgi : MethodGraphsInfo.NO_GRAPHS)
-                                .withAnalysisGraph(location, analysisParsedGraph.isIntrinsic()));
-            }
+            methodsMap.compute(method, (n, mgi) -> (mgi != null ? mgi : MethodGraphsInfo.NO_GRAPHS)
+                            .withAnalysisGraph(method, location, analysisParsedGraph.isIntrinsic()));
         }
     }
 
@@ -1073,14 +1083,14 @@ public void persistMethodStrengthenedGraph(AnalysisMethod method) {
             return;
         }
 
-        String name = imageLayerSnapshotUtil.getMethodDescriptor(method);
-        MethodGraphsInfo graphsInfo = methodsMap.get(name);
-
-        if (graphsInfo == null || graphsInfo.strengthenedGraphLocation == null) {
-            EncodedGraph analyzedGraph = method.getAnalyzedGraph();
-            String location = persistGraph(method, analyzedGraph);
-            methodsMap.compute(name, (n, mgi) -> (mgi != null ? mgi : MethodGraphsInfo.NO_GRAPHS).withStrengthenedGraph(location));
-        }
+        EncodedGraph analyzedGraph = method.getAnalyzedGraph();
+        String location = persistGraph(method, analyzedGraph);
+        /*
+         * This method should only be called once for each method. This check is performed by
+         * withStrengthenedGraph as it will throw if the MethodGraphsInfo already has a strengthened
+         * graph.
+         */
+        methodsMap.compute(method, (n, mgi) -> (mgi != null ? mgi : MethodGraphsInfo.NO_GRAPHS).withStrengthenedGraph(method, location));
     }
 
     private String persistGraph(AnalysisMethod method, EncodedGraph analyzedGraph) {

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/methodhandles/MethodHandleFeature.java

@@ -259,10 +259,12 @@ public Object transform(Object receiver, Object originalValue) {
                                 for (var entry : cache.entrySet()) {
                                     SoftReference<Object> value = entry.getValue();
                                     Object object = value.get();
-                                    MethodHandle constructor = ReflectionUtil.invokeMethod(constructorGetter, object);
-                                    MethodHandle concatenator = ReflectionUtil.invokeMethod(concatenatorGetter, object);
-                                    if (constructor != null && concatenator != null && heapScanner.isObjectReachable(constructor) && heapScanner.isObjectReachable(concatenator)) {
-                                        result.put(entry.getKey(), value);
+                                    if (object != null) {
+                                        MethodHandle constructor = ReflectionUtil.invokeMethod(constructorGetter, object);
+                                        MethodHandle concatenator = ReflectionUtil.invokeMethod(concatenatorGetter, object);
+                                        if (constructor != null && concatenator != null && heapScanner.isObjectReachable(constructor) && heapScanner.isObjectReachable(concatenator)) {
+                                            result.put(entry.getKey(), value);
+                                        }
                                     }
                                 }