-
Notifications
You must be signed in to change notification settings - Fork 780
wasm-interp: Fix catch handlers' value stack sizes #2478
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -831,13 +831,6 @@ Result BinaryReaderInterp::BeginFunctionBody(Index index, Offset size) { | |
| // try-delegate instruction. | ||
| PushLabel(LabelKind::Try, Istream::kInvalidOffset, Istream::kInvalidOffset, | ||
| func_->handlers.size()); | ||
| func_->handlers.push_back(HandlerDesc{HandlerKind::Catch, | ||
| istream_.end(), | ||
| Istream::kInvalidOffset, | ||
| {}, | ||
| {Istream::kInvalidOffset}, | ||
| static_cast<u32>(func_->locals.size()), | ||
| 0}); | ||
| return Result::Ok; | ||
| } | ||
|
|
||
|
|
@@ -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. | ||
| // NOTE: we don't count the parameters, as they're not part of the frame. | ||
| func_->handlers.push_back(HandlerDesc{HandlerKind::Catch, | ||
| istream_.end(), | ||
| Istream::kInvalidOffset, | ||
| {}, | ||
| {Istream::kInvalidOffset}, | ||
| static_cast<u32>(local_decl_count_), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous code uses
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| 0}); | ||
| return Result::Ok; | ||
| } | ||
|
|
||
|
|
@@ -1516,7 +1519,9 @@ Result BinaryReaderInterp::OnTryExpr(Type sig_type) { | |
| u32 exn_stack_height; | ||
| CHECK_RESULT( | ||
| validator_.GetCatchCount(label_stack_.size() - 1, &exn_stack_height)); | ||
| u32 value_stack_height = validator_.type_stack_size(); | ||
| // NOTE: *NOT* GetLocalCount. we don't count the parameters, as they're not | ||
| // part of the frame. | ||
| u32 value_stack_height = validator_.type_stack_size() + local_decl_count_; | ||
| CHECK_RESULT(validator_.OnTry(GetLocation(), sig_type)); | ||
| // Push a label that tracks mapping of exn -> catch | ||
| PushLabel(LabelKind::Try, Istream::kInvalidOffset, Istream::kInvalidOffset, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| ;;; TOOL: run-interp-spec | ||
| ;;; ARGS*: --enable-exceptions | ||
| ;;; NOTE: ref: issue-2476 | ||
| (module | ||
| (tag $e0) | ||
| (func (export "broken-local") (result i32) | ||
| (local $value i32) | ||
| (try $try | ||
| (do | ||
| (local.set $value (i32.const 1)) | ||
| (throw $e0) | ||
| ) | ||
| (catch $e0) | ||
| ) | ||
| (local.get $value) | ||
| ) | ||
| ) | ||
|
|
||
| (assert_return (invoke "broken-local") (i32.const 1)) | ||
| (;; STDOUT ;;; | ||
| 2/2 tests passed. | ||
| ;;; STDOUT ;;) |
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()inBeginFunctionBody(which, presumably, always evaluates to0?). but that's not the fix we came here for.