[GR-69630] Fix canonicalization of always-null guarded values.

PullRequest: graal/22174

Author: rmosaner

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/CanonicalizerPhase.java

@@ -38,6 +38,7 @@
 import org.graalvm.collections.EconomicSet;
 import org.graalvm.collections.Pair;
 
+import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
 import jdk.graal.compiler.core.common.type.Stamp;
 import jdk.graal.compiler.debug.Assertions;
 import jdk.graal.compiler.debug.CounterKey;
@@ -77,6 +78,7 @@
 import jdk.graal.compiler.nodes.calc.ConditionalNode;
 import jdk.graal.compiler.nodes.calc.FloatingNode;
 import jdk.graal.compiler.nodes.cfg.ControlFlowGraph;
+import jdk.graal.compiler.nodes.extended.GuardedNode;
 import jdk.graal.compiler.nodes.extended.GuardingNode;
 import jdk.graal.compiler.nodes.loop.InductionVariable;
 import jdk.graal.compiler.nodes.spi.Canonicalizable;
@@ -615,11 +617,16 @@ private boolean processNode(Node node, Tool tool, NodeBitMap singleShotList) {
         if (tryCanonicalize(node, nodeClass, tool)) {
             return true;
         }
-        if (node instanceof ValueNode) {
-            ValueNode valueNode = (ValueNode) node;
+        if (node instanceof ValueNode valueNode) {
             boolean improvedStamp = tryInferStamp(valueNode, tool);
             Constant constant = valueNode.stamp(NodeView.DEFAULT).asConstant();
-            if (constant != null && !(node instanceof ConstantNode)) {
+            if (constant != null && !(node instanceof ConstantNode) && !isAnchoredNullConstant(valueNode)) {
+                /*
+                 * Do not fold always-null Pis to null constants. Doing so could lead to dependent
+                 * reads floating above safety checks, potentially causing runtime segfaults. By
+                 * contrast, folding integer constants that could cause out-of-bounds reads is safe
+                 * as JVMCI detects these and fails gracefully.
+                 */
                 ConstantNode stampConstant = ConstantNode.forConstant(valueNode.stamp(NodeView.DEFAULT), constant, tool.context.getMetaAccess(), graph);
                 valueNode.replaceAtUsages(stampConstant, InputType.Value);
                 GraphUtil.tryKillUnused(valueNode);
@@ -640,6 +647,16 @@ private boolean processNode(Node node, Tool tool, NodeBitMap singleShotList) {
         return false;
     }
 
+    /**
+     * Tests if {@code node} is an always-null value that is anchored.
+     */
+    private static boolean isAnchoredNullConstant(ValueNode node) {
+        if (node instanceof GuardedNode guarded && guarded.getGuard() != null) {
+            return node.stamp(NodeView.DEFAULT).isObjectStamp() && ((AbstractObjectStamp) node.stamp(NodeView.DEFAULT)).alwaysNull();
+        }
+        return false;
+    }
+
     public static boolean gvn(Node node, NodeClass<?> nodeClass) {
         if (nodeClass.valueNumberable()) {
             Node newNode = node.graph().findDuplicate(node);

web-image/src/com.oracle.svm.hosted.webimage/src/com/oracle/svm/hosted/webimage/wasmgc/codegen/WebImageWasmGCNodeLowerer.java

@@ -79,6 +79,7 @@
 import com.oracle.svm.webimage.wasmgc.WasmGCJSConversion;
 
 import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
+import jdk.graal.compiler.core.common.type.AbstractPointerStamp;
 import jdk.graal.compiler.core.common.type.PrimitiveStamp;
 import jdk.graal.compiler.core.common.type.Stamp;
 import jdk.graal.compiler.core.common.type.TypeReference;
@@ -242,6 +243,37 @@ protected Instruction lowerReturn(ReturnNode returnNode) {
         return new Instruction.Return(result);
     }
 
+    @Override
+    protected Instruction lowerExpression(ValueNode n, WasmIRWalker.Requirements reqs) {
+        Instruction inst = super.lowerExpression(n, reqs);
+
+        /*
+         * Produce a constant null value for always-null stamps.
+         *
+         * Always null stamps are a bit of a special case because they often don't have concrete
+         * type information, which is required in WasmGC because even null-constant have a static
+         * type. For that reason, the actual instruction are emitted as a top-level instruction and
+         * dropped (since it may have a side effect) and we simply return a "ref.null none", which
+         * represents the bottom type and thus satisfies all subtype checks, at all usages.
+         */
+        if (n.stamp(NodeView.DEFAULT) instanceof AbstractPointerStamp pointerStamp && pointerStamp.alwaysNull()) {
+            if (!(inst instanceof Instruction.LocalGet)) {
+                /*
+                 * We can't just not emit the instruction, it may have side effects, so we just emit
+                 * it in the same block and ignore its output (which is guaranteed to be null).
+                 *
+                 * If the node's value was stored in a variable and would be just loaded here, we
+                 * can omit this, as there are no side-effects.
+                 */
+                masm.genInst(new Instruction.Drop(inst));
+            }
+
+            return new Instruction.RefNull(WasmRefType.NONE);
+        } else {
+            return inst;
+        }
+    }
+
     protected Instruction lowerUnwind(UnwindNode n) {
         return new Instruction.Call(masm().getKnownIds().throwTemplate.requestFunctionId(), lowerExpression(n.exception()));
     }

web-image/src/com.oracle.svm.hosted.webimage/src/com/oracle/svm/hosted/webimage/wasmgc/types/WasmRefType.java

@@ -55,6 +55,7 @@ public abstract class WasmRefType implements WasmValType {
     public static final WasmRefType ANYREF = Kind.ANY.nullable();
     public static final WasmRefType FUNCREF = Kind.FUNC.nullable();
     public static final WasmRefType EXTERNREF = Kind.EXTERN.nullable();
+    public static final WasmRefType NONE = Kind.NONE.nullable();
 
     /**
      * Enum of all {@link AbsHeap} types. Pulled up to the superclass for convenience.