Fix a fuzz bug with incremental unreachability in OptimizeInstructions (#5237)

OptimizeInstructions in rare cases can add unreachability. We propagate it out at
the end all at once. The fuzzer was smart enough to find a very special combination
of code + passes that can hit an issue, see the testcase.

As mentioned in the TODO, we should perhaps avoid adding unreachability in
OptimizeInstructions at all. If this happens again that might be worth the effort. But
also checking the type of the child as in this PR doesn't add much complexity in the
code.

2 files changed, 50 insertions, 1 deletions

diff --git a/src/passes/OptimizeInstructions.cpp b/src/passes/OptimizeInstructions.cpp
index 3de7b6519..f96d4d30a 100644
--- a/src/passes/OptimizeInstructions.cpp
+++ b/src/passes/OptimizeInstructions.cpp
@@ -1769,7 +1769,13 @@ struct OptimizeInstructions
   }
 
   void visitRefCast(RefCast* curr) {
-    if (curr->type == Type::unreachable) {
+    // Note we must check the ref's type here and not our own, since we only
+    // refinalize at the end, which means our type may not have been updated yet
+    // after a change in the child.
+    // TODO: we could update unreachability up the stack perhaps, or just move
+    //       all patterns that can add unreachability to a pass that does so
+    //       already like vacuum or dce.
+    if (curr->ref->type == Type::unreachable) {
       return;
     }
 
diff --git a/test/lit/passes/inlining_vacuum_optimize-instructions.wast b/test/lit/passes/inlining_vacuum_optimize-instructions.wast
new file mode 100644
index 000000000..839a5887e
--- /dev/null
+++ b/test/lit/passes/inlining_vacuum_optimize-instructions.wast
@@ -0,0 +1,43 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
+
+;; RUN: wasm-opt %s --inlining --vacuum --optimize-instructions -all -S -o - | filecheck %s
+
+;; Check for a specific bug involving inlining first turning a struct.get's
+;; input into a null, then vacuum gets rid of intervening blocks, and then
+;; optimize-instructions runs into the following situation: first it turns the
+;; struct.get of a null into an unreachable, then it makes a note to itself to
+;; refinalize at the end, but before the end it tries to optimize the ref.cast.
+;; The ref.cast's type has not been updated to unreachable yet, but its ref has,
+;; which is temporarily inconsistent. We must be careful to avoid confusion
+;; there.
+(module
+ ;; CHECK:      (type $B (struct ))
+
+ ;; CHECK:      (type $ref?|$A|_=>_none (func (param (ref null $A))))
+
+ ;; CHECK:      (type $A (struct (field (ref null $B))))
+ (type $A (struct_subtype (field (ref null $B)) data))
+ (type $B (struct_subtype  data))
+ ;; CHECK:      (func $target (param $0 (ref null $A))
+ ;; CHECK-NEXT:  (drop
+ ;; CHECK-NEXT:   (ref.cast_static $B
+ ;; CHECK-NEXT:    (unreachable)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT:  (unreachable)
+ ;; CHECK-NEXT: )
+ (func $target (param $0 (ref null $A))
+  (drop
+   (ref.cast_static $B
+    (struct.get $A 0
+     (call $get-null)
+    )
+   )
+  )
+  (unreachable)
+ )
+ (func $get-null (result (ref null $A))
+  (ref.null none)
+ )
+)
+