Skip to content

[proposals] Exception Handling - initial implementation#794

Merged
andreaTP merged 1 commit into
dylibso:mainfrom
andreaTP:eh-again-latest
Mar 27, 2025
Merged

[proposals] Exception Handling - initial implementation#794
andreaTP merged 1 commit into
dylibso:mainfrom
andreaTP:eh-again-latest

Conversation

@andreaTP
Copy link
Copy Markdown
Collaborator

This is an initial implementation of the Exception Handling proposal.
There are rough edges and a few details to be cleared out, but I'm opening early to receive comments.

In particular a review by @SoniEx2 would be appreciated as they implemented it in Wabt. 🙏

Useful links:

I'll comment inline more this implementation tomorrow.

@andreaTP andreaTP requested review from bhelx, electrum and evacchi March 13, 2025 19:00
@electrum
Copy link
Copy Markdown
Collaborator

Do you plan to merge this before the feature is standardized? Are there any target applications using the feature that you're trying to get running?

I read the explainer and skimmed the code. Unless I'm missing something, this seems compatible with the JVM model for exceptions, so this should be fairly straightforward to implement for AOT.

@evacchi
Copy link
Copy Markdown
Collaborator

evacchi commented Mar 14, 2025

Are there any target applications using the feature that you're trying to get running?

some higher-level languages are starting to embrace the EH spec, this would enable using them. To be fair, most of those languages would also require WasmGC support, that would be the next step, but we would still need both one day or another, and EH feels like the appropriate first step.

Unless I'm missing something, this seems compatible with the JVM model for exceptions, so this should be fairly straightforward to implement for AOT.

I was planning to give it a try :)

@andreaTP
Copy link
Copy Markdown
Collaborator Author

Do you plan to merge this before the feature is standardized?

The proposal should be standardized sometime soon, I think it's safe to merge, we can follow up if there are substantial changes.
Additionally, with the proposed implementation there is almost no overhead for "non-EH" wasm execution.

Are there any target applications using the feature that you're trying to get running?

I was targeting running Java compiled to Wasm, but it turns out to be using an older version of the spec 😅 and now we are going to need also WasmGC support.
As @evacchi mentioned, implementing EH seemed a natural next step progression to enable higher level languages (Scala, Kotlin, Java etc.).

I read the explainer and skimmed the code. Unless I'm missing something, this seems compatible with the JVM model for exceptions, so this should be fairly straightforward to implement for AOT.

I think it is going to be even easier in the AOT, as you don't need all the walking up the stack logic that is happening here.

I was planning to give it a try :)

Don't fight 😄 , as always I'm available to help making this happen, but since we have 2 volunteers I'll focus on something else. Beware the EH testsuite is also testing the interaction with tail-call, not a lot of work, but it would be the first step.

@SoniEx2
Copy link
Copy Markdown

SoniEx2 commented Mar 14, 2025

it is worth noting that the tests for exception handling were incomplete last we checked

@evacchi
Copy link
Copy Markdown
Collaborator

evacchi commented Mar 14, 2025

Don't fight 😄

oh sure, if @electrum wants to go at it I won't get in the way ;)

Comment thread runtime/src/main/java/com/dylibso/chicory/runtime/InterpreterMachine.java Outdated
@electrum
Copy link
Copy Markdown
Collaborator

@evacchi I'm not sure if I'd have time to work on this (though it sounds fun), so feel free to give it a go. I'm happy to review or chat about it.

The tail-call interaction seems basically impossible for AOT, given that the JVM doesn't support that, so we probably need to permanently skip those tests.

@evacchi
Copy link
Copy Markdown
Collaborator

evacchi commented Mar 14, 2025

The tail-call interaction seems basically impossible for AOT, given that the JVM doesn't support that, so we probably need to permanently skip those tests

I don't know if the tests make any real assumption on it being actually a tail call (it's probably more about validation), I think the instructions (return_call etc) will be simply implemented as regular call+return @andreaTP knows more :)

@andreaTP
Copy link
Copy Markdown
Collaborator Author

Re: tail call
I'd be happy to merge a PR that implements the opcodes without actual tail call, I don't think the testsuite will complain.
At the end of the day we can always improve from that baseline.

Comment thread wasm/src/main/java/com/dylibso/chicory/wasm/types/ValueType.java
andreaTP added a commit that referenced this pull request Mar 17, 2025
I found this when working on #794 , and it's extremely confusing, so
separating the fix to get it merged early.
@andreaTP
Copy link
Copy Markdown
Collaborator Author

I updated the PR to get also ref_null.wast passing, happy to receive feedback and review 🙏

@andreaTP
Copy link
Copy Markdown
Collaborator Author

I just fixed the last loose end in the Validator I initially left.

Now the implementation is as clean as it can be given my current understanding of EH in Wasm.
I'd love to have a final review and eventually merge, cc. @electrum @evacchi 🙏

Copy link
Copy Markdown
Collaborator

@evacchi evacchi left a comment

Choose a reason for hiding this comment

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

we had an offline discussion, we believe the design can be improved, but it's fine for now, we can iterate

@andreaTP
Copy link
Copy Markdown
Collaborator Author

Going to merge, thanks everyone for the comments and feedback!
For any additional concern, please file an issue and we fix on top.

@andreaTP andreaTP merged commit 9138e97 into dylibso:main Mar 27, 2025
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.

5 participants