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

Updating Exception Handling in WAMR #3753

Open
woodsmc opened this issue Aug 23, 2024 · 20 comments
Open

Updating Exception Handling in WAMR #3753

woodsmc opened this issue Aug 23, 2024 · 20 comments

Comments

@woodsmc
Copy link
Contributor

woodsmc commented Aug 23, 2024

Feature

Adding support for the latest exception handling specification to WAMR

Benefit

This would bring WAMR's exception handling inline with the current standard,

Implementation

At the moment (see GitHub names below) would like to collaborate to add support for exception handing in WAMR. To reduce the cost of code maintenance going forward the expectation is to replace the current (now out dated) exception handling implementation with one which meets the current Wasm specification.

Contributors
@woodsmc
@ermler
@cjbudnik
@Papadiddypub

Scope

This effort will focus on implementing the new exception handling implementation in WAMRs interpreter mode. Due to time constraints and the availability of the contributors it is unlikely that there would be folks available to address FastJIT / JIT or AoT. If there were additional contributors who would like to assist they would be most welcome.

Alternatives

There are alternative approaches to how the specification is implemented, including keeping the current implementation and adding additional support for the new specification. We considered and rejected this approach as it would increase the amount of code which would need to be maintained, and the number of applications using exception handling on WAMR is limited at the moment.

Implementation Discussion (Open Questions)

The following outlines some of the current discussions as this effort starts to get going, it is shared here for comment, reflection and openness. Feed back and any ideas would be really appreciated:

Every Wasm function can return an exception

The new specification indicated that if an exception handler can not be found (uncaught exception) that the exception should be returned to the caller. This implies that every Wasm function can return an exception OR its originally anticipated results, even if that function returns void, it would still be possible for it to return an exception.

We understand that this is maybe a fundamental shift in how WAMR supports invoking functions, now every Wasm function returns an exception or the data its function signature indicates. Would this mean updating the WAMR runtime's API ? - How could we do this in a way that has minimal impact for existing users, while still adhering to the specification ?

Naming Conventions: When an exception isn't an exception

The existing WAMR code base uses "exceptions" to mean "traps", in the long run this may cause some confusion. Is it worth while refactoring the code to rename "exceptions" to traps, so that when the exception handling implementation is added the code is easier to understand and maintain ?

The growing list of return values

Realistically a Wasm function can return any one of the following:

  • The original function signature parameter(s)
  • An exception
  • A trap

It a discussion with Ben Titzer at CMU, he suggested unifying the return path from exception and trap? - Perhaps this would work if we addressed naming convention issue too ?

Ben also pointed out that stack switching is likely to introduce another return path, where a function call results in a set of suspend / resume calls which result in a broken stack and no return path / value.

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 23, 2024

Some useful resources for the discussion:

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 23, 2024

@wenyongh - Do you have any thoughts on refactoring code for naming conventions / function return paths and data sets ?

@wenyongh
Copy link
Contributor

@woodsmc Thanks for the updating, it is great! Though this update only includes the interpreter mode, it is also fantastic and will definitely be a good start for AOT mode and JIT mode. For the alternatives, agree with you, I think no need to keep the current implementation since (1) currently there should be few developers using the exception handling feature and I believe they will also move forward to align to latest spec, e.g. #3599, (2) we already released WAMR-2.x.x versions which well support old version of exception handling.

For the issue of "refactoring code for naming conventions / function return paths", as you know, now WAMR runtime's API is written in C and the API wasm_runtime_call_wasm (and wasm_runtime_call_wasm_a/v) doesn't return exception directly, instead, developer can call wasm_runtime_get_exception to check whether an exception was thrown after calling wasm_runtime_call_wasm. I think it would be difficult/complex to change the API's return value to return extra exception/trap info besides wasm function return value, and such changes impact developer too much, so we had better keep the current mechanism unchanged (call wasm_runtime_call_wasm and wasm_runtime_get_exception) and keep the two APIs' prototype unchanged.

And I must mentioned that wasm_runtime_call_wasm(.., uint32 argc, uint32 argv[]) already supports multiple return values of a wasm function: the return values of wasm function are stored in argv array compactly. So per my understanding, the main issue is the exception related API, there may be two options:
(1) renaming wasm_runtime_get_exception/wasm_runtime_set_exception to wasm_runtime_get_trap/wasm_runtime_set_trap, and add new APIs for exception handling,
(2) unify traps and exceptions.

My personal opinion is that the exception APIs have been widely used and we had better not changed them, it may impact developer a lot and might make bigger confusing if we change the API. Can we unify traps and exceptions? Not sure what extra info need to be returned for the exception handling proposal, is it just a "uncaught exception" string, or should we distinguish whether the return exception is a trap or an exception and return more for the exception? How about:

(1) keep wasm_runtime_get_exception/wasm_runtime_set_exception API unchanged
(2) if an exception of exception handling proposal isn't caught in a function, wasm_runtime_get_exception returns "uncaught exception"
(3) extra flag or info is stored in the reserved bytes of WASMModuleInstance's uint32 reserved[7] if the space is big enough, and add extra APIs if needed. e.g., check whether the exception is a trap, get extra info for exception.

@bashor
Copy link

bashor commented Aug 26, 2024

Nice to see activities around supporting the final version of the EH proposal.

Regarding old vs final version of the proposal:
For Kotlin/Wasm we are generating the old version by default, though supporting both. Our current plan is to keep this default until all major browsers ship the final proposal and the majority of users migrate to those versions.

@bashor
Copy link

bashor commented Aug 26, 2024

WasmEdge, for example, now supports both versions.

@bashor
Copy link

bashor commented Aug 26, 2024

Actually, on our (Kotlin) side, we can consider having a different migration strategy for non-browser targets 🤔

@wenyongh
Copy link
Contributor

@bashor thanks for bringing us so much info about Kotlin! I think another reason of not keeping the current implementation is that the EH spec proposal has changed a lot and it is difficult to maintain the two versions, at least, the code will be very ugly. Since the new version is just kicked off and may need some time to develop and normally we will merge it into branch dev/exce_handling first, I think we can keep the old version in the main branch and test the new version in branch dev/exce_handling for some time. And after the new version is well tested and we have created a new WAMR release (e.g. 2.2.0), we can merge the new version into the main branch and don't maintain the old version again. And after that, if to use the old version, Kotlin can select WAMR old release (e.g. 2.2.0), and if to use new version, Kotlin can select the main branch. @bashor is that ok for you?

@yamt
Copy link
Collaborator

yamt commented Aug 27, 2024

Every Wasm function can return an exception

The new specification indicated that if an exception handler can not be found (uncaught exception) that the exception should be returned to the caller. This implies that every Wasm function can return an exception OR its originally anticipated results, even if that function returns void, it would still be possible for it to return an exception.

afaik, this is not specific to the new spec.
the old one had it as well.
(and our implementation is a bit broken. see #3131)

@yamt
Copy link
Collaborator

yamt commented Aug 27, 2024

My personal opinion is that the exception APIs have been widely used and we had better not changed them, it may impact developer a lot and might make bigger confusing if we change the API. Can we unify traps and exceptions? Not sure what extra info need to be returned for the exception handling proposal, is it just a "uncaught exception" string, or should we distinguish whether the return exception is a trap or an exception and return more for the exception? How about:

(1) keep wasm_runtime_get_exception/wasm_runtime_set_exception API unchanged (2) if an exception of exception handling proposal isn't caught in a function, wasm_runtime_get_exception returns "uncaught exception" (3) extra flag or info is stored in the reserved bytes of WASMModuleInstance's uint32 reserved[7] if the space is big enough, and add extra APIs if needed. e.g., check whether the exception is a trap, get extra info for exception.

you mean "uncaught exception" as a trap, right?
it seems reasonable to me.

i guess we can live without (3) for the time being.
we can add an extra host api to catch/raise exceptions later if necessary.

@ermler
Copy link
Contributor

ermler commented Aug 27, 2024

Every Wasm function can return an exception

The new specification indicated that if an exception handler can not be found (uncaught exception) that the exception should be returned to the caller. This implies that every Wasm function can return an exception OR its originally anticipated results, even if that function returns void, it would still be possible for it to return an exception.

afaik, this is not specific to the new spec. the old one had it as well. (and our implementation is a bit broken. see #3131)

Yes, you are right. In the first implementation we implemented returning an exception from the called function to the calling function (within interpreter). The topmost function could only return a "uncaught exception"-trap to the embedder.
Following the advice from @wenyongh, i made an prototype yesterday, that extends the WAMR-C-API a bit and uses the "reserved[0]" in WASMModule to give a more detailed reason (could be a trap, an exception and possibly stack-supend to support stack switching)

I also looked to the issue #3131 and i am convinced, that we had a misunderstanding in the implementation. The calculculation for the additional space needed for exception seem to be incorrect. Our current activity should give the chance to improve this part.

@bashor
Copy link

bashor commented Aug 27, 2024

@bashor is that ok for you?

@wenyongh yeah, sounds good. We likely will be ready to switch our wasm-wasi target to the new EH by default earlier, e.g. after you release a WAMR version with the new EH support. (For the browser target, we will need to be more conservative.)

BTW, feel free to ping me or file an issue at kotl.in/issue if you need help with testing the new EH support in WAMR, as soon as it's ready.

@wenyongh
Copy link
Contributor

My personal opinion is that the exception APIs have been widely used and we had better not changed them, it may impact developer a lot and might make bigger confusing if we change the API. Can we unify traps and exceptions? Not sure what extra info need to be returned for the exception handling proposal, is it just a "uncaught exception" string, or should we distinguish whether the return exception is a trap or an exception and return more for the exception? How about:
(1) keep wasm_runtime_get_exception/wasm_runtime_set_exception API unchanged (2) if an exception of exception handling proposal isn't caught in a function, wasm_runtime_get_exception returns "uncaught exception" (3) extra flag or info is stored in the reserved bytes of WASMModuleInstance's uint32 reserved[7] if the space is big enough, and add extra APIs if needed. e.g., check whether the exception is a trap, get extra info for exception.

you mean "uncaught exception" as a trap, right? it seems reasonable to me.

yes.

i guess we can live without (3) for the time being. we can add an extra host api to catch/raise exceptions later if necessary.

yes, but @ermler mentioned that he is using reserved[0] to give a more detailed reason and extending the WAMR-C-API, that also sounds good as it provides more functionality.

@wenyongh
Copy link
Contributor

@bashor is that ok for you?

@wenyongh yeah, sounds good. We likely will be ready to switch our wasm-wasi target to the new EH by default earlier, e.g. after you release a WAMR version with the new EH support. (For the browser target, we will need to be more conservative.)

BTW, feel free to ping me or file an issue at kotl.in/issue if you need help with testing the new EH support in WAMR, as soon as it's ready.

Thanks @bashor, your test cases are really helpful for the exception handling feature (and GC feature), we will let you know when the new EH support is basically ready and ask for your help. Thanks in advance.

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 28, 2024

@bashor Awesome, it's great to know that the Kotlin tool chain is compiling to the new EH spec, it would be great if we could use this tool chain to build some example user code (in addition to the specification tests) as we commence the implementation. I might reach out to you to ensure we can setup a Kotlin development environment correctly to do this...

Thank you.

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 30, 2024

We've been chatting to some folks from CMU about how we could help improve our test coverage beyond the spec-tests for this issue. They've some new research angles which sound really interesting, I just want to mention it here - it'll explain why there may be comments, suggestions and feedback from @clegoues and @RaoNikitha .

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 30, 2024

The C++ Tool chain from Emscripten is also available to use. I had a great conversation with @aheejin
who pointed me to the tooling. @aheejin explained that we need to add -sWASM_EXNREF and -fwasm-exceptions at the same time.

If my understanding is correct emcc is, at the moment, compiling first for the old -fwasm-exceptions and then there is a converter to convert this output to the new format.

It sounds like work is on going to add support to LLVM.

But the good news is we've a compiler tool chain which can generate the new exception format.

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 30, 2024

We're in the middle of defining the tasks / open questions we need to undertake, here's the rough sketch:

  • Update Parsing for new format *(extra error message)
  • Define new opcodes
  • Remove old opcode *(extra error message)
  • Re-implement the loader step
  • There will be changes to the CSP, but we need to define that?
  • Where do we store the exception parameters? - Is there a clue from the branch table structure?
  • Re-Implement the try/throw in the classic-interpreter
  • Remove delegate, rethrow from classic-interpreter
  • Updated Validation to cover new ; update the test-spec to cover the new proposal exceptions wabt at the moment doesn't support exception handling at the moment, wasm-tools does....
  • Extend the WAMR API to provide access to the exception data from an "uncaught exception"
  • *(extra error message) - we should probably introduce an specific error message explaining that the old exception handling format is no longer supported.
  • Ensure that we use tag addresses instead of tag index -- thank you @yamt !

@woodsmc
Copy link
Contributor Author

woodsmc commented Aug 30, 2024

Proposal: When the old exception handling implementation is discovered, we should trap this and return an error message which says "unsupported opcode; old style exception handling has been removed. Please use the latest exnref exceptions"

@yamt
Copy link
Collaborator

yamt commented Sep 5, 2024

Proposal: When the old exception handling implementation is discovered, we should trap this and return an error message which says "unsupported opcode; old style exception handling has been removed. Please use the latest exnref exceptions"

it sounds reasonable to me if you meant a load-time error.

@yamt
Copy link
Collaborator

yamt commented Sep 5, 2024

Where do we store the exception parameters? - Is there a clue from the branch table structure?

there are at least two ways to represent exnref.

  • as a real (gc'ed) reference to an exception. in this case, it's easy to make the exception variable-sized and contain any parameters. a downside is that it depends on gc.
  • as a copy-able exception value on the stack. in this case, maybe it's simpler to limit the size of exception parameters so that the exception value still has a fixed size. i did this way in toywasm.

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

No branches or pull requests

5 participants