Validate pops while ignoring nameless blocks?

README.md

@@ -148,6 +148,9 @@ There are a few differences between Binaryen IR and the WebAssembly language:
       much about this when writing Binaryen passes. For more details see the
       `requiresNonNullableLocalFixups()` hook in `pass.h` and the
       `LocalStructuralDominance` class.
+  * Validation of the `pop` instruction in Wasm Exceptions will ignore nameless
+    blocks, similar to validation of non-nullable locals in the previous point
+    (and for the same reasons).
   * `br_if` output types are more refined in Binaryen IR: they have the type of
     the value, when a value flows in. In the wasm spec the type is that of the
     branch target, which may be less refined. Using the more refined type here

src/ir/eh-utils.cpp

@@ -64,7 +64,18 @@ getFirstPop(Expression* catchBody, bool& isPopNested, Expression**& popPtr) {
         // run more than once
         return nullptr;
       }
-      if (firstChild->is<Block>()) {
+      if (auto* block = firstChild->dynCast<Block>()) {
+        // Ignore nameless blocks, as they vanish in the binary format.
+        if (!block->name.is()) {
+          if (block->list.empty()) {
+            // No children.
+            return nullptr;
+          }
+          firstChild = block->list[0];
+          firstChildPtr = &block->list[0];
+          continue;
+        }
+
         // If there are no branches that targets the implicit block, it will be
         // removed when written back. But if there are branches that target the
         // implicit block,

src/ir/eh-utils.h

@@ -30,6 +30,8 @@ namespace EHUtils {
 //   whose tag type is void or a catch_all's body, this returns false.
 // - This returns true even if there are more pops after the first one within a
 //   catch body, which is invalid. That will be taken care of in validation.
+// - This ignores blocks without a name, as we never emit those in the binary
+//   format.
 bool containsValidDanglingPop(Expression* catchBody);
 
 // Given a 'Try' expression, fixes up 'pop's nested in blocks, which are

test/lit/binary/stacky-eh-legacy.test

@@ -13,47 +13,25 @@
 ;;
 ;; This code is 'stacky' in Binaryen parlance. In the binary reader, Binaryen has
 ;; a special routine of creating a block and a local.get/local.set to read stacky
-;; code into Binaryen AST. So if we don't do any post-fixup, the 'catch' block
-;; becomes:
-;; (catch $e-i32
-;;   (local.set 2
-;;     (block (result i32)
-;;       (local.set $new
-;;         (pop i32)
-;;       )
-;;       (local.set 1
-;;         (i32.const 3)
-;;       )
-;;       (local.get $new)
-;;     )
-;;   )
-;; )
-;; Here the 'block' and `local $new' are newly created to read the stacky code.
-;; But now the 'pop' ends up nested within the 'block', which is invalid. This
-;; test tests if this invalid code is correctly fixed up in the binary reader.
-;; The fixup will hoist the 'pop' and create another local to store it right
-;; after 'catch'.
+;; code into Binaryen AST. This test checks that we do not emit anything invalid
+;; for the pop in such a case.
 
 RUN: wasm-opt -all %s.wasm --print | filecheck %s
 
-;; CHECK:      (func $0
+;; CHECK:      (func $0 (type $1)
 ;; CHECK-NEXT:  (local $0 i32)
 ;; CHECK-NEXT:  (local $1 i32)
 ;; CHECK-NEXT:  (local $2 i32)
 ;; CHECK-NEXT:  (local $3 i32)
-;; CHECK-NEXT:  (local $4 i32)
 ;; CHECK-NEXT:  (try $label$3
 ;; CHECK-NEXT:   (do
 ;; CHECK-NEXT:    (nop)
 ;; CHECK-NEXT:   )
 ;; CHECK-NEXT:   (catch $tag$0
-;; CHECK-NEXT:    (local.set $4
-;; CHECK-NEXT:     (pop i32)
-;; CHECK-NEXT:    )
 ;; CHECK-NEXT:    (local.set $2
 ;; CHECK-NEXT:     (block (result i32)
 ;; CHECK-NEXT:      (local.set $3
-;; CHECK-NEXT:       (local.get $4)
+;; CHECK-NEXT:       (pop i32)
 ;; CHECK-NEXT:      )
 ;; CHECK-NEXT:      (local.set $1
 ;; CHECK-NEXT:       (i32.const 3)
@@ -64,3 +42,4 @@ RUN: wasm-opt -all %s.wasm --print | filecheck %s
 ;; CHECK-NEXT:   )
 ;; CHECK-NEXT:  )
 ;; CHECK-NEXT: )
+;; CHECK-NEXT:)

test/lit/passes/code-folding-eh-legacy.wast

@@ -343,28 +343,22 @@
   )
 
   ;; CHECK:      (func $if-arms-in-catch (type $0) (result i32)
-  ;; CHECK-NEXT:  (local $0 i32)
   ;; CHECK-NEXT:  (try
   ;; CHECK-NEXT:   (do
   ;; CHECK-NEXT:    (unreachable)
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:   (catch $e-i32
-  ;; CHECK-NEXT:    (local.set $0
-  ;; CHECK-NEXT:     (pop i32)
-  ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:    (block
-  ;; CHECK-NEXT:     (block
-  ;; CHECK-NEXT:      (drop
-  ;; CHECK-NEXT:       (local.get $0)
-  ;; CHECK-NEXT:      )
-  ;; CHECK-NEXT:      (drop
-  ;; CHECK-NEXT:       (i32.eqz
-  ;; CHECK-NEXT:        (i32.const 1)
-  ;; CHECK-NEXT:       )
+  ;; CHECK-NEXT:     (drop
+  ;; CHECK-NEXT:      (pop i32)
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (drop
+  ;; CHECK-NEXT:      (i32.eqz
+  ;; CHECK-NEXT:       (i32.const 1)
   ;; CHECK-NEXT:      )
   ;; CHECK-NEXT:     )
-  ;; CHECK-NEXT:     (unreachable)
   ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (unreachable)
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT: )
@@ -375,7 +369,7 @@
       )
       (catch $e-i32
         ;; These if arms can be folded, after which the if is replaced by a
-        ;; block, so we need a fixup for the pop.
+        ;; block. The block is nameless, so we need no fixup for the pop.
         (if
           (pop i32)
           (then

test/lit/passes/dce-eh-legacy.wast

@@ -122,23 +122,17 @@
   )
 
   ;; CHECK:      (func $call-pop-catch (type $0)
-  ;; CHECK-NEXT:  (local $0 i32)
   ;; CHECK-NEXT:  (block $label
   ;; CHECK-NEXT:   (try
   ;; CHECK-NEXT:    (do
   ;; CHECK-NEXT:     (nop)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:    (catch $e-i32
-  ;; CHECK-NEXT:     (local.set $0
-  ;; CHECK-NEXT:      (pop i32)
-  ;; CHECK-NEXT:     )
   ;; CHECK-NEXT:     (block
-  ;; CHECK-NEXT:      (block
-  ;; CHECK-NEXT:       (drop
-  ;; CHECK-NEXT:        (local.get $0)
-  ;; CHECK-NEXT:       )
-  ;; CHECK-NEXT:       (br $label)
+  ;; CHECK-NEXT:      (drop
+  ;; CHECK-NEXT:       (pop i32)
   ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:      (br $label)
   ;; CHECK-NEXT:     )
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
@@ -151,8 +145,8 @@
          (nop)
        )
        (catch $e-i32
-         ;; This call is unreachable and can be removed. The pop, however, must
-         ;; be carefully handled while we do so, to not break validation.
+         ;; This call is unreachable and can be removed. A nameless block will
+         ;; be added around the pop, but that is ok.
          (call $helper-i32-i32
            (pop i32)
            (br $label)
@@ -171,24 +165,18 @@
   )
 
   ;; CHECK:      (func $pop-within-block (type $4) (result i32)
-  ;; CHECK-NEXT:  (local $0 eqref)
   ;; CHECK-NEXT:  (try (result i32)
   ;; CHECK-NEXT:   (do
   ;; CHECK-NEXT:    (i32.const 0)
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:   (catch $e-eqref
-  ;; CHECK-NEXT:    (local.set $0
-  ;; CHECK-NEXT:     (pop eqref)
-  ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:    (block
-  ;; CHECK-NEXT:     (block
-  ;; CHECK-NEXT:      (drop
-  ;; CHECK-NEXT:       (struct.new $struct
-  ;; CHECK-NEXT:        (local.get $0)
-  ;; CHECK-NEXT:       )
+  ;; CHECK-NEXT:     (drop
+  ;; CHECK-NEXT:      (struct.new $struct
+  ;; CHECK-NEXT:       (pop eqref)
   ;; CHECK-NEXT:      )
-  ;; CHECK-NEXT:      (unreachable)
   ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (unreachable)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
@@ -200,16 +188,7 @@
       )
       (catch $e-eqref
         (drop
-          ;; Optimization moves the 'pop' inside a block, which needs to be
-          ;; extracted out of the block at the end.
-          ;; (block
-          ;;   (drop
-          ;;     (struct.new $struct.0
-          ;;       (pop eqref)
-          ;;     )
-          ;;   )
-          ;;   (unreachable)
-          ;; )
+          ;; Optimization moves the 'pop' inside a nameless block.
           (ref.eq
             (struct.new $struct
               (pop (ref null eq))

test/lit/passes/flatten-eh-legacy.wast

@@ -57,37 +57,29 @@
 
   ;; CHECK:      (func $try_catch_pop_fixup (type $1)
   ;; CHECK-NEXT:  (local $0 i32)
-  ;; CHECK-NEXT:  (local $1 i32)
   ;; CHECK-NEXT:  (block $l0
   ;; CHECK-NEXT:   (try
   ;; CHECK-NEXT:    (do
   ;; CHECK-NEXT:     (nop)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:    (catch $e-i32
-  ;; CHECK-NEXT:     (local.set $1
-  ;; CHECK-NEXT:      (pop i32)
-  ;; CHECK-NEXT:     )
   ;; CHECK-NEXT:     (block
-  ;; CHECK-NEXT:      (block
-  ;; CHECK-NEXT:       (local.set $0
-  ;; CHECK-NEXT:        (local.get $1)
-  ;; CHECK-NEXT:       )
-  ;; CHECK-NEXT:       (drop
-  ;; CHECK-NEXT:        (local.get $0)
-  ;; CHECK-NEXT:       )
-  ;; CHECK-NEXT:       (br $l0)
-  ;; CHECK-NEXT:       (unreachable)
+  ;; CHECK-NEXT:      (local.set $0
+  ;; CHECK-NEXT:       (pop i32)
+  ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:      (drop
+  ;; CHECK-NEXT:       (local.get $0)
   ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:      (br $l0)
   ;; CHECK-NEXT:      (unreachable)
   ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (unreachable)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )
   ;; CHECK-NEXT: )
   (func $try_catch_pop_fixup
-    ;; After --flatten, a block is created within the 'catch', which makes the
-    ;; pop's location invalid. This tests whether it is fixed up correctly after
-    ;; --flatten.
+    ;; After --flatten, a nameless block is created within the 'catch'.
     (block $l0
       (try
         (do)
@@ -180,34 +172,28 @@
   ;; CHECK-NEXT:  (local $2 i32)
   ;; CHECK-NEXT:  (local $3 i32)
   ;; CHECK-NEXT:  (local $4 i32)
-  ;; CHECK-NEXT:  (local $5 i32)
   ;; CHECK-NEXT:  (try
   ;; CHECK-NEXT:   (do
   ;; CHECK-NEXT:    (local.set $3
   ;; CHECK-NEXT:     (i32.const 3)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:   (catch $e-i32
-  ;; CHECK-NEXT:    (local.set $5
-  ;; CHECK-NEXT:     (pop i32)
-  ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:    (block
-  ;; CHECK-NEXT:     (block
-  ;; CHECK-NEXT:      (local.set $0
-  ;; CHECK-NEXT:       (local.get $5)
-  ;; CHECK-NEXT:      )
-  ;; CHECK-NEXT:      (drop
-  ;; CHECK-NEXT:       (local.get $0)
-  ;; CHECK-NEXT:      )
-  ;; CHECK-NEXT:      (unreachable)
-  ;; CHECK-NEXT:      (unreachable)
-  ;; CHECK-NEXT:     )
-  ;; CHECK-NEXT:     (local.set $2
-  ;; CHECK-NEXT:      (local.get $1)
+  ;; CHECK-NEXT:     (local.set $0
+  ;; CHECK-NEXT:      (pop i32)
   ;; CHECK-NEXT:     )
-  ;; CHECK-NEXT:     (local.set $3
-  ;; CHECK-NEXT:      (local.get $2)
+  ;; CHECK-NEXT:     (drop
+  ;; CHECK-NEXT:      (local.get $0)
   ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:     (unreachable)
+  ;; CHECK-NEXT:     (unreachable)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (local.set $2
+  ;; CHECK-NEXT:     (local.get $1)
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:    (local.set $3
+  ;; CHECK-NEXT:     (local.get $2)
   ;; CHECK-NEXT:    )
   ;; CHECK-NEXT:   )
   ;; CHECK-NEXT:  )

test/lit/passes/gto-removals.wast

@@ -1405,3 +1405,54 @@
     )
   )
 )
+
+;; Test we handle a pop in a removed field properly without erroring.
+(module
+  ;; CHECK:      (type $0 (func (param eqref)))
+
+  ;; CHECK:      (rec
+  ;; CHECK-NEXT:  (type $struct (struct))
+  (type $struct (struct (field (mut eqref))))
+
+  ;; CHECK:       (type $2 (func))
+
+  ;; CHECK:       (type $3 (func (param eqref)))
+
+  ;; CHECK:      (tag $tag (param eqref))
+  (tag $tag (param eqref))
+
+  ;; CHECK:      (func $test (type $2)
+  ;; CHECK-NEXT:  (local $0 eqref)
+  ;; CHECK-NEXT:  (try
+  ;; CHECK-NEXT:   (do
+  ;; CHECK-NEXT:    (nop)
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:   (catch $tag
+  ;; CHECK-NEXT:    (drop
+  ;; CHECK-NEXT:     (block (result (ref $struct))
+  ;; CHECK-NEXT:      (local.set $0
+  ;; CHECK-NEXT:       (pop eqref)
+  ;; CHECK-NEXT:      )
+  ;; CHECK-NEXT:      (struct.new_default $struct)
+  ;; CHECK-NEXT:     )
+  ;; CHECK-NEXT:    )
+  ;; CHECK-NEXT:   )
+  ;; CHECK-NEXT:  )
+  ;; CHECK-NEXT: )
+  (func $test
+    (try
+      (do
+        (nop)
+      )
+      (catch $tag
+        (drop
+          (struct.new $struct
+            ;; This field receives a pop, but can also be removed from the type.
+            (pop eqref)
+          )
+        )
+      )
+    )
+  )
+)
+