Fix open type world analysis.

cstancu · cstancu · commit 5f0d956f58bd · 2024-09-06T15:22:44.000+02:00
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
@@ -332,6 +332,10 @@ public boolean isFieldIncluded(BigBang bb, Field field) {
         return true;
     }
 
+    public boolean isClosedTypeWorld() {
+        return true;
+    }
+
     /**
      * Helpers to determine what analysis actions should be taken for a given Multi-Method version.
      */
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisMethod.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisMethod.java
@@ -911,6 +911,10 @@ public ResolvedJavaMethod unwrapTowardsOriginalMethod() {
     }
 
     public Executable getJavaMethod() {
+        if (wrapped instanceof BaseLayerMethod) {
+            /* We don't know the corresponding Java method. */
+            return null;
+        }
         return OriginalMethodProvider.getJavaMethod(this);
     }
 
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java
@@ -36,6 +36,7 @@
 import java.util.stream.Stream;
 
 import org.graalvm.collections.EconomicSet;
+import org.graalvm.nativeimage.AnnotationAccess;
 
 import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.PointsToAnalysis;
@@ -53,6 +54,8 @@
 import com.oracle.graal.pointsto.meta.PointsToAnalysisMethod;
 import com.oracle.graal.pointsto.typestate.PrimitiveConstantTypeState;
 import com.oracle.graal.pointsto.typestate.TypeState;
+import com.oracle.graal.pointsto.util.AnalysisError;
+import com.oracle.svm.core.annotate.Delete;
 import com.oracle.svm.util.ImageBuildStatistics;
 
 import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
@@ -182,6 +185,9 @@ public boolean equals(Object obj) {
     private final StrengthenGraphsCounters beforeCounters;
     private final StrengthenGraphsCounters afterCounters;
 
+    /** Used to avoid aggressive optimizations for open type world analysis. */
+    private final boolean isClosedTypeWorld;
+
     public StrengthenGraphs(BigBang bb, Universe converter) {
         this.bb = bb;
         this.converter = converter;
@@ -197,6 +203,7 @@ public StrengthenGraphs(BigBang bb, Universe converter) {
             beforeCounters = null;
             afterCounters = null;
         }
+        this.isClosedTypeWorld = converter.hostVM().isClosedTypeWorld();
     }
 
     private static void reportNeverNullInstanceFields(BigBang bb) {
@@ -291,7 +298,7 @@ public final void applyResults(AnalysisMethod method) {
     protected abstract boolean simplifyDelegate(Node n, SimplifierTool tool);
 
     /* Wrapper to clearly identify phase in IGV graph dumps. */
-    class AnalysisStrengthenGraphsPhase extends BasePhase<CoreProviders> {
+    public class AnalysisStrengthenGraphsPhase extends BasePhase<CoreProviders> {
         final CanonicalizerPhase phase;
 
         AnalysisStrengthenGraphsPhase(AnalysisMethod method, StructuredGraph graph) {
@@ -554,7 +561,8 @@ private void handleInvoke(Invoke invoke, SimplifierTool tool) {
             FixedNode node = invoke.asFixedNode();
             MethodCallTargetNode callTarget = (MethodCallTargetNode) invoke.callTarget();
 
-            if (callTarget.invokeKind().isDirect() && !((AnalysisMethod) callTarget.targetMethod()).isSimplyImplementationInvoked()) {
+            AnalysisMethod targetMethod = (AnalysisMethod) callTarget.targetMethod();
+            if (callTarget.invokeKind().isDirect() && !targetMethod.isSimplyImplementationInvoked()) {
                 /*
                  * This is a direct call to a method that the static analysis did not see as
                  * invoked. This can happen when the receiver is always null. In most cases, the
@@ -619,27 +627,66 @@ private void handleInvoke(Invoke invoke, SimplifierTool tool) {
                 }
             }
 
-            if (callees.size() == 1) {
+            if (callTarget.invokeKind().isDirect()) {
+                /*
+                 * Note: A direct invoke doesn't necessarily imply that the analysis should have
+                 * discovered a single callee. When dealing with interfaces it is in fact possible
+                 * that the Graal stamps are more accurate than the analysis results. So an
+                 * interface call may have already been optimized to a special call by stamp
+                 * strengthening of the receiver object, hence the invoke kind is direct, whereas
+                 * the points-to analysis inaccurately concluded there can be more than one callee.
+                 * 
+                 * Below we just check that if there is a direct invoke *and* the analysis
+                 * discovered a single callee, then the callee should match the target method.
+                 */
+                if (callees.size() == 1) {
+                    AnalysisMethod singleCallee = callees.iterator().next();
+                    assert targetMethod.equals(singleCallee) : "Direct invoke target mismatch: " + targetMethod + " != " + singleCallee + ". Called from " + graph.method().format("%H.%n");
+                }
+            } else if (AnnotationAccess.isAnnotationPresent(targetMethod, Delete.class)) {
+                /* We de-virtualize invokes to deleted methods since the callee must be unique. */
+                AnalysisError.guarantee(callees.size() == 1, "@Delete methods should have a single callee.");
                 AnalysisMethod singleCallee = callees.iterator().next();
-                if (callTarget.invokeKind().isDirect()) {
-                    assert callTarget.targetMethod().equals(singleCallee) : "Direct invoke target mismatch: " + callTarget.targetMethod() + " != " + singleCallee;
-                } else {
+                devirtualizeInvoke(singleCallee, invoke);
+            } else if (targetMethod.canBeStaticallyBound() || isClosedTypeWorld) {
+                /*
+                 * We only de-virtualize invokes if we run a closed type world analysis or the
+                 * target method can be trivially statically bound.
+                 */
+                if (callees.size() == 1) {
+                    AnalysisMethod singleCallee = callees.iterator().next();
                     devirtualizeInvoke(singleCallee, invoke);
-                }
+                } else {
+                    TypeState receiverTypeState = null;
+                    /* If the receiver flow is saturated, its exact type state does not matter. */
+                    if (invokeFlow.getTargetMethod().hasReceiver() && !methodFlow.isSaturated((PointsToAnalysis) bb, invokeFlow.getReceiver())) {
+                        receiverTypeState = methodFlow.foldTypeFlow((PointsToAnalysis) bb, invokeFlow.getReceiver());
+                    }
 
-            } else {
-                TypeState receiverTypeState = null;
-                /* If the receiver flow is saturated, its exact type state does not matter. */
-                if (invokeFlow.getTargetMethod().hasReceiver() && !methodFlow.isSaturated((PointsToAnalysis) bb, invokeFlow.getReceiver())) {
-                    receiverTypeState = methodFlow.foldTypeFlow((PointsToAnalysis) bb, invokeFlow.getReceiver());
-                }
+                    /*
+                     * In an open type world we cannot trust the type state of the receiver for
+                     * virtual calls as new subtypes could be added later.
+                     *
+                     * Note: MethodFlowsGraph.saturateAllParameters() does saturate the receiver in
+                     * many cases, so the check above would also lead to a null typeProfile, but we
+                     * cannot guarantee that we cover all cases.
+                     */
+                    JavaTypeProfile typeProfile = makeTypeProfile(receiverTypeState);
+                    /*
+                     * In a closed type world analysis the method profile of an invoke is complete
+                     * and contains all the callees reachable at that invocation location. Even if
+                     * that invoke is saturated it is still correct as it contains all the reachable
+                     * implementations of the target method. However, in an open type world the
+                     * method profile of an invoke, saturated or not, is incomplete, as there can be
+                     * implementations that we haven't yet seen.
+                     */
+                    JavaMethodProfile methodProfile = makeMethodProfile(callees);
 
-                JavaTypeProfile typeProfile = makeTypeProfile(receiverTypeState);
-                JavaMethodProfile methodProfile = makeMethodProfile(callees);
-                assert typeProfile == null || typeProfile.getTypes().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
-                assert methodProfile == null || methodProfile.getMethods().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
+                    assert typeProfile == null || typeProfile.getTypes().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
+                    assert methodProfile == null || methodProfile.getMethods().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
 
-                setInvokeProfiles(invoke, typeProfile, methodProfile);
+                    setInvokeProfiles(invoke, typeProfile, methodProfile);
+                }
             }
 
             if (allowOptimizeReturnParameter) {
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java
@@ -1264,14 +1264,19 @@ public static boolean includeAll() {
     public static final HostedOptionKey<Boolean> ForeignAPISupport = new HostedOptionKey<>(false);
 
     @Option(help = "Assume new types cannot be added after analysis", type = OptionType.Expert) //
-    public static final HostedOptionKey<Boolean> ClosedTypeWorld = new HostedOptionKey<>(true);
+    public static final HostedOptionKey<Boolean> ClosedTypeWorld = new HostedOptionKey<>(true) {
+        @Override
+        protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean oldValue, Boolean newValue) {
+            ClosedTypeWorldHubLayout.update(values, newValue);
+        }
+    };
 
-    @Option(help = "Use the closed type world dynamichub representation. This is only allowed when the option ClosedTypeWorld is also set to true.", type = OptionType.Expert) //
+    @Option(help = "Use the closed type world dynamic hub representation. This is only allowed when the option ClosedTypeWorld is also set to true.", type = OptionType.Expert) //
     public static final HostedOptionKey<Boolean> ClosedTypeWorldHubLayout = new HostedOptionKey<>(true);
 
     @Fold
     public static boolean useClosedTypeWorldHubLayout() {
-        return useClosedTypeWorld() && ClosedTypeWorldHubLayout.getValue();
+        return ClosedTypeWorldHubLayout.getValue();
     }
 
     @Fold
diff --git a/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/NonSnippetLowerings.java b/substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/NonSnippetLowerings.java
@@ -293,11 +293,13 @@ public static class InvokeLowering implements NodeLoweringProvider<FixedNode> {
         protected final RuntimeConfiguration runtimeConfig;
         protected final boolean verifyTypes;
         protected final KnownOffsets knownOffsets;
+        private final boolean isClosedTypeWorld;
 
         public InvokeLowering(RuntimeConfiguration runtimeConfig, boolean verifyTypes, KnownOffsets knownOffsets) {
             this.runtimeConfig = runtimeConfig;
             this.verifyTypes = verifyTypes;
             this.knownOffsets = knownOffsets;
+            isClosedTypeWorld = SubstrateOptions.useClosedTypeWorld();
         }
 
         @Override
@@ -366,14 +368,15 @@ public void lower(FixedNode node, LoweringTool tool) {
                 }
 
                 CallTargetNode loweredCallTarget;
-                if (invokeKind.isDirect() || implementations.length == 1) {
+                if (invokeKind.isDirect() || (implementations.length == 1 && (isClosedTypeWorld || method.canBeStaticallyBound()))) {
                     SharedMethod targetMethod = method;
                     if (!invokeKind.isDirect()) {
                         /*
                          * We only have one possible implementation for an indirect call, so we can
                          * emit a direct call to the unique implementation.
                          */
                         targetMethod = implementations[0];
+                        assert targetMethod != null : "Expecting a unique callee for target method " + method;
                     }
 
                     if (SubstrateUtil.HOSTED && targetMethod.forceIndirectCall()) {
@@ -421,7 +424,7 @@ public void lower(FixedNode node, LoweringTool tool) {
                                         address, parameters.toArray(new ValueNode[parameters.size()]), callTarget.returnStamp(), signature, targetMethod, callType, invokeKind));
                         graph.addBeforeFixed(node, codeStart);
                     }
-                } else if (implementations.length == 0) {
+                } else if (implementations.length == 0 && isClosedTypeWorld) {
                     /*
                      * We are calling an abstract method with no implementation, i.e., the
                      * closed-world analysis showed that there is no concrete receiver ever
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
@@ -201,6 +201,7 @@ public enum UsageKind {
     private Set<Field> excludedFields;
 
     private final Boolean optionAllowUnsafeAllocationOfAllInstantiatedTypes = SubstrateOptions.AllowUnsafeAllocationOfAllInstantiatedTypes.getValue();
+    private final boolean isClosedTypeWorld = SubstrateOptions.useClosedTypeWorld();
 
     @SuppressWarnings("this-escape")
     public SVMHost(OptionValues options, ImageClassLoader loader, ClassInitializationSupport classInitializationSupport, AnnotationSubstitutionProcessor annotationSubstitutions,
@@ -936,6 +937,11 @@ public boolean isFieldIncluded(BigBang bb, Field field) {
         return super.isFieldIncluded(bb, field);
     }
 
+    @Override
+    public boolean isClosedTypeWorld() {
+        return isClosedTypeWorld;
+    }
+
     private final List<BiPredicate<AnalysisMethod, AnalysisMethod>> neverInlineTrivialHandlers = new CopyOnWriteArrayList<>();
 
     public void registerNeverInlineTrivialHandler(BiPredicate<AnalysisMethod, AnalysisMethod> handler) {
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/HostedInstanceClass.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/HostedInstanceClass.java
@@ -26,8 +26,10 @@
 
 import com.oracle.graal.pointsto.meta.AnalysisType;
 
+import jdk.vm.ci.meta.Assumptions.AssumptionResult;
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.ResolvedJavaField;
+import jdk.vm.ci.meta.ResolvedJavaMethod;
 
 public class HostedInstanceClass extends HostedClass {
 
@@ -138,4 +140,16 @@ public void setIdentityHashOffset(int offset) {
         assert offset > 0;
         this.identityHashOffset = offset;
     }
+
+    @Override
+    public AssumptionResult<ResolvedJavaMethod> findUniqueConcreteMethod(ResolvedJavaMethod m) {
+        if (m.canBeStaticallyBound() || universe.hostVM().isClosedTypeWorld()) {
+            return super.findUniqueConcreteMethod(m);
+        }
+        /*
+         * With an open type world analysis we cannot make assumptions for methods that cannot be
+         * trivially statically bound.
+         */
+        return null;
+    }
 }
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/HostedMethod.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/HostedMethod.java
@@ -111,6 +111,10 @@ public final class HostedMethod extends HostedElement implements SharedMethod, W
     /**
      * All concrete methods that can actually be called when calling this method. This includes all
      * overridden methods in subclasses, as well as this method if it is non-abstract.
+     * <p>
+     * With an open type world analysis the list of implementations is incomplete, i.e., no
+     * aggressive optimizations should be performed based on the contents of this list as one must
+     * assume that additional implementations can be discovered later.
      */
     HostedMethod[] implementations;
 
@@ -340,7 +344,7 @@ public boolean hasVTableIndex() {
 
     @Override
     public int getVTableIndex() {
-        assert vtableIndex != -1;
+        assert vtableIndex != -1 : "Missing vtable index for method " + this.format("%H.%n(%p)");
         return vtableIndex;
     }
 
@@ -451,7 +455,14 @@ public boolean isConstructor() {
 
     @Override
     public boolean canBeStaticallyBound() {
-        return implementations.length == 1 && implementations[0].equals(this);
+        if (holder.universe.hostVM().isClosedTypeWorld()) {
+            return implementations.length == 1 && implementations[0].equals(this);
+        }
+        /*
+         * In open type world analysis we cannot make assumptions based on discovered
+         * implementations.
+         */
+        return wrapped.canBeStaticallyBound();
     }
 
     @Override
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/TypeCheckBuilder.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/TypeCheckBuilder.java
@@ -153,9 +153,10 @@ public final class TypeCheckBuilder {
         VMError.guarantee(result != 0, "Unexpected match of types %s %s", o1, o2);
         return result;
     };
+    private final boolean isClosedTypeWorld;
 
     public static int buildTypeMetadata(HostedUniverse hUniverse, Collection<HostedType> types, HostedType objectType, HostedType cloneableType, HostedType serializableType) {
-        var builder = new TypeCheckBuilder(types, objectType, cloneableType, serializableType);
+        var builder = new TypeCheckBuilder(types, objectType, cloneableType, serializableType, hUniverse.hostVM().isClosedTypeWorld());
         if (SubstrateOptions.useClosedTypeWorldHubLayout()) {
             builder.buildTypeInformation(hUniverse, 0);
             builder.calculateClosedTypeWorldTypeMetadata();
@@ -169,7 +170,7 @@ public static int buildTypeMetadata(HostedUniverse hUniverse, Collection<HostedT
         }
     }
 
-    private TypeCheckBuilder(Collection<HostedType> types, HostedType objectType, HostedType cloneableType, HostedType serializableType) {
+    private TypeCheckBuilder(Collection<HostedType> types, HostedType objectType, HostedType cloneableType, HostedType serializableType, boolean isClosedTypeWorld) {
         this.allTypes = types;
         this.objectType = objectType;
         this.cloneableType = cloneableType;
@@ -187,6 +188,7 @@ private TypeCheckBuilder(Collection<HostedType> types, HostedType objectType, Ho
         allIncludedRoots = allIncludedTypes.stream().filter(t -> !hasParent.contains(t)).toList();
 
         heightOrderedTypes = generateHeightOrder(allIncludedRoots, subtypeMap);
+        this.isClosedTypeWorld = isClosedTypeWorld;
     }
 
     private int getNumTypeCheckSlots() {
@@ -472,6 +474,16 @@ public void buildTypeInformation(HostedUniverse hUniverse, int startingTypeID) {
         for (int i = heightOrderedTypes.size() - 1; i >= 0; i--) {
             HostedType type = heightOrderedTypes.get(i);
 
+            if (!type.isLeaf() && !isClosedTypeWorld) {
+                /*
+                 * With an open type world analysis we have to assume that a non-final type can have
+                 * multiple instantiated subtypes.
+                 */
+                type.strengthenStampType = type;
+                type.uniqueConcreteImplementation = null;
+                continue;
+            }
+
             HostedType subtypeStampType = null;
             for (HostedType child : subtypeMap.get(type)) {
                 if (child.strengthenStampType != null) {
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/VTableBuilder.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/VTableBuilder.java
@@ -39,7 +39,6 @@
 import com.oracle.svm.core.InvalidMethodPointerHandler;
 import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.SubstrateUtil;
-import com.oracle.svm.core.imagelayer.ImageLayerBuildingSupport;
 import com.oracle.svm.hosted.OpenTypeWorldFeature;
 
 import jdk.graal.compiler.debug.Assertions;
@@ -119,12 +118,12 @@ private List<HostedMethod> generateITable(HostedType type) {
 
     private List<HostedMethod> generateDispatchTable(HostedType type, int startingIndex) {
         Predicate<HostedMethod> includeMethod;
-        if (ImageLayerBuildingSupport.buildingImageLayer()) {
-            // include all methods
-            includeMethod = m -> true;
-        } else {
+        if (hUniverse.hostVM().isClosedTypeWorld()) {
             // include only methods which will be indirect calls
             includeMethod = m -> m.implementations.length > 1 || m.wrapped.isVirtualRootMethod();
+        } else {
+            // include all methods
+            includeMethod = m -> true;
         }
         var table = type.getWrapped().getOpenTypeWorldDispatchTableMethods().stream().map(hUniverse::lookup).filter(includeMethod).sorted(HostedUniverse.METHOD_COMPARATOR).toList();
 

Original file line number	Diff line number	Diff line change
`@@ -911,6 +911,10 @@ public ResolvedJavaMethod unwrapTowardsOriginalMethod() {`
`911`	`911`	`}`
`912`	`912`
`913`	`913`	`public Executable getJavaMethod() {`
	`914`	`+ if (wrapped instanceof BaseLayerMethod) {`
	`915`	`+ /* We don't know the corresponding Java method. */`
	`916`	`+ return null;`
	`917`	`+ }`
`914`	`918`	`return OriginalMethodProvider.getJavaMethod(this);`
`915`	`919`	`}`
`916`	`920`