ReFinalize fix (#1742)

Handle a corner case in ReFinalize, which incrementally re-types code after changes. The problem is that if we need to figure out the type of a block, we look to the last element flowing out, or to breaks with values. If there is no such last element, and the breaks are not taken - they have unreachable values - then they don't tell us the block's proper type. We asserted that in such a case the block still had a type, and didn't handle this.

To fix it, we could look on the parent to see what type would fit. However, it seem simpler to just remove untaken breaks/switches as part of ReFinalization - they carry no useful info anyhow. After removing them, if the block has no other signal of a concrete type, it can just be unreachable.

This bug existed for at least 1.5 years - I didn't look back further. I think it was noticed by the fuzzer now due to recent fuzzing improvements and optimizer improvements, as I just saw this bug found a second time.

Author: kripken

src/ir/utils.h

@@ -83,13 +83,23 @@ struct ExpressionAnalyzer {
   static uint32_t hash(Expression* curr);
 };
 
-// Re-Finalizes all node types
+// Re-Finalizes all node types. This can be run after code was modified in
+// various ways that require propagating types around, and it does such an
+// "incremental" update. This is done under the assumption that there is
+// a valid assignment of types to apply.
 // This removes "unnecessary' block/if/loop types, i.e., that are added
 // specifically, as in
 //  (block (result i32) (unreachable))
 // vs
 //  (block (unreachable))
 // This converts to the latter form.
+// This also removes un-taken branches that would be a problem for
+// refinalization: if a block has been marked unreachable, and has
+// branches to it with values of type unreachable, then we don't
+// know the type for the block: it can't be none since the breaks
+// exist, but the breaks don't declare the type, rather everything
+// depends on the block. To avoid looking at the parent or something
+// else, just remove such un-taken branches.
 struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<ReFinalize>>> {
   bool isFunctionParallel() override { return true; }
 
@@ -108,7 +118,6 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<R
       return;
     }
     // do this quickly, without any validation
-    auto old = curr->type;
     // last element determines type
     curr->type = curr->list.back()->type;
     // if concrete, it doesn't matter if we have an unreachable child, and we
@@ -121,14 +130,8 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<R
       if (iter != breakValues.end()) {
         // there is a break to here
         auto type = iter->second;
-        if (type == unreachable) {
-          // all we have are breaks with values of type unreachable, and no
-          // concrete fallthrough either. we must have had an existing type, then
-          curr->type = old;
-          assert(isConcreteType(curr->type));
-        } else {
-          curr->type = type;
-        }
+        assert(type != unreachable); // we would have removed such branches
+        curr->type = type;
         return;
       }
     }
@@ -147,15 +150,24 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<R
   void visitLoop(Loop* curr) { curr->finalize(); }
   void visitBreak(Break* curr) {
     curr->finalize();
-    updateBreakValueType(curr->name, getValueType(curr->value));
+    auto valueType = getValueType(curr->value);
+    if (valueType == unreachable) {
+      replaceUntaken(curr->value, curr->condition);
+    } else {
+      updateBreakValueType(curr->name, valueType);
+    }
   }
   void visitSwitch(Switch* curr) {
     curr->finalize();
     auto valueType = getValueType(curr->value);
-    for (auto target : curr->targets) {
-      updateBreakValueType(target, valueType);
+    if (valueType == unreachable) {
+      replaceUntaken(curr->value, curr->condition);
+    } else {
+      for (auto target : curr->targets) {
+        updateBreakValueType(target, valueType);
+      }
+      updateBreakValueType(curr->default_, valueType);
     }
-    updateBreakValueType(curr->default_, valueType);
   }
   void visitCall(Call* curr) { curr->finalize(); }
   void visitCallIndirect(CallIndirect* curr) { curr->finalize(); }
@@ -195,6 +207,7 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<R
   void visitMemory(Memory* curr) { WASM_UNREACHABLE(); }
   void visitModule(Module* curr) { WASM_UNREACHABLE(); }
 
+private:
   Type getValueType(Expression* value) {
     return value ? value->type : none;
   }
@@ -204,6 +217,31 @@ struct ReFinalize : public WalkerPass<PostWalker<ReFinalize, OverriddenVisitor<R
       breakValues[name] = type;
     }
   }
+
+  // Replace an untaken branch/switch with an unreachable value.
+  // A condition may also exist and may or may not be unreachable.
+  void replaceUntaken(Expression* value, Expression* condition) {
+    assert(value->type == unreachable);
+    auto* replacement = value;
+    if (condition) {
+      Builder builder(*getModule());
+      // Even if we have
+      //  (block
+      //   (unreachable)
+      //   (i32.const 1)
+      //  )
+      // we want the block type to be unreachable. That is valid as
+      // the value is unreachable, and necessary since the type of
+      // the condition did not have an impact before (the break/switch
+      // type was unreachable), and might not fit in.
+      if (isConcreteType(condition->type)) {
+        condition = builder.makeDrop(condition);
+      }
+      replacement = builder.makeSequence(value, condition);
+      assert(replacement->type);
+    }
+    replaceCurrent(replacement);
+  }
 };
 
 // Re-finalize a single node. This is slow, if you want to refinalize

test/passes/code-folding.txt

@@ -109,8 +109,8 @@
  (func $leave-inner-block-type (; 6 ;) (type $1)
   (block $label$1
    (drop
-    (block $label$2 (result i32)
-     (br_if $label$2
+    (block $label$2
+     (block
       (unreachable)
       (unreachable)
      )

test/passes/precompute.txt

@@ -162,7 +162,7 @@
  (func $refinalize-two-breaks-one-unreachable (; 6 ;) (type $2)
   (drop
    (block $label$0 (result i64)
-    (br_if $label$0
+    (block
      (select
       (i64.const 1)
       (block $block
@@ -175,17 +175,21 @@
       )
       (i32.const 0)
      )
-     (i32.const 1)
+     (drop
+      (i32.const 1)
+     )
     )
    )
   )
  )
  (func $one-break-value-and-it-is-unreachable (; 7 ;) (type $3) (result f64)
   (local $var$0 i32)
-  (block $label$6 (result f64)
-   (br_if $label$6
+  (block $label$6
+   (block
     (unreachable)
-    (i32.const 0)
+    (drop
+     (i32.const 0)
+    )
    )
   )
  )

test/passes/remove-unused-brs.txt

@@ -1922,4 +1922,28 @@
    )
   )
  )
+ (func $fuzz-block-unreachable-brs-with-values (; 80 ;) (type $2) (result i32)
+  (local $0 i32)
+  (loop $label$1
+   (block $label$2
+    (br_if $label$1
+     (i32.eqz
+      (get_local $0)
+     )
+    )
+    (tee_local $0
+     (loop $label$5
+      (br_if $label$5
+       (block
+        (unreachable)
+        (drop
+         (i32.const 0)
+        )
+       )
+      )
+     )
+    )
+   )
+  )
+ )
 )

test/passes/remove-unused-brs.wast

@@ -1540,5 +1540,26 @@
     (br $label$1)
    )
   )
+  (func $fuzz-block-unreachable-brs-with-values (result i32)
+   (local $0 i32)
+   (loop $label$1 (result i32)
+    (block $label$2 (result i32)
+     (if
+      (get_local $0)
+      (set_local $0
+       (loop $label$5
+        (br_if $label$5
+         (br_if $label$2
+          (unreachable)
+          (i32.const 0)
+         )
+        )
+       )
+      )
+     )
+     (br $label$1)
+    )
+   )
+  )
 )
 

test/wasm2js/br.2asm.js

@@ -510,7 +510,7 @@ function asmFunc(global, env, buffer) {
  }
 
  function $54() {
-  var $0 = 0, $1_1 = 0;
+  var $0 = 0;
   block : {
    block0 : {
     $0 = 8;
@@ -523,10 +523,8 @@ function asmFunc(global, env, buffer) {
  function $55() {
   var $0 = 0, $1_1 = 0;
   block : {
-   block1 : {
-    $0 = 8;
-    break block;
-   };
+   $0 = 8;
+   break block;
   };
   return 1 + $0 | 0 | 0;
  }
@@ -541,12 +539,10 @@ function asmFunc(global, env, buffer) {
  }
 
  function $57() {
-  var $0 = 0, $1_1 = 0;
+  var $0 = 0;
   block : {
-   block2 : {
-    $0 = 8;
-    break block;
-   };
+   $0 = 8;
+   break block;
   };
   return 1 + $0 | 0 | 0;
  }