Make InitMemoryVerificationPhase a proper VerifyPhase

oracle · Sep 6, 2024 · d630223 · d630223
1 parent d35ad51
commit d630223
Show file tree

Hide file tree

Showing 6 changed files with 39 additions and 41 deletions.
diff --git a/.../jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/InitMemoryVerificationTest.java b/.../jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/InitMemoryVerificationTest.java
@@ -26,6 +26,7 @@
 
 import java.util.Optional;
 
+import org.junit.Assert;
 import org.junit.Test;
 
 import jdk.graal.compiler.debug.DebugContext;
@@ -37,6 +38,7 @@
 import jdk.graal.compiler.nodes.extended.PublishWritesNode;
 import jdk.graal.compiler.nodes.util.GraphUtil;
 import jdk.graal.compiler.phases.BasePhase;
+import jdk.graal.compiler.phases.VerifyPhase;
 import jdk.graal.compiler.phases.common.InitMemoryVerificationPhase;
 import jdk.graal.compiler.phases.tiers.MidTierContext;
 import jdk.graal.compiler.phases.tiers.Suites;
@@ -99,10 +101,8 @@ protected void run(StructuredGraph graph, MidTierContext context) {
         try (DebugContext.Scope scope = getDebugContext().disable()) {
             new InitMemoryVerificationPhase().apply(g, getDefaultLowTierContext());
             throw new GraalError("Should fail init memory verification");
-        } catch (AssertionError e) {
-            // expected
-        } catch (GraalError e) {
-            throw e;
+        } catch (VerifyPhase.VerificationError e) {
+            Assert.assertTrue(e.getMessage().contains("unpublished allocations"));
         }
     }
 
@@ -129,10 +129,8 @@ protected void run(StructuredGraph graph, MidTierContext context) {
         try (DebugContext.Scope scope = getDebugContext().disable()) {
             new InitMemoryVerificationPhase().apply(g, getDefaultLowTierContext());
             throw new GraalError("Should fail init memory verification");
-        } catch (AssertionError e) {
-            // expected
-        } catch (GraalError e) {
-            throw e;
+        } catch (VerifyPhase.VerificationError e) {
+            Assert.assertTrue(e.getMessage().contains("writes to init memory not guarded by an init barrier"));
         }
     }
 }
diff --git a/...c/jdk.graal.compiler/src/jdk/graal/compiler/core/phases/EconomyCompilerConfiguration.java b/...c/jdk.graal.compiler/src/jdk/graal/compiler/core/phases/EconomyCompilerConfiguration.java
@@ -39,7 +39,6 @@
 import jdk.graal.compiler.phases.tiers.HighTierContext;
 import jdk.graal.compiler.phases.tiers.LowTierContext;
 import jdk.graal.compiler.phases.tiers.MidTierContext;
-
 import jdk.vm.ci.code.Architecture;
 
 /**
@@ -60,7 +59,7 @@ public PhaseSuite<MidTierContext> createMidTier(OptionValues options) {
 
     @Override
     public PhaseSuite<LowTierContext> createLowTier(OptionValues options, Architecture arch) {
-        return new EconomyLowTier();
+        return new EconomyLowTier(options);
     }
 
     @Override

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/phases/EconomyLowTier.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/phases/EconomyLowTier.java
@@ -25,6 +25,8 @@
 package jdk.graal.compiler.core.phases;
 
 import jdk.graal.compiler.debug.Assertions;
+import jdk.graal.compiler.graph.Graph;
+import jdk.graal.compiler.options.OptionValues;
 import jdk.graal.compiler.phases.PlaceholderPhase;
 import jdk.graal.compiler.phases.common.AddressLoweringPhase;
 import jdk.graal.compiler.phases.common.BarrierSetVerificationPhase;
@@ -40,8 +42,8 @@
 public class EconomyLowTier extends BaseTier<LowTierContext> {
 
     @SuppressWarnings("this-escape")
-    public EconomyLowTier() {
-        if (Assertions.assertionsEnabled()) {
+    public EconomyLowTier(OptionValues options) {
+        if (Graph.Options.VerifyGraalGraphs.getValue(options)) {
             appendPhase(new InitMemoryVerificationPhase());
         }
         CanonicalizerPhase canonicalizer = CanonicalizerPhase.create();

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/phases/LowTier.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/phases/LowTier.java
@@ -27,7 +27,7 @@
 import static jdk.graal.compiler.phases.common.DeadCodeEliminationPhase.Optionality.Required;
 
 import jdk.graal.compiler.core.common.GraalOptions;
-import jdk.graal.compiler.debug.Assertions;
+import jdk.graal.compiler.graph.Graph;
 import jdk.graal.compiler.options.Option;
 import jdk.graal.compiler.options.OptionKey;
 import jdk.graal.compiler.options.OptionType;
@@ -74,7 +74,7 @@ public LowTier(OptionValues options) {
             appendPhase(new ProfileCompiledMethodsPhase());
         }
 
-        if (Assertions.assertionsEnabled()) {
+        if (Graph.Options.VerifyGraalGraphs.getValue(options)) {
             appendPhase(new InitMemoryVerificationPhase());
         }
 

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/extended/MembarNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/extended/MembarNode.java
@@ -104,6 +104,14 @@ public enum FenceKind {
 
         private final int barriers;
 
+        /**
+         * @return true iff this fence should be used at the end of object initialization to make a
+         *         newly allocated object visible to a different thread.
+         */
+        public boolean isInit() {
+            return this == ALLOCATION_INIT || this == CONSTRUCTOR_FREEZE;
+        }
+
         FenceKind(int barriers) {
             this.barriers = barriers;
         }

diff --git a/.../jdk.graal.compiler/src/jdk/graal/compiler/phases/common/InitMemoryVerificationPhase.java b/.../jdk.graal.compiler/src/jdk/graal/compiler/phases/common/InitMemoryVerificationPhase.java
@@ -31,7 +31,6 @@
 import org.graalvm.collections.EconomicSet;
 import org.graalvm.word.LocationIdentity;
 
-import jdk.graal.compiler.debug.Assertions;
 import jdk.graal.compiler.nodes.AbstractBeginNode;
 import jdk.graal.compiler.nodes.AbstractMergeNode;
 import jdk.graal.compiler.nodes.FixedNode;
@@ -50,28 +49,27 @@
 import jdk.graal.compiler.nodes.memory.MultiMemoryKill;
 import jdk.graal.compiler.nodes.memory.SingleMemoryKill;
 import jdk.graal.compiler.nodes.spi.CoreProviders;
-import jdk.graal.compiler.phases.BasePhase;
 import jdk.graal.compiler.phases.RecursivePhase;
+import jdk.graal.compiler.phases.VerifyPhase;
 import jdk.graal.compiler.phases.graph.ReentrantNodeIterator;
 import jdk.vm.ci.code.MemoryBarriers;
 
 /**
- * To maintain object safety while not interacting with the memory graph, operations on
- * {@link LocationIdentity#INIT_LOCATION INIT} memory must follow some rules, namely:
+ * Verifies the following two invariants related to {@link LocationIdentity#INIT_LOCATION INIT}
+ * memory in the graph:
  * <ul>
- * <li>All newly allocated objects are published by a {@link PublishWritesNode} <b>after</b> all
- * initializing writes have been performed.</li>
+ * <li>Newly allocated objects are published by a {@link PublishWritesNode} <b>after</b> all
+ * initializing writes have been performed. By having later uses of the allocated object depend on
+ * the {@link PublishWritesNode}, we prevent these uses from floating above the initializing writes.
+ * </li>
  * <li>A {@link MemoryBarriers#STORE_STORE STORE_STORE} barrier is emitted after all initializing
- * writes have been performed.</li>
- * <li>Later reads and writes of the allocated objects have a data dependence on the
- * {@link PublishWritesNode} instead of the raw allocation node.</li>
+ * writes have been performed. This ensures all allocation initializations before this fence have
+ * completed before the object can become visible to a different thread.</li>
  * </ul>
- * This phase verifies the first two properties. The third property is instead verified by
- * {@link PublishWritesNode#verify()}.
  * <p>
  * See {@link PublishWritesNode} for a description of init memory semantics in the Graal IR.
  */
-public class InitMemoryVerificationPhase extends BasePhase<CoreProviders> implements RecursivePhase {
+public class InitMemoryVerificationPhase extends VerifyPhase<CoreProviders> implements RecursivePhase {
     @Override
     public Optional<NotApplicable> notApplicableTo(GraphState graphState) {
         /*
@@ -107,28 +105,21 @@ private static boolean killsInitMemory(MultiMemoryKill kill) {
             return false;
         }
 
-        private static boolean isInitBarrier(MembarNode membar) {
-            if (!membar.getKilledLocationIdentity().isInit()) {
-                return false;
-            }
-            return membar.getFenceKind().equals(MembarNode.FenceKind.ALLOCATION_INIT) || membar.getFenceKind().equals(MembarNode.FenceKind.CONSTRUCTOR_FREEZE);
-        }
-
         @Override
         protected Integer processNode(FixedNode node, Integer kills) {
             int liveKills = kills;
             if (node instanceof MembarNode memBar) {
-                if (isInitBarrier(memBar)) {
+                if (memBar.getFenceKind().isInit()) {
                     liveKills = 0;
-                } else {
-                    assert liveKills == 0 : "Non-init memBar while there are live init writes: " + memBar;
+                } else if (liveKills > 0) {
+                    throw new VerificationError("%s is a non-init barrier, but there are %d live init writes", memBar, liveKills);
                 }
             } else if (MemoryKill.isSingleMemoryKill(node) && killsInitMemory(MemoryKill.asSingleMemoryKill(node))) {
                 liveKills++;
             } else if (MemoryKill.isMultiMemoryKill(node) && killsInitMemory(MemoryKill.asMultiMemoryKill(node))) {
                 liveKills++;
-            } else if (node instanceof ReturnNode) {
-                assert liveKills == 0 : String.format("%d unpublished writes at %s", liveKills, node);
+            } else if (node instanceof ReturnNode && liveKills > 0) {
+                throw new VerificationError("%d writes to init memory not guarded by an init barrier at node %s", liveKills, node);
             }
             return liveKills;
         }
@@ -183,8 +174,8 @@ protected EconomicSet<AbstractNewObjectNode> processNode(FixedNode node, Economi
                 processAlloc(allocation, unpublished);
             } else if (node instanceof PublishWritesNode publish) {
                 markPublished(publish.allocation(), unpublished);
-            } else if (node instanceof ReturnNode) {
-                assert unpublished.isEmpty() : Assertions.errorMessageContext("unpublished allocations", unpublished, "at node", node);
+            } else if (node instanceof ReturnNode && !unpublished.isEmpty()) {
+                throw new VerificationError("unpublished allocations at node %s: %s", unpublished, node);
             }
             return unpublished;
         }
@@ -211,7 +202,7 @@ protected EconomicMap<LoopExitNode, EconomicSet<AbstractNewObjectNode>> processL
     }
 
     @Override
-    protected void run(StructuredGraph graph, CoreProviders context) {
+    protected void verify(StructuredGraph graph, CoreProviders context) {
         ReentrantNodeIterator.apply(new InitBarrierVerificationClosure(), graph.start(), 0);
         ReentrantNodeIterator.apply(new AllocPublishVerificationClosure(), graph.start(), EconomicSet.create());
     }