[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.

3 files changed, 67 insertions, 21 deletions

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)
+     )
+   )
+  )
 )