Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate pops while ignoring nameless blocks? #6950

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Validate pops while ignoring nameless blocks? #6950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e374e6e
start
kripken Sep 17, 2024
cebcc7e
tests
kripken Sep 17, 2024
330da85
update a lit test
kripken Sep 17, 2024
0298f18
progress
kripken Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 12 additions & 1 deletion src/ir/eh-utils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions src/ir/eh-utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 22 additions & 10 deletions src/wasm/wasm-ir-builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Pop>()) {
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<Pop>()) {
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{};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change avoids the error, but it doesn't actually move the pop into the right place. Whenever we try to pop from an empty stack, we should look up the stack through nameless blocks to find pops that we could use.

Weird edge case to test:

  • We lazily give unnamed blocks names when we encounter branches that target them via an index. What happens if we have previously tried to pull a pop through such a block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this a situation of popping from an empty stack? Are Pop instructions not added to the stack like anything else? (they have a concrete type)

Copy link
Member

@tlively tlively Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, pops are special. They are automatically pushed onto the stack when the beginning of a catch is parsed, not when the pop instruction itself is parsed. That's why the existing code here just does some error checking and doesn't push anything. The reason is that we fully support parsing standard code that does not contain any pops, so we can't wait until we see a pop to push it onto the stack. For example, this parses:

try
catch $e
 drop
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the edge case here, we should use a scratch local to pull the value of the pop into the block. That won't help the example I gave below, though, so we still need to relax the validation behavior as well.

}
return Ok{};
}

Result<> IRBuilder::makeRefNull(HeapType type) {
Expand Down
31 changes: 5 additions & 26 deletions test/lit/binary/stacky-eh-legacy.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -64,3 +42,4 @@ RUN: wasm-opt -all %s.wasm --print | filecheck %s
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT:)
22 changes: 8 additions & 14 deletions test/lit/passes/code-folding-eh-legacy.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: )
Expand All @@ -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
Expand Down
41 changes: 10 additions & 31 deletions test/lit/passes/dce-eh-legacy.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: )
Expand All @@ -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)
Expand All @@ -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: )
Expand All @@ -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))
Expand Down
54 changes: 20 additions & 34 deletions test/lit/passes/flatten-eh-legacy.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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: )
Expand Down
Loading
Loading