[Parser] Fix bug in unreachable fallback logic

When popping past an unreachable instruction would lead to popping from an empty
stack or popping an incorrect type, we need to avoid popping and produce new
`Unreachable` instructions instead to ensure we parse valid IR. The logic for
this was flawed and made the synthetic `Unreachable` come before the popped
unreachable child, which was not correct in the case that that popped
unreachable was a branch or other non-trapping instruction. Fix and simplify the
logic and re-enable the spec test that uncovered the bug.

Author: tlively

scripts/test/shared.py

@@ -417,9 +417,6 @@ def get_tests(test_dir, extensions=[], recursive=False):
     'type.wast',
     'unreached-invalid.wast',
 
-    # WAT parser error
-    'unwind.wast',
-
     # WAST parser error
     'binary.wast',
 

src/wasm/wasm-ir-builder.cpp

@@ -407,9 +407,12 @@ struct IRBuilder::ChildPopper
           --stackTupleIndex;
         } else {
           if (stackIndex == 0) {
-            // No more available values. This is fine iff we are reaching past
-            // an unreachable. Any error will be caught later when we are
-            // popping.
+            // No more available values. This is valid iff we are reaching past
+            // an unreachable, but we still need the fallback behavior to ensure
+            // the input unreachable instruction is executed first. If we are
+            // not reaching past an unreachable, the error will be caught we
+            // when pop.
+            needUnreachableFallback = true;
             goto pop;
           }
           --stackIndex;
@@ -458,18 +461,20 @@ struct IRBuilder::ChildPopper
     // We have checked all the constraints, so we are ready to pop children.
     for (int i = children.size() - 1; i >= 0; --i) {
       if (needUnreachableFallback &&
-          scope.exprStack.size() == *unreachableIndex + 1) {
-        // The expressions remaining on the stack may be executed, but they do
-        // not satisfy the requirements to be children of the current parent.
-        // Explicitly drop them so they will still be executed for their side
-        // effects and so the remaining children will be filled with
-        // unreachables.
-        assert(scope.exprStack.back()->type == Type::unreachable);
-        for (auto& expr : scope.exprStack) {
-          expr = Builder(builder.wasm).dropIfConcretelyTyped(expr);
-        }
+          scope.exprStack.size() == *unreachableIndex + 1 && i > 0) {
+        // The next item on the stack is the unreachable instruction we must
+        // not pop past. We cannot insert unreachables in front of it because
+        // it might be a branch we actually have to execute, so this next item
+        // must be child 0. But we are not ready to pop child 0 yet, so
+        // synthesize an unreachable instead of popping. The deeper
+        // instructions that would otherwise have been popped will remain on
+        // the stack to become prior children of future expressions or to be
+        // implicitly dropped at the end of the scope.
+        *children[i].childp = builder.builder.makeUnreachable();
+        continue;
       }
 
+      // Pop a child normally.
       auto val = pop(children[i].constraint.size());
       CHECK_ERR(val);
       *children[i].childp = *val;

test/lit/wat-kitchen-sink.wast

@@ -2728,6 +2728,42 @@
   br 0
  )
 
+ ;; CHECK:      (func $br-mismatch-after (type $1) (result i32)
+ ;; CHECK-NEXT:  (block $label (result i32)
+ ;; CHECK-NEXT:   (i32.add
+ ;; CHECK-NEXT:    (br $label
+ ;; CHECK-NEXT:     (i32.const 1)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:    (unreachable)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $br-mismatch-after (result i32)
+  i32.const 1
+  br 0
+  i32.add
+ )
+
+ ;; CHECK:      (func $br-mismatch-after-extra (type $1) (result i32)
+ ;; CHECK-NEXT:  (block $label (result i32)
+ ;; CHECK-NEXT:   (drop
+ ;; CHECK-NEXT:    (i64.const 0)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:   (i32.add
+ ;; CHECK-NEXT:    (br $label
+ ;; CHECK-NEXT:     (i32.const 1)
+ ;; CHECK-NEXT:    )
+ ;; CHECK-NEXT:    (unreachable)
+ ;; CHECK-NEXT:   )
+ ;; CHECK-NEXT:  )
+ ;; CHECK-NEXT: )
+ (func $br-mismatch-after-extra (result i32)
+  i64.const 0
+  i32.const 1
+  br 0
+  i32.add
+ )
+
  ;; CHECK:      (func $br_if (type $0)
  ;; CHECK-NEXT:  (block $l
  ;; CHECK-NEXT:   (br_if $l
@@ -3669,7 +3705,7 @@
  (func $ref-func
   ref.func $ref-func
   drop
-  ref.func 159
+  ref.func 161
   drop
  )