[GR-58381] Synchronize accesses to class init state

PullRequest: graal/18879
oracle · Oct 7, 2024 · b9b4c7d · b9b4c7d
2 parents 437a2c4 + db9b041
commit b9b4c7d
Show file tree

Hide file tree

Showing 12 changed files with 187 additions and 80 deletions.
diff --git a/...dk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/DefaultHotSpotLoweringProvider.java b/...dk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/DefaultHotSpotLoweringProvider.java
@@ -38,7 +38,6 @@
 import java.util.List;
 import java.util.Map;
 
-import jdk.graal.compiler.serviceprovider.LibGraalService;
 import org.graalvm.word.LocationIdentity;
 
 import jdk.graal.compiler.core.common.CompressEncoding;
@@ -70,6 +69,7 @@
 import jdk.graal.compiler.hotspot.nodes.HotSpotDirectCallTargetNode;
 import jdk.graal.compiler.hotspot.nodes.HotSpotIndirectCallTargetNode;
 import jdk.graal.compiler.hotspot.nodes.KlassBeingInitializedCheckNode;
+import jdk.graal.compiler.hotspot.nodes.KlassFullyInitializedCheckNode;
 import jdk.graal.compiler.hotspot.nodes.VMErrorNode;
 import jdk.graal.compiler.hotspot.nodes.VirtualThreadUpdateJFRNode;
 import jdk.graal.compiler.hotspot.nodes.type.HotSpotNarrowOopStamp;
@@ -203,6 +203,7 @@
 import jdk.graal.compiler.replacements.nodes.CStringConstant;
 import jdk.graal.compiler.replacements.nodes.LogNode;
 import jdk.graal.compiler.serviceprovider.GraalServices;
+import jdk.graal.compiler.serviceprovider.LibGraalService;
 import jdk.vm.ci.code.CodeUtil;
 import jdk.vm.ci.code.TargetDescription;
 import jdk.vm.ci.hotspot.HotSpotCallingConventionType;
@@ -252,7 +253,7 @@ void initialize(HotSpotProviders providers,
     public interface Extensions {
         /**
          * Gets the extensions provided by this object.
-         *
+         * <p>
          * In the context of service caching done when building a libgraal image, implementations of
          * this method must return a new value each time to avoid sharing extensions between
          * different {@link DefaultHotSpotLoweringProvider}s.
@@ -549,6 +550,10 @@ private boolean lowerWithoutDelegation(Node n, LoweringTool tool) {
             if (graph.getGuardsStage() == GuardsStage.AFTER_FSA) {
                 getAllocationSnippets().lower((KlassBeingInitializedCheckNode) n, tool);
             }
+        } else if (n instanceof KlassFullyInitializedCheckNode) {
+            if (graph.getGuardsStage() == GuardsStage.AFTER_FSA) {
+                getAllocationSnippets().lower((KlassFullyInitializedCheckNode) n, tool);
+            }
         } else if (n instanceof FastNotifyNode) {
             if (graph.getGuardsStage() == GuardsStage.AFTER_FSA) {
                 objectSnippets.lower(n, tool);

diff --git a/...rc/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java b/...rc/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
@@ -77,6 +77,7 @@
 import jdk.graal.compiler.hotspot.nodes.CurrentJavaThreadNode;
 import jdk.graal.compiler.hotspot.nodes.HotSpotLoadReservedReferenceNode;
 import jdk.graal.compiler.hotspot.nodes.HotSpotStoreReservedReferenceNode;
+import jdk.graal.compiler.hotspot.nodes.KlassFullyInitializedCheckNode;
 import jdk.graal.compiler.hotspot.replacements.CallSiteTargetNode;
 import jdk.graal.compiler.hotspot.replacements.DigestBaseSnippets;
 import jdk.graal.compiler.hotspot.replacements.FastNotifyNode;
@@ -140,6 +141,7 @@
 import jdk.graal.compiler.nodes.java.DynamicNewInstanceNode;
 import jdk.graal.compiler.nodes.java.DynamicNewInstanceWithExceptionNode;
 import jdk.graal.compiler.nodes.java.NewArrayNode;
+import jdk.graal.compiler.nodes.java.ValidateNewInstanceClassNode;
 import jdk.graal.compiler.nodes.memory.address.AddressNode;
 import jdk.graal.compiler.nodes.memory.address.OffsetAddressNode;
 import jdk.graal.compiler.nodes.spi.Replacements;
@@ -534,16 +536,18 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
                 }
                 /* Emits a null-check for the otherwise unused receiver. */
                 unsafe.get(true);
+
+                ValidateNewInstanceClassNode clazzLegal = b.add(new ValidateNewInstanceClassNode(clazz));
                 /*
-                 * Note that the provided clazz might not be initialized. The HotSpot lowering
-                 * snippet for DynamicNewInstanceNode performs the necessary class initialization
-                 * check. Such a DynamicNewInstanceNode is also never constant folded to a
-                 * NewInstanceNode.
+                 * Note that the provided clazz might not be initialized. The lowering snippet for
+                 * KlassFullyInitializedCheckNode performs the necessary initialization check.
                  */
+                b.add(new KlassFullyInitializedCheckNode(clazzLegal));
+
                 if (b.currentBlockCatchesOOM()) {
-                    DynamicNewInstanceWithExceptionNode.createAndPush(b, clazz, true);
+                    b.addPush(JavaKind.Object, new DynamicNewInstanceWithExceptionNode(clazzLegal, true));
                 } else {
-                    DynamicNewInstanceNode.createAndPush(b, clazz, true);
+                    b.addPush(JavaKind.Object, new DynamicNewInstanceNode(clazzLegal, true));
                 }
                 return true;
             }

diff --git a/...k.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassBeingInitializedCheckNode.java b/...k.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassBeingInitializedCheckNode.java
@@ -27,15 +27,19 @@
 import static jdk.graal.compiler.nodeinfo.NodeCycles.CYCLES_4;
 import static jdk.graal.compiler.nodeinfo.NodeSize.SIZE_16;
 
+import org.graalvm.word.LocationIdentity;
+
 import jdk.graal.compiler.core.common.type.StampFactory;
 import jdk.graal.compiler.graph.NodeClass;
+import jdk.graal.compiler.nodeinfo.InputType;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
 import jdk.graal.compiler.nodes.DeoptimizingFixedWithNextNode;
 import jdk.graal.compiler.nodes.ValueNode;
+import jdk.graal.compiler.nodes.memory.SingleMemoryKill;
 import jdk.graal.compiler.nodes.spi.Lowerable;
 
-@NodeInfo(cycles = CYCLES_4, size = SIZE_16)
-public class KlassBeingInitializedCheckNode extends DeoptimizingFixedWithNextNode implements Lowerable {
+@NodeInfo(allowedUsageTypes = {InputType.Memory}, cycles = CYCLES_4, size = SIZE_16)
+public class KlassBeingInitializedCheckNode extends DeoptimizingFixedWithNextNode implements Lowerable, SingleMemoryKill {
     public static final NodeClass<KlassBeingInitializedCheckNode> TYPE = NodeClass.create(KlassBeingInitializedCheckNode.class);
 
     @Input protected ValueNode klass;
@@ -45,6 +49,15 @@ public KlassBeingInitializedCheckNode(ValueNode klass) {
         this.klass = klass;
     }
 
+    @Override
+    public LocationIdentity getKilledLocationIdentity() {
+        /*
+         * Since JDK-8338379, reading the class init state requires an ACQUIRE barrier, which orders
+         * memory accesses
+         */
+        return LocationIdentity.ANY_LOCATION;
+    }
+
     public ValueNode getKlass() {
         return klass;
     }

diff --git a/...k.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassFullyInitializedCheckNode.java b/...k.graal.compiler/src/jdk/graal/compiler/hotspot/nodes/KlassFullyInitializedCheckNode.java
@@ -0,0 +1,80 @@
+/*
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package jdk.graal.compiler.hotspot.nodes;
+
+import static jdk.graal.compiler.nodeinfo.NodeCycles.CYCLES_4;
+import static jdk.graal.compiler.nodeinfo.NodeSize.SIZE_16;
+
+import org.graalvm.word.LocationIdentity;
+
+import jdk.graal.compiler.core.common.type.AbstractPointerStamp;
+import jdk.graal.compiler.core.common.type.Stamp;
+import jdk.graal.compiler.core.common.type.StampFactory;
+import jdk.graal.compiler.graph.NodeClass;
+import jdk.graal.compiler.nodeinfo.InputType;
+import jdk.graal.compiler.nodeinfo.NodeInfo;
+import jdk.graal.compiler.nodes.DeoptimizingFixedWithNextNode;
+import jdk.graal.compiler.nodes.NodeView;
+import jdk.graal.compiler.nodes.ValueNode;
+import jdk.graal.compiler.nodes.memory.SingleMemoryKill;
+import jdk.graal.compiler.nodes.spi.Lowerable;
+
+/**
+ * Checks that the input {@link Class}'s hub is either null or has been fully initialized, or
+ * deoptimizes otherwise.
+ */
+@NodeInfo(allowedUsageTypes = {InputType.Memory}, cycles = CYCLES_4, size = SIZE_16)
+public class KlassFullyInitializedCheckNode extends DeoptimizingFixedWithNextNode implements Lowerable, SingleMemoryKill {
+    public static final NodeClass<KlassFullyInitializedCheckNode> TYPE = NodeClass.create(KlassFullyInitializedCheckNode.class);
+
+    @Input protected ValueNode klass;
+
+    public KlassFullyInitializedCheckNode(ValueNode klassNonNull) {
+        super(TYPE, StampFactory.forVoid());
+        Stamp inputStamp = klassNonNull.stamp(NodeView.DEFAULT);
+        assert inputStamp instanceof AbstractPointerStamp : klassNonNull + " has wrong input stamp type for klass init state check: " + inputStamp;
+        assert ((AbstractPointerStamp) inputStamp).nonNull() : klassNonNull + " must have non-null stamp: " + inputStamp;
+        this.klass = klassNonNull;
+    }
+
+    @Override
+    public LocationIdentity getKilledLocationIdentity() {
+        /*
+         * Since JDK-8338379, reading the class init state requires an ACQUIRE barrier, which orders
+         * memory accesses
+         */
+        return LocationIdentity.ANY_LOCATION;
+    }
+
+    public ValueNode getKlass() {
+        return klass;
+    }
+
+    @Override
+    public boolean canDeoptimize() {
+        return true;
+    }
+
+}
diff --git a/...graal.compiler/src/jdk/graal/compiler/hotspot/replacements/HotSpotAllocationSnippets.java b/...graal.compiler/src/jdk/graal/compiler/hotspot/replacements/HotSpotAllocationSnippets.java
@@ -88,6 +88,7 @@
 import jdk.graal.compiler.hotspot.meta.HotSpotProviders;
 import jdk.graal.compiler.hotspot.meta.HotSpotRegistersProvider;
 import jdk.graal.compiler.hotspot.nodes.KlassBeingInitializedCheckNode;
+import jdk.graal.compiler.hotspot.nodes.KlassFullyInitializedCheckNode;
 import jdk.graal.compiler.hotspot.nodes.type.KlassPointerStamp;
 import jdk.graal.compiler.hotspot.word.KlassPointer;
 import jdk.graal.compiler.nodes.ConstantNode;
@@ -179,25 +180,21 @@ public Object allocateInstanceDynamic(@NonNullParameter Class<?> type,
         KlassPointer hub = ClassGetHubNode.readClass(type);
         if (probability(FAST_PATH_PROBABILITY, !hub.isNull())) {
             KlassPointer nonNullHub = ClassGetHubNode.piCastNonNull(hub, SnippetAnchorNode.anchor());
-
-            if (probability(VERY_FAST_PATH_PROBABILITY, isInstanceKlassFullyInitialized(nonNullHub))) {
-                int layoutHelper = readLayoutHelper(nonNullHub);
+            // klass initialization check was already performed by KlassFullyInitializedCheckNode
+            int layoutHelper = readLayoutHelper(nonNullHub);
+            /*
+             * src/share/vm/oops/klass.hpp: For instances, layout helper is a positive number, the
+             * instance size. This size is already passed through align_object_size and scaled to
+             * bytes. The low order bit is set if instances of this class cannot be allocated using
+             * the fastpath.
+             */
+            if (probability(FAST_PATH_PROBABILITY, (layoutHelper & 1) == 0)) {
                 /*
-                 * src/share/vm/oops/klass.hpp: For instances, layout helper is a positive number,
-                 * the instance size. This size is already passed through align_object_size and
-                 * scaled to bytes. The low order bit is set if instances of this class cannot be
-                 * allocated using the fastpath.
+                 * FIXME(je,ds): we should actually pass typeContext instead of "" but late binding
+                 * of parameters is not yet supported by the GraphBuilderPlugin system.
                  */
-                if (probability(FAST_PATH_PROBABILITY, (layoutHelper & 1) == 0)) {
-                    /*
-                     * FIXME(je,ds): we should actually pass typeContext instead of "" but late
-                     * binding of parameters is not yet supported by the GraphBuilderPlugin system.
-                     */
-                    UnsignedWord size = WordFactory.unsigned(layoutHelper);
-                    return allocateInstanceImpl(nonNullHub.asWord(), size, false, fillContents, emitMemoryBarrier, false, profilingData, withException);
-                }
-            } else {
-                DeoptimizeNode.deopt(None, RuntimeConstraint);
+                UnsignedWord size = WordFactory.unsigned(layoutHelper);
+                return allocateInstanceImpl(nonNullHub.asWord(), size, false, fillContents, emitMemoryBarrier, false, profilingData, withException);
             }
         }
         return PiNode.piCastToSnippetReplaceeStamp(dynamicNewInstanceStub(type, withException));
@@ -297,6 +294,17 @@ private void verifyHeap() {
         }
     }
 
+    @Snippet
+    private static void klassFullyInitializedCheck(@NonNullParameter Class<?> klass) {
+        KlassPointer hub = ClassGetHubNode.readClass(klass);
+        if (probability(VERY_FAST_PATH_PROBABILITY, !hub.isNull())) {
+            KlassPointer nonNullHub = ClassGetHubNode.piCastNonNull(hub, SnippetAnchorNode.anchor());
+            if (probability(DEOPT_PROBABILITY, !isInstanceKlassFullyInitialized(nonNullHub))) {
+                DeoptimizeNode.deopt(None, RuntimeConstraint);
+            }
+        }
+    }
+
     @Snippet
     private void threadBeingInitializedCheck(KlassPointer klass) {
         int state = readInstanceKlassInitState(klass);
@@ -548,6 +556,7 @@ public static class Templates extends AbstractTemplates {
         private final SnippetInfo newmultiarray;
         private final SnippetInfo verifyHeap;
         private final SnippetInfo threadBeingInitializedCheck;
+        private final SnippetInfo klassFullyInitializedCheck;
 
         @SuppressWarnings("this-escape")
         public Templates(HotSpotAllocationSnippets receiver, OptionValues options, SnippetCounter.Group.Factory groupFactory, HotSpotProviders providers,
@@ -623,6 +632,12 @@ public Templates(HotSpotAllocationSnippets receiver, OptionValues options, Snipp
                             receiver,
                             CLASS_INIT_STATE_LOCATION,
                             CLASS_INIT_THREAD_LOCATION);
+            klassFullyInitializedCheck = snippet(providers,
+                            HotSpotAllocationSnippets.class,
+                            "klassFullyInitializedCheck",
+                            MARK_WORD_LOCATION,
+                            HUB_WRITE_LOCATION,
+                            CLASS_INIT_STATE_LOCATION);
         }
 
         private HotSpotAllocationProfilingData getProfilingData(OptionValues localOptions, String path, ResolvedJavaType type) {
@@ -899,6 +914,13 @@ public void lower(KlassBeingInitializedCheckNode node, LoweringTool tool) {
             template(tool, node, args).instantiate(tool.getMetaAccess(), node, DEFAULT_REPLACER, args);
         }
 
+        public void lower(KlassFullyInitializedCheckNode node, LoweringTool tool) {
+            Arguments args = new Arguments(klassFullyInitializedCheck, node.graph().getGuardsStage(), tool.getLoweringStage());
+            args.add("klass", node.getKlass());
+
+            template(tool, node, args).instantiate(tool.getMetaAccess(), node, DEFAULT_REPLACER, args);
+        }
+
         private static HotSpotResolvedObjectType lookupArrayClass(LoweringTool tool, JavaKind kind) {
             return HotSpotAllocationSnippets.lookupArrayClass(tool.getMetaAccess(), kind);
         }

diff --git a/...k.graal.compiler/src/jdk/graal/compiler/hotspot/replacements/HotSpotReplacementsUtil.java b/...k.graal.compiler/src/jdk/graal/compiler/hotspot/replacements/HotSpotReplacementsUtil.java
@@ -847,7 +847,7 @@ public static boolean isInstanceKlassFullyInitialized(KlassPointer hub) {
     }
 
     static byte readInstanceKlassInitState(KlassPointer hub) {
-        return hub.readByte(instanceKlassInitStateOffset(INJECTED_VMCONFIG), CLASS_INIT_STATE_LOCATION);
+        return hub.readByteVolatile(instanceKlassInitStateOffset(INJECTED_VMCONFIG), CLASS_INIT_STATE_LOCATION);
     }
 
     static Word readInstanceKlassInitThread(KlassPointer hub) {

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/word/MetaspacePointer.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/word/MetaspacePointer.java
@@ -27,15 +27,16 @@
 import static jdk.graal.compiler.hotspot.word.HotSpotOperation.HotspotOpcode.FROM_POINTER;
 import static jdk.graal.compiler.hotspot.word.HotSpotOperation.HotspotOpcode.IS_NULL;
 
-import jdk.graal.compiler.core.common.memory.BarrierType;
-import jdk.graal.compiler.word.Word;
-import jdk.graal.compiler.word.Word.Opcode;
-import jdk.graal.compiler.word.Word.Operation;
 import org.graalvm.word.LocationIdentity;
 import org.graalvm.word.SignedWord;
 import org.graalvm.word.UnsignedWord;
 import org.graalvm.word.WordBase;
 
+import jdk.graal.compiler.core.common.memory.BarrierType;
+import jdk.graal.compiler.word.Word;
+import jdk.graal.compiler.word.Word.Opcode;
+import jdk.graal.compiler.word.Word.Operation;
+
 /**
  * Marker type for a metaspace pointer.
  */
@@ -281,6 +282,16 @@ public abstract class MetaspacePointer {
     @Operation(opcode = Opcode.READ_POINTER)
     public abstract Object readObject(int offset, LocationIdentity locationIdentity);
 
+    /**
+     * Reads the memory at address {@code (this + offset)} in accordance with volatile semantics.
+     * Both the base address and offset are in bytes.
+     *
+     * @param offset the signed offset for the memory access
+     * @return the result of the memory access
+     */
+    @Operation(opcode = Opcode.READ_POINTER_VOLATILE)
+    public abstract byte readByteVolatile(int offset, LocationIdentity locationIdentity);
+
     /**
      * Writes the memory at address {@code (this + offset)}. Both the base address and offset are in
      * bytes.

diff --git a/...iler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/DynamicNewInstanceNode.java b/...iler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/DynamicNewInstanceNode.java
@@ -35,11 +35,9 @@
 import jdk.graal.compiler.nodeinfo.NodeInfo;
 import jdk.graal.compiler.nodes.NodeView;
 import jdk.graal.compiler.nodes.ValueNode;
-import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderContext;
 import jdk.graal.compiler.nodes.spi.Canonicalizable;
 import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
 import jdk.graal.compiler.nodes.spi.CoreProviders;
-import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.MetaAccessProvider;
 import jdk.vm.ci.meta.ResolvedJavaType;
 
@@ -49,17 +47,7 @@ public final class DynamicNewInstanceNode extends AbstractNewObjectNode implemen
 
     @Input ValueNode clazz;
 
-    public static void createAndPush(GraphBuilderContext b, ValueNode clazz, boolean validateClass) {
-        ResolvedJavaType constantType = tryConvertToNonDynamic(clazz, b);
-        if (constantType != null) {
-            b.addPush(JavaKind.Object, new NewInstanceNode(constantType, true));
-        } else {
-            ValueNode clazzLegal = validateClass ? b.add(new ValidateNewInstanceClassNode(clazz)) : clazz;
-            b.addPush(JavaKind.Object, new DynamicNewInstanceNode(clazzLegal, true));
-        }
-    }
-
-    protected DynamicNewInstanceNode(ValueNode clazz, boolean fillContents) {
+    public DynamicNewInstanceNode(ValueNode clazz, boolean fillContents) {
         super(TYPE, StampFactory.objectNonNull(), fillContents, null);
         this.clazz = clazz;
         assert ((ObjectStamp) clazz.stamp(NodeView.DEFAULT)).nonNull();