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

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

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Aug 25, 2022

Fixes #189.
Fixes #201.

@Ms2ger Ms2ger requested a review from aheejin August 25, 2022 15:48
Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry for the late reply; I was OOO.

Sorry for the many questions; I am not very familiar with the JS API text.

Other questions:

  1. We have sections called "Call an exported function" and "Create a host function". Does "Call an exported function" happen on the JS side and does "Create a host function" on the Wasm side?

  2. Another question about the existing issue:

Issue: Should it be possible for `catch <JS-exception-tag>` to extract the payload from an exception with this tag?

Can we consider this resolved given that we have an JS API that can create WebAssembly.Exception?

  1. Does this also address Preserve identity of exceptions from JS when passing through wasm #189, or does it need to be separately addressed?

document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
document/js-api/index.bs Show resolved Hide resolved
document/js-api/index.bs Outdated Show resolved Hide resolved
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Sep 16, 2022

  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. Preserve identity of exceptions from JS when passing through wasm #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 Ms2ger changed the title [js-api] Add stack trace support. [js-api] Add stack trace support and preserve identity of JS exception values passing through WebAssembly. Sep 20, 2022
@Ms2ger
Copy link
Contributor Author

Ms2ger commented Sep 20, 2022

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

Copy link
Member

@aheejin aheejin left a comment

Choose a reason for hiding this comment

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

  1. Create a new exported function
  2. Call an exported function
  3. Create a host function
  4. Run a host function

We only handle exceptions in 2 and 3. I think I don't have a clear concept of the difference between "create" and "call/run"; the steps look kind of similar to me. Why do we only need to handle "call" in case of an exported function and "create" in case of a host function?

1. Let |payload| be [=ToWebAssemblyValue=](|v|, [=externref=]).
1. [=WebAssembly/Throw=] with |type| and |payload|.
1. Let |payload| be « ».
1. Let |opaqueData| be [=ToWebAssemblyValue=](|v|, [=externref=])
Copy link
Member

Choose a reason for hiding this comment

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

It looks this wraps the whole value itself as opaqueData, correct? Where does the stack trace belong then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stack trace just stays in a slot on the Exception JS object

Comment on lines 1109 to +1110
1. Let |type| be the [=JavaScript exception tag=].
1. Let |payload| be [=ToWebAssemblyValue=](|v|, [=externref=]).
1. [=WebAssembly/Throw=] with |type| and |payload|.
1. Let |payload| be « ».
Copy link
Member

@aheejin aheejin Sep 21, 2022

Choose a reason for hiding this comment

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

Do we not need to handle |v|.\[[Stack]] in here and also in line 1106-1107?

1. If |exntag| is the [=JavaScript exception tag=], then
1. Let « [=ref.extern=] |externaddr| » be |payload|.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |opaqueData| is not [=ref.null=] [=externref=],
Copy link
Member

Choose a reason for hiding this comment

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

In which case is this ref.null? When it is thrown from wasm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

Copy link
Contributor Author

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

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

The split between call/run/create is not particularly meaningful. In the "host function" case, it exists because there's no well-defined try-finally construct in prose, so "run a host function" exists as an algorithm you can early-return from if there's an exception, and then "create a host function" can put the (exception-or-normal) result in a variable. I think we could merge "call an Exported Function" into "a new Exported function" now that ECMAScript uses abstract closures for CreateBuiltinFunction (tc39/ecma262#1894); the current split predates that (WebAssembly/spec@d15cabb).

1. If |exntag| is the [=JavaScript exception tag=], then
1. Let « [=ref.extern=] |externaddr| » be |payload|.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |opaqueData| is not [=ref.null=] [=externref=],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

1. Let |payload| be [=ToWebAssemblyValue=](|v|, [=externref=]).
1. [=WebAssembly/Throw=] with |type| and |payload|.
1. Let |payload| be « ».
1. Let |opaqueData| be [=ToWebAssemblyValue=](|v|, [=externref=])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The stack trace just stays in a slot on the Exception JS object

@aheejin
Copy link
Member

aheejin commented Sep 22, 2022

@dschuff Do you have any comments?

@dschuff
Copy link
Member

dschuff commented Sep 23, 2022

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?

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Sep 23, 2022

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.

@aheejin
Copy link
Member

aheejin commented Sep 28, 2022

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
Copy link
Member

aheejin commented Oct 6, 2022

@Ms2ger Gentle ping for my latest question :)

@ioannad
Copy link
Collaborator

ioannad commented Oct 21, 2022

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 ?

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Nov 4, 2022

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.

@rossberg
Copy link
Member

rossberg commented Nov 9, 2022

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. :) )

@Ms2ger
Copy link
Contributor Author

Ms2ger commented Nov 9, 2022

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 Ms2ger merged commit 423f261 into main Nov 9, 2022
@Ms2ger Ms2ger deleted the stack branch November 9, 2022 12:41
aheejin added a commit to aheejin/exception-handling that referenced this pull request Jan 6, 2023
This basically does the "handwaving" we discussed in WebAssembly#218. I think
adding a concept of a backing store through backdoors in the core spec
is not very feasible, so this is what we can do practically at this
point.

Hopefully resolves WebAssembly#242.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants