WebAssembly / exception-handling

Proposal to add exception handling to WebAssembly
https://webassembly.github.io/exception-handling/
Other
162 stars 35 forks source link

Update JS API for exnref #301

Closed dschuff closed 7 months ago

dschuff commented 7 months ago

This change updates exception object allocation, initialization, and construction for exnref; as well as dealing with exception propagation from invoking exported functions; throwing exceptions from host functions into wasm; and wrapping and unwrapping JS exceptions as they propagate into and out of wasm.

It depends on the embedding interfaces added in #268.

eqrion commented 7 months ago

I don't see this in this PR or the overview, but is it intended for exn to be passable to/from JS through params/results, or be like v128 and cause TypeError's?

dschuff commented 7 months ago

I don't see this in this PR or the overview, but is it intended for exn to be passable to/from JS through params/results, or be like v128 and cause TypeError's?

When Andreas updated the core spec he also did a minimal update to this spec that made it the latter (see e.g. here or in the getters/setters for table elements and globals). In this PR when exceptions are thrown or propagate from wasm into JS, they end up wrapped in a WebAssembly.Exception object (that's line 1023 in this patch) just as they are in phase 3 exceptions. I'm not sure what a concrete use case would be for passing/returning but it seems slightly weird and asymmetric that you can get any exception instance out to JS by throwing it, but not by returning it.

rossberg commented 7 months ago

It is intentional. I can't find the original discussion right now, but being able to pass exception packages as first-class values to JS would have nasty implications, such as:

Conceptually, its better to think of and exnref as not the exception instance being thrown, but a "pair" of an exception instance that's been caught plus abstract context information for rethrowing it that a Wasm engine might need internally. As such it is a different kind of object from the exception instance and entirely Wasm-internal.

eqrion commented 7 months ago

Okay, that was my understanding as well, but wanted to make sure. SpiderMonkey throws a TypeError in ToJSValue.

dschuff commented 7 months ago

I think this PR is now reasonably correct and self-contained according to the description in OP. I think it correctly (although may not yet completely) implements what we discussed in #282. (I had forgotten about that thread, we can move further discussion about the desired value semantics back there). So I think it probably makes sense to finish this review and land it, to keep PRs of a reasonable size.

dschuff commented 7 months ago

But this algorithm is invoked from another one that already describes Wasm semantics, not host semantics. So we're morally "inside Wasm". So what I'd suggest is to say something like "execute the Wasm instructions (ref.exn |address|) (throw_ref). And with my other comment there is only one place left where this is invoked, so I'd just inline that step there and remove this algorithm.

This is done too, but since the lines it was attached to are now gone, github won't let me reply...

dschuff commented 7 months ago

I'm going to go ahead and land this so I can rebase #269 and do some more cleanups. @aheejin let me know if you have more comments, we can update as needed.