bytecodealliance / wasm-micro-runtime

WebAssembly Micro Runtime (WAMR)
Apache License 2.0
4.79k stars 606 forks source link

Updating Exception Handling in WAMR #3753

Open woodsmc opened 3 weeks ago

woodsmc commented 3 weeks ago

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:

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 commented 3 weeks ago

Some useful resources for the discussion:

woodsmc commented 3 weeks ago

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

wenyongh commented 2 weeks ago

@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. https://github.com/bytecodealliance/wasm-micro-runtime/blob/e8c2952bf959f6df81972fc469a4357f04ee629a/core/iwasm/interpreter/wasm_runtime.h#L433

bashor commented 2 weeks ago

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 commented 2 weeks ago

WasmEdge, for example, now supports both versions.

bashor commented 2 weeks ago

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

wenyongh commented 2 weeks ago

@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 commented 2 weeks ago
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 https://github.com/bytecodealliance/wasm-micro-runtime/issues/3131)

yamt commented 2 weeks ago

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.

https://github.com/bytecodealliance/wasm-micro-runtime/blob/e8c2952bf959f6df81972fc469a4357f04ee629a/core/iwasm/interpreter/wasm_runtime.h#L433

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 commented 2 weeks ago
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 commented 2 weeks ago

@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 commented 2 weeks ago

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. https://github.com/bytecodealliance/wasm-micro-runtime/blob/e8c2952bf959f6df81972fc469a4357f04ee629a/core/iwasm/interpreter/wasm_runtime.h#L433

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 commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

woodsmc commented 2 weeks ago

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 commented 1 week ago

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 commented 1 week ago

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.