-
Notifications
You must be signed in to change notification settings - Fork 693
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
wasm-interp: Fix catch handlers' value stack sizes #2478
Conversation
Istream::kInvalidOffset, | ||
{}, | ||
{Istream::kInvalidOffset}, | ||
static_cast<u32>(local_decl_count_), |
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 previous code uses func_->locals.size()
here. How is that different? Perhaps you could mention in the PR description how the old code is wrong?
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
src/interp/binary-reader-interp.cc
Outdated
@@ -856,6 +849,16 @@ Result BinaryReaderInterp::EndFunctionBody(Index index) { | |||
Result BinaryReaderInterp::OnLocalDeclCount(Index count) { | |||
local_decl_count_ = count; | |||
local_count_ = 0; | |||
// FIXME(Soni): does the value of `values` even matter here? it used to be | |||
// always 0 when this call was in BeginFunctionBody. |
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.
I'm not sure this FIXME comment is useful, maybe just remove it?
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.
yeah that's fair.
... ugh, the more we look at all this code the more questionable stuff we find, like the func_->handlers.size()
in BeginFunctionBody
(which, presumably, always evaluates to 0
?). but that's not the fix we came here for.
@sbc100 ping? |
Fixes the value stack size of the catch handler. There were two (related) issues here:
func_->locals.size()
as soon as the function was available, but it hadn't processed the function's locals yet, so it was always empty. (This might not matter in practice, as it's only used by the "function-wide catch handler", which just rethrows.)Closes #2476
(Split from #2470 )