exception-handling icon indicating copy to clipboard operation
exception-handling copied to clipboard

[js-api] Add stack trace support and preserve identity of JS exception values passing through WebAssembly.

Open Ms2ger opened this issue 3 years ago • 8 comments

Fixes #189. Fixes #201.

Ms2ger avatar Aug 25 '22 15:08 Ms2ger

  1. "Call an exported function" is calling a wasm function from js; "Create a host function" is calling a js function from wasm.
  2. Consider the JS function f defined as function f() { throw {} } - that is, f throws an exception value that is not a WebAssembly.Exception. The question is, if f is called from webassembly, whether the webassembly code should be able to extract an externref from that exception. This is not yet supported, but I'm thinking that it could be deferred to a future version of wasm.
  3. #189 is not solved yet.

However, I'm realizing I don't fully understand how traceStack is supposed to work. The explainer has a line "traceStack serves as a request to the WebAssembly engine to attach a stack trace", but I don't know what that's supposed to look like. We pass the option to the JS Exception constructor and capture a stack at that point, but that doesn't change the behavior of the wasm engine. Or is the expectation that in JS code called from wasm, an Exception object will have a stack trace that either contains only JS frames or interleaved JS and wasm frames?

Ms2ger avatar Sep 16 '22 14:09 Ms2ger

Thanks for making me think through #189; this PR now fixes that as well.

Ms2ger avatar Sep 20 '22 10:09 Ms2ger

@dschuff Do you have any comments?

aheejin avatar Sep 22 '22 15:09 aheejin

Thinking about the exception identity here... IIUC: When an exception is originated from wasm:

  1. the first time it propagates into JS opaqueData is null (so it lands in 12.2 of "call an Exported Function") and its tag and payload get wrapped in a new WebAssembly.Exception and thrown.
  2. If it propagates back into wasm, it will land in 3.9.2 of "create a host function". The type and payload are taken out of the object and a reference to the object itself goes into opaqueData.
  3. If it then propagates back into JS again, it will land in 12.1, and opaqueData (the original wrapping from 1) gets thrown.

When an exception is originated from JS:

  1. the first time it propagates into wasm, it will land in 3.9.3 of "create a host function", the type will be the JS tag, the payload will be empty, and opaqueData will be the original thrown object.
  2. If it propagates back into JS, it will be like 3 above (landing in 12.1 of "call an exported function")

One thing I can't recall from our discussion: when exceptions propagate into wasm (i.e. where we say "To throw" in section 4.7.3) it says "invoke the catch block with payload and opaqueData". What does that mean? Which of the payload and/or opaqueData gets actually pushed onto the wasm value stack in the catch block?

dschuff avatar Sep 23 '22 00:09 dschuff

Thinking about the exception identity here... IIUC:

All this matches my understanding.

One thing I can't recall from our discussion: when exceptions propagate into wasm (i.e. where we say "To throw" in section 4.7.3) it says "invoke the catch block with payload and opaqueData". What does that mean? Which of the payload and/or opaqueData gets actually pushed onto the wasm value stack in the catch block?

I believe the goal is:

  • the opaque data is not exposed to wasm code (in fact, if your wasm implementation doesn't interoperate with JS, the opaque data can be ignored entirely)
  • catch (assuming the tag matches) pushes the payload onto the stack and keeps track of the opaque data separately
  • rethrow grabs the opaque data from wherever catch put it, so that catch+rethrow is a no-op

I think it would be best if this was clarified in the core spec, but I haven't had time to dig into where exactly that would go.

Ms2ger avatar Sep 23 '22 12:09 Ms2ger

The current formal reduction rule for rethrow in the core spec is:

caught{a val*} B^l[rethrow l] end
  ↪ caught{a val*} B^l[val* (throw a)] end

So this does a fresh throw, which I think can't be helped in the formal spec because formal spec doesn't have a concept of opaque data. The core spec only knows about the tag and the payload, so there's no other way to express the functionality anyway.

But should we say something about the core spec's rethrow here in the JS API, because apparently a fresh throw within the wasm is not what we want? This might sound weird, but not sure if there's any other way around this. cc @ioannad

aheejin avatar Sep 28 '22 01:09 aheejin

@Ms2ger Gentle ping for my latest question :)

aheejin avatar Oct 06 '22 15:10 aheejin

The current formal reduction rule for rethrow in the core spec is:

caught{a val*} B^l[rethrow l] end
  ↪ caught{a val*} B^l[val* (throw a)] end

So this does a fresh throw, which I think can't be helped in the formal spec because formal spec doesn't have a concept of opaque data. The core spec only knows about the tag and the payload, so there's no other way to express the functionality anyway.

But should we say something about the core spec's rethrow here in the JS API, because apparently a fresh throw within the wasm is not what we want? This might sound weird, but not sure if there's any other way around this. cc @ioannad

Indeed, according to the current formal spec, caught does only store the tag and payload. However, I consider throw tagidx a fresh throw, not the administrative throw tagaddr, which could be seen as just initiating the throwing process internally.

I would have to think of a different way to address opaque data in the core spec, if that is even necessary. Any thoughts or ideas on this, @rossberg ?

ioannad avatar Oct 21 '22 10:10 ioannad

Apologies for the delay - I haven't had much wasm time recently. I'd prefer landing this PR as is, and I'll work with @ioannad to update the core spec to clarify how the opaque data is passed through.

Ms2ger avatar Nov 04 '22 12:11 Ms2ger

The core spec doesn't have any notion of exception identity – we removed that intentionally, otherwise we could have kept exnref. To model such an identity, you'll basically have to introduce exnref internally, but I'm not sure how desirable it is to reintroduce it through the backdoor.

So with the current state of affairs, I think it rather ought to be hand-waved in the JS API spec somehow.

(Of course, I'd be all for reintroducing exnref properly, which would allow to significantly simplify the proposal and solve the known expressiveness problems. :) )

rossberg avatar Nov 09 '22 10:11 rossberg

I'm going to merge this as is, as it's a significant improvement over the status quo, and I'll file an issue on either making the changes to the core spec or hand-waving in the js-api spec.

Ms2ger avatar Nov 09 '22 12:11 Ms2ger