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.

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

Add interpreter support for the exception handling proposal #1749

Merged
merged 9 commits into from
Nov 17, 2021

Conversation

takikawa
Copy link
Contributor

@takikawa takikawa commented Oct 28, 2021

This PR adds interpreter support for the exception handling proposal.

Details about the implementation approach:

  • Try blocks generate metadata tracking the instruction ranges for the handlers and which exception tags are handled (or if a catch_all is present). The metadata is stored in a function's FuncDesc, and is transferred into the Frame when a function call is executed.
  • The stack is unwound when a throw is executed. This unwinding also handles tag dispatch to the appropriate catch. The metadata to find the matching handler is looked up in the call Frame stack.
  • If a try-delegate is present, it is used in the stack unwinding process to skip over to the relevant handler.
  • A separate exceptions_ stack in call frames tracks caught exceptions that can be accessed via a rethrow. The stack is popped on exit from a try block or when exiting via control instructions like br.
  • Because stack unwinding relies on finding metadata in the call frame, return_call needs to be modified slightly to adjust the current frame when executing the call, rather than re-using the frame completely as-is.

Spec tests will be followed up in future PRs once the proposal tests are merged into the official testsuite.

@takikawa takikawa force-pushed the eh-interpreter branch 2 times, most recently from a3a194f to 9167cfd Compare October 28, 2021 00:31
@takikawa
Copy link
Contributor Author

Added f260e67, which is a small fixup commit that can be squashed later. It fixes a bug triggered by a newer version of the spec tests, with a cross-instance throw. It's difficult to test this with the wabt test framework, but the spec tests will catch it.

@takikawa
Copy link
Contributor Author

Thanks for the merging the other two PRs @aheejin :) I forgot to also CC you on this big patch.

@sbc100
Copy link
Member

sbc100 commented Oct 30, 2021

Can you re-base on top of the already landed parts. Also, if there are remaining separate commits can you split them out into their own PRs maybe? Or does that make no sense in this case?

Details about the implementation approach:

  * Try blocks generate metadata tracking the instruction ranges for the
    handlers and which exception tags are handled (or if a `catch_all` is
    present). The metadata is stored in a function's `FuncDesc`, and is
    transferred into the `Frame` when a function call is executed.
  * The stack is unwound when a `throw` is executed. This unwinding also
    handles tag dispatch to the appropriate catch. The metadata to find
    the matching handler is looked up in the call `Frame` stack.
  * If a `try-delegate` is present, it is used in the stack unwinding
    process to skip over to the relevant handler.
  * A separate `exceptions_` stack in call frames tracks caught
    exceptions that can be accessed via a `rethrow`. The stack is popped
    on exit from a try block or when exiting via control instructions
    like `br`.
  * Because stack unwinding relies on finding metadata in the call
    frame, `return_call` needs to be modified slightly to adjust the
    current frame when executing the call, rather than re-using the
    frame completely as-is.
@takikawa
Copy link
Contributor Author

Can you re-base on top of the already landed parts.

Sure, no problem. I just pushed a rebased version. Also I squashed the other commit into this too as it's not really a separate thing (since I was force pushing for the rebase anyway).

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Nice!

Will some of the tests added here be redundant once we start running the spec tests?

Can you move the notes to the main PR description?

I'll leave the final lgtm to @aheejin

src/interp/interp.h Outdated Show resolved Hide resolved
src/interp/istream.h Show resolved Hide resolved
@takikawa
Copy link
Contributor Author

takikawa commented Nov 2, 2021

Re: tests, there is some redundancy with spec tests in that there's overlap in what's tested and so they look similar (though I didn't copy-paste the spec tests). I did try to write tests that specifically target the more tricky bits of the wabt interpreter implementation (e.g., the exception stack for rethrow, how exception handlers are looked up, etc).

@sbc100
Copy link
Member

sbc100 commented Nov 2, 2021

lgtm. WDYT @aheejin ?

@takikawa
Copy link
Contributor Author

takikawa commented Nov 9, 2021

Friendly ping on this @aheejin, any feedback or comments appreciated :)

@aheejin
Copy link
Member

aheejin commented Nov 10, 2021

Sorry! I was OOO for a while. I will take a look shortly.

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

Sorry again for the super delayed reply. Haven't read the tests yet, but here are comments and questions. I'm not very familiar with the wabt codebase, so I might be asking some basic questions 😅

src/type-checker.cc Show resolved Hide resolved
src/type-checker.cc Outdated Show resolved Hide resolved
src/interp/interp.h Outdated Show resolved Hide resolved
src/interp/binary-reader-interp.cc Outdated Show resolved Hide resolved
src/interp/binary-reader-interp.cc Outdated Show resolved Hide resolved
src/interp/interp.cc Outdated Show resolved Hide resolved
src/interp/interp.cc Outdated Show resolved Hide resolved
src/interp/binary-reader-interp.cc Show resolved Hide resolved
src/interp/interp.cc Outdated Show resolved Hide resolved
src/interp/binary-reader-interp.cc Show resolved Hide resolved
@takikawa
Copy link
Contributor Author

takikawa commented Nov 16, 2021

Thanks for the review feedback! I think I've addressed pretty much all the feedback now, although I still need to check if return_call_indirect has any faulty interaction with this.

@takikawa
Copy link
Contributor Author

Pushed an extra commit that should fix the CI failure

src/interp/binary-reader-interp.cc Outdated Show resolved Hide resolved
src/interp/binary-reader-interp.cc Show resolved Hide resolved
@takikawa
Copy link
Contributor Author

takikawa commented Nov 16, 2021

I pushed a fix for the return_call_indirect case in 34fb572. I put in a sanity check assert that checks, when a function call is popped, that the frame's exception stack height (the height on function entry) matches the current height. The reason for the assertion is it's hard to catch problems with the stack height with just unit tests.

This assert caught another bug, so I included that fix (some catchs weren't popping the exception because it was being done only for end).

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

Thanks for your patient answers and fixes! Sorry again this took so long. LGTM % remaining small nits

test/interp/rethrow-and-br.txt Outdated Show resolved Hide resolved
test/interp/try.txt Outdated Show resolved Hide resolved
@takikawa
Copy link
Contributor Author

Thanks for all the comments! I think I've addressed everything now, and CI looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants