[Wasm GC] Ignore unreachable sets for purposes of non-nullable locals

src/ir/LocalStructuralDominance.cpp

@@ -103,7 +103,13 @@ LocalStructuralDominance::LocalStructuralDominance(Function* func,
     }
 
     static void doLocalSet(Scanner* self, Expression** currp) {
-      auto index = (*currp)->cast<LocalSet>()->index;
+      auto* curr = *currp;
+      if (curr->type == Type::unreachable) {
+        // Unreachable sets are not emitted in the binary format, so we do not
+        // count them.
+        return;
+      }
+      auto index = curr->cast<LocalSet>()->index;
       if (!self->localsSet[index]) {
         // This local is now set until the end of this scope.
         self->localsSet[index] = true;

src/ir/ReFinalize.cpp

@@ -16,6 +16,7 @@
 
 #include "ir/branch-utils.h"
 #include "ir/find_all.h"
+#include "ir/type-updating.h"
 #include "ir/utils.h"
 
 namespace wasm {
@@ -96,7 +97,19 @@ void ReFinalize::visitSwitch(Switch* curr) {
 void ReFinalize::visitCall(Call* curr) { curr->finalize(); }
 void ReFinalize::visitCallIndirect(CallIndirect* curr) { curr->finalize(); }
 void ReFinalize::visitLocalGet(LocalGet* curr) { curr->finalize(); }
-void ReFinalize::visitLocalSet(LocalSet* curr) { curr->finalize(); }
+void ReFinalize::visitLocalSet(LocalSet* curr) {
+  auto oldType = curr->type;
+  curr->finalize();
+  if (curr->type == Type::unreachable && oldType != curr->type) {
+    // We changed the type of this set from reachable to unreachable. This can
+    // affect non-nullable local validation BLAH BLAH
+    auto localType = getFunction()->getLocalType(curr->index);
+    if (localType.isNonNullable()) {
+      // Perhaps just this local? But this is rare, so maybe not that bad.
+      needNonNullableLocalFixups = true;
+    }
+  }
+}
 void ReFinalize::visitGlobalGet(GlobalGet* curr) { curr->finalize(); }
 void ReFinalize::visitGlobalSet(GlobalSet* curr) { curr->finalize(); }
 void ReFinalize::visitLoad(Load* curr) { curr->finalize(); }
@@ -191,6 +204,12 @@ void ReFinalize::visitStringSliceIter(StringSliceIter* curr) {
   curr->finalize();
 }
 
+void ReFinalize::visitFunction(Function* curr) {
+  if (needNonNullableLocalFixups) {
+    TypeUpdating::handleNonDefaultableLocals(curr, *getModule());
+  }
+}
+
 void ReFinalize::visitExport(Export* curr) { WASM_UNREACHABLE("unimp"); }
 void ReFinalize::visitGlobal(Global* curr) { WASM_UNREACHABLE("unimp"); }
 void ReFinalize::visitTable(Table* curr) { WASM_UNREACHABLE("unimp"); }

src/ir/utils.h

@@ -133,11 +133,17 @@ struct ReFinalize
   // bulk, tracking break value types so we just do a linear pass.
   std::unordered_map<Name, std::unordered_set<Type>> breakTypes;
 
+  // In certain cases we may affect the validation of non-nullable locals. See
+  // ReFinalize::visitLocalSet().
+  bool needNonNullableLocalFixups = false;
+
 #define DELEGATE(CLASS_TO_VISIT)                                               \
   void visit##CLASS_TO_VISIT(CLASS_TO_VISIT* curr);
 
 #include "wasm-delegations.def"
 
+  void visitFunction(Function* curr);
+
   void visitExport(Export* curr);
   void visitGlobal(Global* curr);
   void visitTable(Table* curr);

src/passes/LocalSubtyping.cpp

@@ -75,32 +75,6 @@ struct LocalSubtyping : public WalkerPass<PostWalker<LocalSubtyping>> {
       }
     }
 
-    // Find which vars can be non-nullable.
-    std::unordered_set<Index> cannotBeNonNullable;
-
-    if (getModule()->features.hasGCNNLocals()) {
-      // If the feature is enabled then the only constraint is being able to
-      // read the default value - if it is readable, the local cannot become
-      // non-nullable.
-      for (auto& [get, sets] : localGraph.getSetses) {
-        auto index = get->index;
-        if (func->isVar(index) &&
-            std::any_of(sets.begin(), sets.end(), [&](LocalSet* set) {
-              return set == nullptr;
-            })) {
-          cannotBeNonNullable.insert(index);
-        }
-      }
-    } else {
-      // Without GCNNLocals, validation rules follow the spec rules: all gets
-      // must be dominated structurally by sets, for the local to be non-
-      // nullable.
-      LocalStructuralDominance info(func, *getModule());
-      for (auto index : info.nonDominatingIndices) {
-        cannotBeNonNullable.insert(index);
-      }
-    }
-
     auto varBase = func->getVarIndexBase();
 
     // Keep iterating while we find things to change. There can be chains like
@@ -125,6 +99,33 @@ struct LocalSubtyping : public WalkerPass<PostWalker<LocalSubtyping>> {
       // the next step for knowing if there is more work to do.
       ReFinalize().walkFunctionInModule(func, getModule());
 
+      // Find which vars can be non-nullable. Note that we must do this after
+      // refinalizing, as that operation can alter the types of locals.
+      std::unordered_set<Index> cannotBeNonNullable;
+
+      if (getModule()->features.hasGCNNLocals()) {
+        // If the feature is enabled then the only constraint is being able to
+        // read the default value - if it is readable, the local cannot become
+        // non-nullable.
+        for (auto& [get, sets] : localGraph.getSetses) {
+          auto index = get->index;
+          if (func->isVar(index) &&
+              std::any_of(sets.begin(), sets.end(), [&](LocalSet* set) {
+                return set == nullptr;
+              })) {
+            cannotBeNonNullable.insert(index);
+          }
+        }
+      } else {
+        // Without GCNNLocals, validation rules follow the spec rules: all gets
+        // must be dominated structurally by sets, for the local to be non-
+        // nullable.
+        LocalStructuralDominance info(func, *getModule());
+        for (auto index : info.nonDominatingIndices) {
+          cannotBeNonNullable.insert(index);
+        }
+      }
+
       // Second, find vars whose actual applied values allow a more specific
       // type.
 

test/lit/passes/memory-packing-gc.wast

@@ -174,3 +174,75 @@
     )
   )
 )
+
+(module
+  (type $[i32] (array i32))
+
+  ;; CHECK:      (type $none_=>_none (func))
+
+  ;; CHECK:      (memory $0 (shared 16 17))
+  (memory $0 (shared 16 17))
+  (data $0 (i32.const 37) "")
+
+  ;; CHECK:      (func $test (type $none_=>_none)
+  ;; CHECK-NEXT:  (local $temp anyref)
+  ;; CHECK-NEXT:  (block
+  ;; CHECK-NEXT:   (drop
+  ;; CHECK-NEXT:    (i32.const 0)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (drop
+  ;; CHECK-NEXT:    (i32.const 8)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (drop
+  ;; CHECK-NEXT:    (i32.const 5)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (unreachable)
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (local.tee $temp
+  ;; CHECK-NEXT:   (block ;; (replaces something unreachable we can't emit)
+  ;; CHECK-NEXT:    (drop
+  ;; CHECK-NEXT:     (if
+  ;; CHECK-NEXT:      (i32.const 1)
+  ;; CHECK-NEXT:      (unreachable)
+  ;; CHECK-NEXT:      (unreachable)
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (drop
+  ;; CHECK-NEXT:     (i32.const 1)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (unreachable)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (ref.as_non_null
+  ;; CHECK-NEXT:    (local.get $temp)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $test
+    (local $temp (ref any))
+    ;; The memory.init will trigger a ReFinalize. That will cause the if to
+    ;; become unreachable, after which the local.set is unreachable. We do not
+    ;; consider such sets to help non-nullable locals to validate (we do not emit
+    ;; unreachable code), so we need to fix up the non-nullable get afterwards
+    ;; by making it nullable, and adding a ref.as_non_null).
+    (memory.init $0
+      (i32.const 0)
+      (i32.const 8)
+      (i32.const 5)
+    )
+    (local.set $temp
+      (array.new $[i32]
+        (if (result i32)
+          (i32.const 1)
+          (unreachable)
+          (unreachable)
+        )
+        (i32.const 1)
+      )
+    )
+    (drop
+      (local.get $temp)
+    )
+  )
+)

test/lit/passes/precompute-gc.wast

@@ -1033,4 +1033,34 @@
    )
   )
  )
+
+ ;; CHECK:      (func $unreachable-set (type $none_=>_none)
+ ;; CHECK-NEXT:  (local $x funcref)
+ ;; CHECK-NEXT:  (local.tee $x
+ ;; CHECK-NEXT:   (block
+ ;; CHECK-NEXT:    (unreachable)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (ref.as_non_null
+ ;; CHECK-NEXT:    (local.get $x)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $unreachable-set
+  ;; An unreachable set is not helpful for validation of non-nullable locals
+  ;; since we won't emit it in the binary format. We ignore the set in that
+  ;; computation, and as a result we'll make the local nullable here after
+  ;; this pass runs and the set becomes unreachable (due to calling refinalize
+  ;; which propagates the unreachability).
+  (local $x (ref func))
+  (local.set $x
+   (block (result (ref func))
+    (unreachable)
+   )
+  )
+  (drop
+   (local.get $x)
+  )
+ )
 )