-
Notifications
You must be signed in to change notification settings - Fork 787
Fix inner block problem with 'catch' #3129
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.
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
Conversation
// block. Can be used when we need to assume certain number of values are on top | ||
// of the stack in the beginning. | ||
Expression* WasmBinaryBuilder::getBlockOrSingleton(Type type, | ||
unsigned numPops) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This numPops
thing was added to be used by catch
, but now it does not use this method anymore, so deleted it.
@@ -2757,12 +2747,12 @@ void WasmBinaryBuilder::visitLoop(Loop* curr) { | |||
auto start = expressionStack.size(); | |||
processExpressions(); | |||
size_t end = expressionStack.size(); | |||
if (start > end) { | |||
throwError("block cannot pop from outside"); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was just moved to make code cleaner (in my opinion)
@@ -0,0 +1,19 @@ | |||
(module |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original break-within-catch.wasm
's wasm2wat
output is this:
(module
(type (;0;) (func))
(func (;0;) (type 0)
try ;; label = @1
nop
catch
drop
br 0 (;@1;)
end))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Comment changes are just suggestions, not sure how much they help.
src/wasm/wasm-binary.cpp
Outdated
auto* block = allocator.alloc<Block>(); | ||
block->list.push_back(curr); | ||
block->name = catchLabel; | ||
block->finalize(curr->type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could use a Builder and builder.makeBlock(catchLabel, curr)
on a single line.
(maybe the previous block allocation too, but it's not an obvious win there since you need to manually call pushBlockElements
anyhow.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Left the previous allocation as is, because of pushBlockElements
as you said.
Co-authored-by: Alon Zakai <[email protected]>
Fixes #3114.