[Wasm GC] Fix invalid intermediate IR in OptimizeInstructions (#4169)

We added an optional ReFinalize in OptimizeInstructions at some point,
but that is not valid: The ReFinalize only updates types when all other
works is done, but the pass works incrementally. The bug the fuzzer found
is that a child is changed to be unreachable, and then the parent is
optimized before finalize() is called on it, which led to an assertion being
hit (as the child was unreachable but not the parent, which should also
be).

To fix this, do not change types in this pass. Emit an extra block with a
declared type when necessary. Other passes can remove the extra block.

5 files changed, 78 insertions, 33 deletions

diff --git a/scripts/fuzz_opt.py b/scripts/fuzz_opt.py
index abc6225ee..d4e9ccc4d 100755
--- a/scripts/fuzz_opt.py
+++ b/scripts/fuzz_opt.py
@@ -176,6 +176,8 @@ IMPORTANT_INITIAL_CONTENTS = [
     os.path.join('lit', 'passes', 'optimize-instructions-bulk-memory.wast'),
     os.path.join('lit', 'passes', 'optimize-instructions-ignore-traps.wast'),
     os.path.join('lit', 'passes', 'optimize-instructions-gc.wast'),
+    os.path.join('lit', 'passes', 'optimize-instructions-gc-iit.wast'),
+    os.path.join('lit', 'passes', 'optimize-instructions-call_ref.wast'),
     os.path.join('lit', 'passes', 'inlining_splitting.wast'),
 ]
 IMPORTANT_INITIAL_CONTENTS = [os.path.join(shared.get_test_dir('.'), t) for t in IMPORTANT_INITIAL_CONTENTS]
diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index d18990e8e..387d76b37 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -207,8 +207,6 @@ struct OptimizeInstructions
 
   bool fastMath;
 
-  bool refinalize = false;
-
   void doWalkFunction(Function* func) {
     fastMath = getPassOptions().fastMath;
 
@@ -222,11 +220,6 @@ struct OptimizeInstructions
     // Main walk.
     super::doWalkFunction(func);
 
-    // If we need to update parent types, do so.
-    if (refinalize) {
-      ReFinalize().walkFunctionInModule(func, getModule());
-    }
-
     // Final optimizations.
     {
       FinalOptimizer optimizer(getPassOptions());
@@ -1383,11 +1376,12 @@ struct OptimizeInstructions
         // This cast cannot succeed. If the input is not a null, it will
         // definitely trap.
         if (fallthrough->type.isNonNullable()) {
-          // Our type will now be unreachable; update the parents.
-          refinalize = true;
+          // Make sure to emit a block with the same type as us; leave updating
+          // types for other passes.
           replaceCurrent(builder.makeBlock({builder.makeDrop(curr->ref),
                                             builder.makeDrop(curr->rtt),
-                                            builder.makeUnreachable()}));
+                                            builder.makeUnreachable()},
+                                           curr->type));
           return;
         }
         // Otherwise, we are not sure what it is, and need to wait for runtime
@@ -1575,8 +1569,11 @@ struct OptimizeInstructions
       // drop, which is no worse, and the value and the drop can be optimized
       // out later if the value has no side effects.
       Builder builder(*getModule());
-      replaceCurrent(builder.makeSequence(builder.makeDrop(curr->value),
-                                          builder.makeUnreachable()));
+      // Make sure to emit a block with the same type as us; leave updating
+      // types for other passes.
+      replaceCurrent(builder.makeBlock(
+        {builder.makeDrop(curr->value), builder.makeUnreachable()},
+        curr->type));
       return;
     }
 
diff --git a/test/lit/passes/optimize-instructions-call_ref.wast b/test/lit/passes/optimize-instructions-call_ref.wast
index b3b781b1d..320237d1e 100644
--- a/test/lit/passes/optimize-instructions-call_ref.wast
+++ b/test/lit/passes/optimize-instructions-call_ref.wast
@@ -5,12 +5,12 @@
 ;; remove-unused-names is to allow fallthrough computations to work on blocks
 
 (module
- ;; CHECK:      (type $i32_i32_=>_none (func (param i32 i32)))
- (type $i32_i32_=>_none (func (param i32 i32)))
-
  ;; CHECK:      (type $none_=>_i32 (func (result i32)))
  (type $none_=>_i32 (func (result i32)))
 
+ ;; CHECK:      (type $i32_i32_=>_none (func (param i32 i32)))
+ (type $i32_i32_=>_none (func (param i32 i32)))
+
  ;; CHECK:      (type $none_=>_none (func))
  (type $none_=>_none (func))
 
@@ -143,7 +143,7 @@
 
  ;; CHECK:      (func $fallthrough-bad-type (result i32)
  ;; CHECK-NEXT:  (call_ref
- ;; CHECK-NEXT:   (block
+ ;; CHECK-NEXT:   (block (result (ref $none_=>_i32))
  ;; CHECK-NEXT:    (drop
  ;; CHECK-NEXT:     (ref.func $return-nothing)
  ;; CHECK-NEXT:    )
diff --git a/test/lit/passes/optimize-instructions-gc-iit.wast b/test/lit/passes/optimize-instructions-gc-iit.wast
index b99042643..d6bb9eb31 100644
--- a/test/lit/passes/optimize-instructions-gc-iit.wast
+++ b/test/lit/passes/optimize-instructions-gc-iit.wast
@@ -57,7 +57,7 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result (ref $other))
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $child)
   ;; CHECK-NEXT:    )
@@ -92,7 +92,7 @@
   ;; NOMNL-NEXT:   )
   ;; NOMNL-NEXT:  )
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result (ref $other))
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $child)
   ;; NOMNL-NEXT:    )
@@ -127,7 +127,7 @@
   ;; NOMNL-TNH-NEXT:   )
   ;; NOMNL-TNH-NEXT:  )
   ;; NOMNL-TNH-NEXT:  (drop
-  ;; NOMNL-TNH-NEXT:   (block
+  ;; NOMNL-TNH-NEXT:   (block (result (ref $other))
   ;; NOMNL-TNH-NEXT:    (drop
   ;; NOMNL-TNH-NEXT:     (local.get $child)
   ;; NOMNL-TNH-NEXT:    )
diff --git a/test/lit/passes/optimize-instructions-gc.wast b/test/lit/passes/optimize-instructions-gc.wast
index 3f12f58e9..993eb222c 100644
--- a/test/lit/passes/optimize-instructions-gc.wast
+++ b/test/lit/passes/optimize-instructions-gc.wast
@@ -569,7 +569,7 @@
   ;; data, etc.), so we know we will trap
   ;; CHECK:      (func $unneeded_as_bad_kinds (param $func funcref) (param $data (ref null data)) (param $i31 (ref null i31))
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result (ref func))
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $data)
   ;; CHECK-NEXT:    )
@@ -577,7 +577,7 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result dataref)
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $i31)
   ;; CHECK-NEXT:    )
@@ -585,7 +585,7 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result i31ref)
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $func)
   ;; CHECK-NEXT:    )
@@ -593,7 +593,7 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result (ref func))
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $data)
   ;; CHECK-NEXT:    )
@@ -601,7 +601,7 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result dataref)
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $i31)
   ;; CHECK-NEXT:    )
@@ -609,7 +609,7 @@
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result i31ref)
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $func)
   ;; CHECK-NEXT:    )
@@ -619,7 +619,7 @@
   ;; CHECK-NEXT: )
   ;; NOMNL:      (func $unneeded_as_bad_kinds (param $func funcref) (param $data (ref null data)) (param $i31 (ref null i31))
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result (ref func))
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $data)
   ;; NOMNL-NEXT:    )
@@ -627,7 +627,7 @@
   ;; NOMNL-NEXT:   )
   ;; NOMNL-NEXT:  )
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result dataref)
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $i31)
   ;; NOMNL-NEXT:    )
@@ -635,7 +635,7 @@
   ;; NOMNL-NEXT:   )
   ;; NOMNL-NEXT:  )
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result i31ref)
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $func)
   ;; NOMNL-NEXT:    )
@@ -643,7 +643,7 @@
   ;; NOMNL-NEXT:   )
   ;; NOMNL-NEXT:  )
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result (ref func))
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $data)
   ;; NOMNL-NEXT:    )
@@ -651,7 +651,7 @@
   ;; NOMNL-NEXT:   )
   ;; NOMNL-NEXT:  )
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result dataref)
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $i31)
   ;; NOMNL-NEXT:    )
@@ -659,7 +659,7 @@
   ;; NOMNL-NEXT:   )
   ;; NOMNL-NEXT:  )
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result i31ref)
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $func)
   ;; NOMNL-NEXT:    )
@@ -1668,7 +1668,7 @@
 
   ;; CHECK:      (func $incompatible-cast-of-non-null (param $struct (ref $struct))
   ;; CHECK-NEXT:  (drop
-  ;; CHECK-NEXT:   (block
+  ;; CHECK-NEXT:   (block (result (ref $array))
   ;; CHECK-NEXT:    (drop
   ;; CHECK-NEXT:     (local.get $struct)
   ;; CHECK-NEXT:    )
@@ -1681,7 +1681,7 @@
   ;; CHECK-NEXT: )
   ;; NOMNL:      (func $incompatible-cast-of-non-null (param $struct (ref $struct))
   ;; NOMNL-NEXT:  (drop
-  ;; NOMNL-NEXT:   (block
+  ;; NOMNL-NEXT:   (block (result (ref $array))
   ;; NOMNL-NEXT:    (drop
   ;; NOMNL-NEXT:     (local.get $struct)
   ;; NOMNL-NEXT:    )
@@ -1908,4 +1908,50 @@
       )
     )
   )
+
+  ;; CHECK:      (func $consecutive-opts-with-unreachable (param $func funcref)
+  ;; CHECK-NEXT:  (drop
+  ;; CHECK-NEXT:   (ref.cast
+  ;; CHECK-NEXT:    (block (result dataref)
+  ;; CHECK-NEXT:     (drop
+  ;; CHECK-NEXT:      (local.get $func)
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (unreachable)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (rtt.canon $struct)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  ;; NOMNL:      (func $consecutive-opts-with-unreachable (param $func funcref)
+  ;; NOMNL-NEXT:  (drop
+  ;; NOMNL-NEXT:   (ref.cast
+  ;; NOMNL-NEXT:    (block (result dataref)
+  ;; NOMNL-NEXT:     (drop
+  ;; NOMNL-NEXT:      (local.get $func)
+  ;; NOMNL-NEXT:     )
+  ;; NOMNL-NEXT:     (unreachable)
+  ;; NOMNL-NEXT:    )
+  ;; NOMNL-NEXT:    (rtt.canon $struct)
+  ;; NOMNL-NEXT:   )
+  ;; NOMNL-NEXT:  )
+  ;; NOMNL-NEXT: )
+  (func $consecutive-opts-with-unreachable (param $func funcref)
+   (drop
+     (ref.cast
+       ;; Casting a funcref to data will definitely fail, so this will be
+       ;; replaced with an unreachable. But it should be enclosed in a block of
+       ;; the previous type, so that the outside ref.cast is not confused. This
+       ;; is a regression test for a bug where we replace this node with an
+       ;; unreachable one, but we left refinalize til the end of all the other
+       ;; opts - and that meant that we got to our parent, the ref.cast, with
+       ;; one unreachable child but before it itself was refinalized, so its
+       ;; type was *not* unreachable yet, which meant it saw inconsistent IR
+       ;; that then led to an assertion.
+       (ref.as_data
+         (local.get $func)
+       )
+       (rtt.canon $struct)
+     )
+   )
+  )
 )