[GR-33693] Do not expose that static fields are stored in arrays, to avoid memory graph problems in the compiler.

PullRequest: graal/9754

Author: Christian Wimmer

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/StaticFieldsSupport.java

@@ -24,17 +24,38 @@
  */
 package com.oracle.svm.core;
 
+import static org.graalvm.compiler.nodeinfo.NodeCycles.CYCLES_0;
+import static org.graalvm.compiler.nodeinfo.NodeSize.SIZE_1;
+
 import java.util.Objects;
 
+import org.graalvm.compiler.api.replacements.SnippetReflectionProvider;
+import org.graalvm.compiler.core.common.type.StampFactory;
+import org.graalvm.compiler.graph.NodeClass;
+import org.graalvm.compiler.nodeinfo.NodeInfo;
+import org.graalvm.compiler.nodes.ConstantNode;
+import org.graalvm.compiler.nodes.calc.FloatingNode;
+import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration.Plugins;
+import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderContext;
+import org.graalvm.compiler.nodes.graphbuilderconf.InvocationPlugin;
+import org.graalvm.compiler.nodes.graphbuilderconf.InvocationPlugins.Registration;
+import org.graalvm.compiler.nodes.spi.Lowerable;
+import org.graalvm.compiler.nodes.spi.LoweringTool;
+import org.graalvm.compiler.phases.util.Providers;
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
-import org.graalvm.nativeimage.hosted.Feature;
+import org.graalvm.word.LocationIdentity;
 
 import com.oracle.svm.core.annotate.AutomaticFeature;
-import com.oracle.svm.core.annotate.Uninterruptible;
-import com.oracle.svm.core.annotate.UnknownObjectField;
+import com.oracle.svm.core.graal.GraalFeature;
 import com.oracle.svm.core.meta.SharedField;
+import com.oracle.svm.core.meta.SubstrateObjectConstant;
+import com.oracle.svm.core.util.VMError;
+
+import jdk.vm.ci.meta.JavaConstant;
+import jdk.vm.ci.meta.JavaKind;
+import jdk.vm.ci.meta.ResolvedJavaMethod;
 
 /**
  * Static fields are represented as two arrays in the native image heap: one for Object fields and
@@ -44,11 +65,24 @@
  * Implementation notes: The arrays are created after static analysis, but before compilation. We
  * need to know how many static fields are reachable in order to compute the appropriate size for
  * the arrays, which is only available after static analysis.
+ * 
+ * When bytecode is parsed before static analysis, the arrays are not available yet. Therefore, the
+ * accessor functions {@link #getStaticObjectFields()}} and {@link #getStaticPrimitiveFields()} are
+ * intrinsified to a {@link StaticFieldBaseNode}, which is then during compilation lowered to the
+ * constant arrays. This also solves memory graph problems in the Graal compiler: Direct
+ * loads/stores using the arrays, for example via Unsafe or VarHandle, alias with static field
+ * loads/stores that have dedicated {@link LocationIdentity}. If the arrays are already exposed in
+ * the high-level optimization phases of Graal, the compiler would miss the alias since the location
+ * identities for arrays are considered non-aliasing with location identities for fields. Replacing
+ * the {@link StaticFieldBaseNode} with a {@link ConstantNode} only in the low tier of the compiler
+ * solves this problem.
  */
 public final class StaticFieldsSupport {
 
-    @UnknownObjectField(types = {Object[].class}) private Object[] staticObjectFields;
-    @UnknownObjectField(types = {byte[].class}) private byte[] staticPrimitiveFields;
+    @Platforms(Platform.HOSTED_ONLY.class) //
+    private Object[] staticObjectFields;
+    @Platforms(Platform.HOSTED_ONLY.class) //
+    private byte[] staticPrimitiveFields;
 
     @Platforms(Platform.HOSTED_ONLY.class)
     protected StaticFieldsSupport() {
@@ -61,30 +95,98 @@ public static void setData(Object[] staticObjectFields, byte[] staticPrimitiveFi
         support.staticPrimitiveFields = Objects.requireNonNull(staticPrimitiveFields);
     }
 
-    @Platforms(Platform.HOSTED_ONLY.class)
-    public static boolean dataAvailable() {
-        return ImageSingletons.lookup(StaticFieldsSupport.class).staticObjectFields != null;
-    }
-
-    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-    public static Object[] getStaticObjectFields() {
+    /* Intrinsified by the graph builder plugin below. */
+    public static Object getStaticObjectFields() {
         Object[] result = ImageSingletons.lookup(StaticFieldsSupport.class).staticObjectFields;
-        assert result != null;
+        VMError.guarantee(result != null, "arrays that hold static fields are only available after static analysis");
         return result;
     }
 
-    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
-    public static byte[] getStaticPrimitiveFields() {
+    /* Intrinsified by the graph builder plugin below. */
+    public static Object getStaticPrimitiveFields() {
         byte[] result = ImageSingletons.lookup(StaticFieldsSupport.class).staticPrimitiveFields;
-        assert result != null;
+        VMError.guarantee(result != null, "arrays that hold static fields are only available after static analysis");
         return result;
     }
+
+    public static FloatingNode createStaticFieldBaseNode(boolean primitive) {
+        return new StaticFieldBaseNode(primitive);
+    }
 }
 
 @AutomaticFeature
-class StaticFieldsFeature implements Feature {
+final class StaticFieldsFeature implements GraalFeature {
     @Override
     public void afterRegistration(AfterRegistrationAccess access) {
         ImageSingletons.add(StaticFieldsSupport.class, new StaticFieldsSupport());
     }
+
+    @Override
+    public void registerInvocationPlugins(Providers providers, SnippetReflectionProvider snippetReflection, Plugins plugins, ParsingReason reason) {
+        Registration r = new Registration(plugins.getInvocationPlugins(), StaticFieldsSupport.class);
+        r.register0("getStaticObjectFields", new InvocationPlugin() {
+            @Override
+            public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver unused) {
+                b.addPush(JavaKind.Object, new StaticFieldBaseNode(false));
+                return true;
+            }
+        });
+        r.register0("getStaticPrimitiveFields", new InvocationPlugin() {
+            @Override
+            public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver unused) {
+                b.addPush(JavaKind.Object, new StaticFieldBaseNode(true));
+                return true;
+            }
+        });
+    }
+}
+
+@NodeInfo(cycles = CYCLES_0, size = SIZE_1)
+final class StaticFieldBaseNode extends FloatingNode implements Lowerable {
+    public static final NodeClass<StaticFieldBaseNode> TYPE = NodeClass.create(StaticFieldBaseNode.class);
+
+    private final boolean primitive;
+
+    /**
+     * We must not expose that the stamp will eventually be an array, to avoid memory graph
+     * problems. See the comment on {@link StaticFieldsSupport}.
+     */
+    protected StaticFieldBaseNode(boolean primitive) {
+        super(TYPE, StampFactory.objectNonNull());
+        this.primitive = primitive;
+    }
+
+    /**
+     * At first glance, this method looks like a circular dependency:
+     * {@link StaticFieldsSupport#getStaticPrimitiveFields} is intrinsified to a
+     * {@link StaticFieldBaseNode}, and {@link StaticFieldBaseNode} is lowered by calling
+     * {@link StaticFieldsSupport#getStaticPrimitiveFields}. So why does this code work?
+     * 
+     * The intrinsification to the {@link StaticFieldBaseNode} is only effective for code executed
+     * at image run time. So when executed during AOT compilation, {@link StaticFieldBaseNode#lower}
+     * really invokes {@link StaticFieldsSupport#getStaticPrimitiveFields}, which returns the proper
+     * result.
+     * 
+     * For an image that uses Graal as a JIT compiler, {@link StaticFieldBaseNode#lower} is
+     * reachable at run time. But it is AOT compiled, and lowering during that AOT compilation again
+     * invokes {@link StaticFieldsSupport#getStaticPrimitiveFields}.
+     * 
+     * So in summary, this code works because there is proper "bootstrapping" during AOT compilation
+     * where the intrinsification is not applied.
+     */
+    @Override
+    public void lower(LoweringTool tool) {
+        if (tool.getLoweringStage() != LoweringTool.StandardLoweringStage.LOW_TIER) {
+            /*
+             * Lowering to a ConstantNode must only happen after the memory graph has been built,
+             * i.e., when the information that static fields are stored in an array is no longer
+             * misleading alias analysis.
+             */
+            return;
+        }
+
+        JavaConstant constant = SubstrateObjectConstant.forObject(primitive ? StaticFieldsSupport.getStaticPrimitiveFields() : StaticFieldsSupport.getStaticObjectFields());
+        assert constant.isNonNull();
+        replaceAndDelete(ConstantNode.forConstant(constant, tool.getMetaAccess(), graph()));
+    }
 }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/meta/SubstrateBasicLoweringProvider.java

@@ -79,7 +79,6 @@
 import com.oracle.svm.core.hub.DynamicHub;
 import com.oracle.svm.core.identityhashcode.IdentityHashCodeSupport;
 import com.oracle.svm.core.meta.SharedField;
-import com.oracle.svm.core.meta.SubstrateObjectConstant;
 import com.oracle.svm.core.snippets.SubstrateIsArraySnippets;
 
 import jdk.vm.ci.code.CodeUtil;
@@ -151,8 +150,7 @@ public int arrayLengthOffset() {
     public ValueNode staticFieldBase(StructuredGraph graph, ResolvedJavaField f) {
         SharedField field = (SharedField) f;
         assert field.isStatic();
-        Object fields = field.getStorageKind() == JavaKind.Object ? StaticFieldsSupport.getStaticObjectFields() : StaticFieldsSupport.getStaticPrimitiveFields();
-        return ConstantNode.forConstant(SubstrateObjectConstant.forObject(fields), getProviders().getMetaAccess(), graph);
+        return graph.unique(StaticFieldsSupport.createStaticFieldBaseNode(field.getStorageKind() != JavaKind.Object));
     }
 
     private static ValueNode maybeUncompress(ValueNode node) {

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/snippets/SubstrateGraphBuilderPlugins.java

@@ -677,26 +677,12 @@ private static boolean isValidField(Field targetField, boolean isSunMiscUnsafe)
         return true;
     }
 
-    private static final Function<CoreProviders, JavaConstant> primitiveStaticFieldBaseConstant = new Function<CoreProviders, JavaConstant>() {
-        @Override
-        public JavaConstant apply(CoreProviders providers) {
-            return StaticFieldsSupport.dataAvailable() ? SubstrateObjectConstant.forObject(StaticFieldsSupport.getStaticPrimitiveFields()) : null;
-        }
-    };
-    private static final Function<CoreProviders, JavaConstant> objectStaticFieldBaseConstant = new Function<CoreProviders, JavaConstant>() {
-        @Override
-        public JavaConstant apply(CoreProviders providers) {
-            return StaticFieldsSupport.dataAvailable() ? SubstrateObjectConstant.forObject(StaticFieldsSupport.getStaticObjectFields()) : null;
-        }
-    };
-
     private static boolean processStaticFieldBase(GraphBuilderContext b, Field targetField, boolean isSunMiscUnsafe) {
         if (!isValidField(targetField, isSunMiscUnsafe)) {
             return false;
         }
 
-        b.addPush(JavaKind.Object, LazyConstantNode.create(StampFactory.objectNonNull(),
-                        targetField.getType().isPrimitive() ? primitiveStaticFieldBaseConstant : objectStaticFieldBaseConstant, b));
+        b.addPush(JavaKind.Object, StaticFieldsSupport.createStaticFieldBaseNode(targetField.getType().isPrimitive()));
         return true;
     }
 

substratevm/src/com.oracle.svm.jni/src/com/oracle/svm/jni/JNIGeneratedMethodSupport.java

@@ -81,11 +81,11 @@ static WordBase getFieldOffsetFromId(JNIFieldId fieldId) {
         return JNIAccessibleField.getOffsetFromId(fieldId);
     }
 
-    static byte[] getStaticPrimitiveFieldsArray() {
+    static Object getStaticPrimitiveFieldsArray() {
         return StaticFieldsSupport.getStaticPrimitiveFields();
     }
 
-    static Object[] getStaticObjectFieldsArray() {
+    static Object getStaticObjectFieldsArray() {
         return StaticFieldsSupport.getStaticObjectFields();
     }