From e374e6e4a0c3a8085a2055684b1d9dac8c8a848f Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 17 Sep 2024 11:49:42 -0700 Subject: [PATCH 1/4] start --- README.md | 3 ++ src/ir/eh-utils.cpp | 13 +++++++- src/ir/eh-utils.h | 2 ++ test/lit/passes/gto-removals.wast | 51 +++++++++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3eaafb7f72a..eb698fbd1c1 100644 --- a/README.md +++ b/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 diff --git a/src/ir/eh-utils.cpp b/src/ir/eh-utils.cpp index 70b5452a674..5e2e4d32951 100644 --- a/src/ir/eh-utils.cpp +++ b/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()) { + if (auto* block = firstChild->dynCast()) { + // 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, diff --git a/src/ir/eh-utils.h b/src/ir/eh-utils.h index 79ccbc50705..3d2825f6b75 100644 --- a/src/ir/eh-utils.h +++ b/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 diff --git a/test/lit/passes/gto-removals.wast b/test/lit/passes/gto-removals.wast index c2ac66a5793..7a3c250ec1b 100644 --- a/test/lit/passes/gto-removals.wast +++ b/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) + ) + ) + ) + ) + ) +) + From cebcc7e66731603685f8bca9e0267121d6975d05 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 17 Sep 2024 12:00:32 -0700 Subject: [PATCH 2/4] tests --- test/lit/passes/code-folding-eh-legacy.wast | 22 +++----- test/lit/passes/dce-eh-legacy.wast | 41 ++++---------- test/lit/passes/flatten-eh-legacy.wast | 54 +++++++----------- test/lit/passes/gufa-refs.wast | 30 +++------- test/lit/passes/gufa-tags.wast | 12 +--- test/lit/passes/heap2local.wast | 17 ++---- test/lit/passes/signature-pruning.wast | 62 ++++++++------------- 7 files changed, 76 insertions(+), 162 deletions(-) diff --git a/test/lit/passes/code-folding-eh-legacy.wast b/test/lit/passes/code-folding-eh-legacy.wast index 852ec126a92..897ae82d121 100644 --- a/test/lit/passes/code-folding-eh-legacy.wast +++ b/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 diff --git a/test/lit/passes/dce-eh-legacy.wast b/test/lit/passes/dce-eh-legacy.wast index 107c35609cd..c00ae6483bb 100644 --- a/test/lit/passes/dce-eh-legacy.wast +++ b/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)) diff --git a/test/lit/passes/flatten-eh-legacy.wast b/test/lit/passes/flatten-eh-legacy.wast index 56057b7798e..8bd38e00c07 100644 --- a/test/lit/passes/flatten-eh-legacy.wast +++ b/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,7 +172,6 @@ ;; 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 @@ -188,26 +179,21 @@ ;; 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: ) diff --git a/test/lit/passes/gufa-refs.wast b/test/lit/passes/gufa-refs.wast index d70de6a7a62..60bf9a8b5e2 100644 --- a/test/lit/passes/gufa-refs.wast +++ b/test/lit/passes/gufa-refs.wast @@ -1911,7 +1911,6 @@ (tag $empty (param)) ;; CHECK: (func $func (type $0) - ;; CHECK-NEXT: (local $0 anyref) ;; CHECK-NEXT: (throw $nothing ;; CHECK-NEXT: (ref.null none) ;; CHECK-NEXT: ) @@ -1920,15 +1919,12 @@ ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $nothing - ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (pop anyref) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result nullref) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $0) + ;; CHECK-NEXT: (pop anyref) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (ref.null none) ;; CHECK-NEXT: ) @@ -2139,7 +2135,6 @@ (tag $tag (param (ref null any)) (param (ref null any))) ;; CHECK: (func $func (type $1) - ;; CHECK-NEXT: (local $0 (tuple anyref anyref)) ;; CHECK-NEXT: (throw $tag ;; CHECK-NEXT: (ref.null none) ;; CHECK-NEXT: (struct.new_default $struct) @@ -2149,13 +2144,10 @@ ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag - ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (pop (tuple anyref anyref)) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result nullref) ;; CHECK-NEXT: (tuple.drop 2 - ;; CHECK-NEXT: (local.get $0) + ;; CHECK-NEXT: (pop (tuple anyref anyref)) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (ref.null none) ;; CHECK-NEXT: ) @@ -4171,7 +4163,6 @@ ) ;; CHECK: (func $rethrow (type $0) - ;; CHECK-NEXT: (local $0 i32) ;; CHECK-NEXT: (try $l0 ;; CHECK-NEXT: (do ;; CHECK-NEXT: (throw $e-i32 @@ -4179,20 +4170,15 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $e-i32 - ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (pop i32) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (block (result i32) - ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $0) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block (result i32) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (pop i32) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (i32.const 0) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (rethrow $l0) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (rethrow $l0) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) diff --git a/test/lit/passes/gufa-tags.wast b/test/lit/passes/gufa-tags.wast index a035673073b..6fb40a23fc1 100644 --- a/test/lit/passes/gufa-tags.wast +++ b/test/lit/passes/gufa-tags.wast @@ -17,8 +17,6 @@ (tag $tag$f32 (param f32)) ;; CHECK: (func $test (type $2) - ;; CHECK-NEXT: (local $0 i32) - ;; CHECK-NEXT: (local $1 f32) ;; CHECK-NEXT: (try ;; CHECK-NEXT: (do ;; CHECK-NEXT: (throw $tag$i32 @@ -26,26 +24,20 @@ ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag$i32 - ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (pop i32) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result i32) ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $0) + ;; CHECK-NEXT: (pop i32) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (i32.const 42) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag$f32 - ;; CHECK-NEXT: (local.set $1 - ;; CHECK-NEXT: (pop f32) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block ;; CHECK-NEXT: (drop - ;; CHECK-NEXT: (local.get $1) + ;; CHECK-NEXT: (pop f32) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (unreachable) ;; CHECK-NEXT: ) diff --git a/test/lit/passes/heap2local.wast b/test/lit/passes/heap2local.wast index 179437c3874..f00b5ee79e9 100644 --- a/test/lit/passes/heap2local.wast +++ b/test/lit/passes/heap2local.wast @@ -4504,19 +4504,15 @@ ;; CHECK: (func $struct-with-pop (type $0) ;; CHECK-NEXT: (local $0 i32) ;; CHECK-NEXT: (local $1 i32) - ;; CHECK-NEXT: (local $2 i32) ;; CHECK-NEXT: (try ;; CHECK-NEXT: (do ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag - ;; CHECK-NEXT: (local.set $2 - ;; CHECK-NEXT: (pop i32) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result nullref) ;; CHECK-NEXT: (local.set $1 - ;; CHECK-NEXT: (local.get $2) + ;; CHECK-NEXT: (pop i32) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (local.set $0 ;; CHECK-NEXT: (local.get $1) @@ -4534,8 +4530,7 @@ ) (catch $tag (drop - ;; We create a block when we replace the struct with locals, which the - ;; pop must be moved out of. + ;; We create a nameless block when we replace the struct with locals. (struct.new $struct (pop i32) ) @@ -4552,19 +4547,15 @@ ;; CHECK-NEXT: (local $4 i32) ;; CHECK-NEXT: (local $5 i32) ;; CHECK-NEXT: (local $6 i32) - ;; CHECK-NEXT: (local $7 i32) ;; CHECK-NEXT: (try ;; CHECK-NEXT: (do ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag - ;; CHECK-NEXT: (local.set $7 - ;; CHECK-NEXT: (pop i32) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (drop ;; CHECK-NEXT: (block (result (ref null $2)) ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (local.get $7) + ;; CHECK-NEXT: (pop i32) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (block (result nullref) ;; CHECK-NEXT: (local.set $4 @@ -4599,7 +4590,7 @@ ) (catch $tag (drop - ;; As above, but with an array + ;; As above, but with an array. (array.new $array (pop i32) (i32.const 3) diff --git a/test/lit/passes/signature-pruning.wast b/test/lit/passes/signature-pruning.wast index 57dd783622c..b9f0c29e924 100644 --- a/test/lit/passes/signature-pruning.wast +++ b/test/lit/passes/signature-pruning.wast @@ -997,33 +997,27 @@ ;; CHECK: (func $catch-pop (type $2) (result i32) ;; CHECK-NEXT: (local $0 i32) ;; CHECK-NEXT: (local $1 i32) - ;; CHECK-NEXT: (local $2 i32) ;; CHECK-NEXT: (block $block (result i32) ;; CHECK-NEXT: (try $try ;; CHECK-NEXT: (do ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag - ;; CHECK-NEXT: (local.set $2 - ;; CHECK-NEXT: (pop i32) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (local.get $2) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (local.set $1 - ;; CHECK-NEXT: (br_if $block - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: (i32.const 2) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (call $target - ;; CHECK-NEXT: (local.get $0) + ;; CHECK-NEXT: (local.set $0 + ;; CHECK-NEXT: (pop i32) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $1 + ;; CHECK-NEXT: (br_if $block + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (i32.const 2) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (call $target + ;; CHECK-NEXT: (local.get $0) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (i32.const 3) @@ -1039,15 +1033,13 @@ (call $target (pop i32) ;; We can remove this parameter by moving it to a local first, which - ;; also moves the pop, which then needs to be fixed up. + ;; also moves the pop. (br_if $block (i32.const 1) (i32.const 2) ) ) - ;; This nop causes the call to be in a block. When we add another - ;; block to hold the code that we move, we'd get an error if we don't - ;; apply fixups. + ;; This nop causes the call to be in a block. (nop) ) ) @@ -1086,33 +1078,27 @@ ;; CHECK: (func $catch-pop (type $2) (result i32) ;; CHECK-NEXT: (local $0 i32) ;; CHECK-NEXT: (local $1 i32) - ;; CHECK-NEXT: (local $2 i32) ;; CHECK-NEXT: (block $block (result i32) ;; CHECK-NEXT: (try $try ;; CHECK-NEXT: (do ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (catch $tag - ;; CHECK-NEXT: (local.set $2 - ;; CHECK-NEXT: (pop i32) - ;; CHECK-NEXT: ) ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (block - ;; CHECK-NEXT: (local.set $0 - ;; CHECK-NEXT: (local.get $2) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (local.set $1 - ;; CHECK-NEXT: (br_if $block - ;; CHECK-NEXT: (i32.const 1) - ;; CHECK-NEXT: (i32.const 2) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (call $target - ;; CHECK-NEXT: (local.get $1) + ;; CHECK-NEXT: (local.set $0 + ;; CHECK-NEXT: (pop i32) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (local.set $1 + ;; CHECK-NEXT: (br_if $block + ;; CHECK-NEXT: (i32.const 1) + ;; CHECK-NEXT: (i32.const 2) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) - ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: (call $target + ;; CHECK-NEXT: (local.get $1) + ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (nop) ;; CHECK-NEXT: ) ;; CHECK-NEXT: ) ;; CHECK-NEXT: (i32.const 3) From 330da85703ceb7f136dc9595090c882cf030a1fc Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 17 Sep 2024 13:41:31 -0700 Subject: [PATCH 3/4] update a lit test --- test/lit/binary/stacky-eh-legacy.test | 31 +++++---------------------- 1 file changed, 5 insertions(+), 26 deletions(-) diff --git a/test/lit/binary/stacky-eh-legacy.test b/test/lit/binary/stacky-eh-legacy.test index c22a5165d5d..70e45a12ec6 100644 --- a/test/lit/binary/stacky-eh-legacy.test +++ b/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:) From 0298f182458f12a0ebf8594e3be013b6c244d1f8 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Wed, 18 Sep 2024 12:41:15 -0700 Subject: [PATCH 4/4] progress --- src/wasm/wasm-ir-builder.cpp | 32 +++++++++++++++++++--------- test/lit/validation/eh-nameless.wast | 29 +++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 test/lit/validation/eh-nameless.wast diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index b238a926c42..a4496c95542 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -1441,17 +1441,29 @@ Result<> IRBuilder::makePop(Type type) { // already create them automatically when starting a legacy catch block that // needs one. Just verify that the Pop we are being asked to make is the same // type as the Pop we have already made. - auto& scope = getScope(); - if (!scope.getCatch() || scope.exprStack.size() != 1 || - !scope.exprStack[0]->is()) { - return Err{ - "pop instructions may only appear at the beginning of catch blocks"}; - } - auto expectedType = scope.exprStack[0]->type; - if (!Type::isSubType(expectedType, type)) { - return Err{std::string("Expected pop of type ") + expectedType.toString()}; + // + // We need to look up the scope stack, as we can look through nameless blocks. + Index i = 0; + while (1) { + auto scopeCheck = getScope(i++); + CHECK_ERR(scopeCheck); + auto& scope = **scopeCheck; + if (auto* block = scope.getBlock()) { + if (!block->name.is()) { + continue; + } + } + if (!scope.getCatch() || scope.exprStack.size() != 1 || + !scope.exprStack[0]->is()) { + return Err{ + "pop instructions may only appear at the beginning of catch blocks"}; + } + auto expectedType = scope.exprStack[0]->type; + if (!Type::isSubType(expectedType, type)) { + return Err{std::string("Expected pop of type ") + expectedType.toString()}; + } + return Ok{}; } - return Ok{}; } Result<> IRBuilder::makeRefNull(HeapType type) { diff --git a/test/lit/validation/eh-nameless.wast b/test/lit/validation/eh-nameless.wast new file mode 100644 index 00000000000..90ff5723e7d --- /dev/null +++ b/test/lit/validation/eh-nameless.wast @@ -0,0 +1,29 @@ +;; Test for a nameless block containing a pop, which is allowed. + +;; XXXXXXXXXXXXXXX RUN: not wasm-opt --enable-reference-types %s -o - -S 2>&1 | filecheck %s --check-prefix NO-GC +;; RUN: wasm-opt --enable-reference-types --enable-gc %s -o - -S | filecheck %s --check-prefix GC + +;; NO-GC: all used types should be allowed + +;; GC: (func $foo (type $0) (param $x eqref) + +(module + (tag $tag (param i32)) + + (func $0 + (try + (do + (nop) + ) + (catch $tag + (drop + (block (result i32) + (pop i32) + ) + ) + ) + ) + ) +) + +;; TODO: test two nested blocks