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.

Sign up for GitHub

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

Jump to bottom

Update JS API to better specify opaqueData exception identity #250

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Update JS API to better specify opaqueData exception identity #250

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b87068d
Update JS API to better specify opaqueData exception identity
dschuff Feb 15, 2023
af87b72
review
dschuff Feb 16, 2023
b6df4c2
clarify language about null identity from wasm throw
dschuff Mar 3, 2023
38bbc27
fix typo
dschuff Mar 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions document/js-api/index.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,7 +798,7 @@ Each {{Table}} object has a \[[Table]] internal slot, which is a [=table address
The <dfn constructor for="Table">Table(|descriptor|, |value|)</dfn> constructor, when invoked, performs the following steps:
1. Let |elementType| be [=ToValueType=](|descriptor|["element"]).
1. If |elementType| is not a [=reftype=],
1. [=Throw=] a {{TypeError}} exception.
1. Throw a {{TypeError}} exception.
aheejin marked this conversation as resolved.
Show resolved Hide resolved
1. Let |initial| be |descriptor|["initial"].
1. If |descriptor|["maximum"] [=map/exists=], let |maximum| be |descriptor|["maximum"]; otherwise, let |maximum| be empty.
1. If |maximum| is not empty and |maximum| &lt; |initial|, throw a {{RangeError}} exception.
Expand Down Expand Up @@ -1044,10 +1044,12 @@ This slot holds a [=function address=] relative to the [=surrounding agent=]'s [
1. [=list/Append=] [=ToWebAssemblyValue=](|arg|, |t|) to |args|.
1. Set |i| to |i| + 1.
1. Let (|store|, |ret|) be the result of [=func_invoke=](|store|, |funcaddr|, |args|).
1. Note: The expectation is that [=func_invoke=] will be updated to return (|store|, <var ignore>val</var>* | [=error=] | (exception |exntag| |payload| |opaqueData|)).
1. Note: The expectation is that [=func_invoke=] will be updated to return (|store|, <var ignore>val</var>* | [=error=] | (exception |exntag| |payload|)). XXX (we still need to update document/core/appendix/embedding.rst for this)

1. Set the [=surrounding agent=]'s [=associated store=] to |store|.
1. If |ret| is [=error=], throw an exception. This exception should be a WebAssembly {{RuntimeError}} exception, unless otherwise indicated by <a href="#errors">the WebAssembly error mapping</a>.
1. If |ret| is exception |exntag| |payload| |opaqueData|, then
1. If |ret| is exception |exntag| |payload|, then
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
Copy link
Member

Choose a reason for hiding this comment

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

How can we retrieve this opaqueData from payload? This is what I was asking about in #248 (comment). Also what exactly is "the returned exception"? Is that opaqueData or a combination of a tag and a payload?

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO "payload" is a separate concept from identity. Different exceptions (with different identity) can have the same payload, so the identity/opaqueData can't be uniquely identified from the payload. As written here, the exact mechanism used to track the identity is unspecified; it just says that exceptions have a unique identity and says where it needs to be created and preserved.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Let |opaqueData| be the [=externref=] assocated with the returned exception.
1. Let |opaqueData| be the [=externref=] associated with the returned exception.

Though I think this wording is a bit insufficient. If the exception originates from a Wasm throw (and has not yet escaped to JS), then there is no externref associated with it yet, and you are actually have to create that association at this point. So I believe you need to distinguish this case (which should be the common case).

Copy link
Member Author

Choose a reason for hiding this comment

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

The change down on line 1373 attempts to get at this. Those lines are the ones that really modify the behavior specified in the core spec, so it's not 100% obvious where they should go.
The rule that says "fresh exceptions created by wasm throw get null identities" could reasonably go here I think; but it's less obvious where the "identities propagate to JS" and "rethrows preserve identity" rules go, if not there.

I modified 1373 to try to make it a little more clear.

1. If |opaqueData| is not [=ref.null=] [=externref=],
1. Let « [=ref.extern=] |externaddr| » be |opaqueData|.
Copy link
Member

Choose a reason for hiding this comment

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

Does this (preexisting) line mean we have the reverse mapping from opaqueData (or object) to externaddr..?

Copy link
Member Author

Choose a reason for hiding this comment

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

My interpretation of the preexisting line is that the core spec was expected to be updated to say that func_invoke (from the core spec) returns the opaqueData (and is also expected to say where opaqueData comes from). There is no need for a reverse mapping because opaqueData is the externaddr and is passed directly into this step. I think of this like as basically just being a cast from opaqueData (a construct that was to be defined in the core spec) to externaddr (a construct that is used in this spec as a key in the map).

1. Throw the result of [=retrieving an extern value=] from |externaddr|.
Expand Down Expand Up @@ -1081,7 +1083,7 @@ Note: Exported Functions do not have a \[[Construct]] method and thus it is not
1. Otherwise, if |resultsSize| is 1, return « [=?=] [=ToWebAssemblyValue=](|ret|, |results|[0]) ».
1. Otherwise,
1. Let |method| be [=?=] [=GetMethod=](|ret|, [=@@iterator=]).
1. If |method| is undefined, [=throw=] a {{TypeError}}.
1. If |method| is undefined, throw a {{TypeError}}.
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason as above.

1. Let |values| be [=?=] [=IterableToList=](|ret|, |method|).
1. Let |wasmValues| be a new, empty [=list=].
1. If |values|'s [=list/size=] is not |resultsSize|, throw a {{TypeError}} exception.
Expand Down Expand Up @@ -1113,7 +1115,7 @@ Note: Exported Functions do not have a \[[Construct]] method and thus it is not
1. Let |type| be the [=JavaScript exception tag=].
1. Let |payload| be « ».
1. Let |opaqueData| be [=ToWebAssemblyValue=](|v|, [=externref=])
1. [=WebAssembly/Throw=] with |type|, |payload| and |opaqueData|.
1. [=WebAssembly/Throw=] with |type|, |payload|, associating |opaqueData| with the thrown exception.
Copy link
Member

Choose a reason for hiding this comment

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

How? Also I'm not sure what exactly is "the thrown exception". Is that a combination of a tag and a payload? Do we have an object that represents that combination which can be used as a key to some map? (And where do we have that map? Don't we only have a map of externaddr -> object?)

Copy link
Member Author

Choose a reason for hiding this comment

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

This "association" is defined a little more down in the definition of Throw below.
(but really it's the hand-wavy bit).
But I just realized I wrote the definition of throw as taking opaqueData as an argument, so there's no need to use the term "association" or "thrown" exception here. It should just say
[=WebAssembly/Throw=] with |type|, |payload|, and |opaqueData| as it did before, and then the definition down below should say what the association means. I'll make that update here, and put more replies down there.

1. Otherwise, return |result|.\[[Value]].
1. Let |store| be the [=surrounding agent=]'s [=associated store=].
1. Let (|store|, |funcaddr|) be [=func_alloc=](|store|, |functype|, |hostfunc|).
Expand Down Expand Up @@ -1361,10 +1363,14 @@ For any [=associated store=] |store|, the result of

To <dfn for=WebAssembly>throw</dfn> with a [=tag address=] |type|, a matching [=list=] of WebAssembly values |payload|, and an [=externref=] |opaqueData|, perform the following steps:
Copy link
Member

Choose a reason for hiding this comment

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

Why is the tag address referred to as a "type" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll fix the tag/type wording in a separate change.


1. Associate |opaqueData| with the thrown exception.
Copy link
Member Author

Choose a reason for hiding this comment

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

But by the "thrown exception" I really mean the unique identity that the exception has as it propagates through the frames. The definition down below is what says that the identity needs to be preserved across a rethrow. Just a combination of tag+payload wouldn't be enough, because there could be more than one identical tag+payload (e.g. if an exception was thrown from JS, caught and swallowed in wasm, and then a fresh exception thrown from inside wasm with the same numeric payload; you would want to be able to tell the difference, it would have no stack trace, etc).
As you say, we only have a map of externaddr -> object, but because tag+payload isn't unique we can't have a mapping in this spec from tag+payload to externaddr (or any other form of unique identity).

Instead the implementation needs to create some kind of association between the propagating exception's identity and the opaqueData (i.e. its identity as defined in this spec). It does so in an unspecified way, that's the hand-wavy part: we just assume that this identity exists and can be tracked, and we specify that the identity is maintained across a rethrow, and that fresh throws start with a null identity.

1. Unwind the stack until reaching the *catching try block* given |type|.
Copy link
Member

Choose a reason for hiding this comment

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

This seems to assume that there is a catch block to handle it, but of course that does not need to be the case, and it is not when the exception actually escapes to JS.

Copy link
Member Author

@dschuff dschuff Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed. Ideally this text should probably say something like "create an exception and unwind the stack" according to the language of the core spec, and "do whatever the core spec says should be done with exceptions"; i.e. unwind until the exception is caught or propagate it back to the embedder. Is there a particular spot in the core spec that defines that creation?

Copy link
Member Author

@dschuff dschuff Mar 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe what we actually need to do is define some kind of interface in the embedding appendix for that (that appendix at least needs to be updated to say what happens when an invoked function raises/propagates an exception). The question still remains though, since that interface will itself need to say what happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

#268 is the beginning of what I meant by the above comment; although I noticed that it doesn't say anything about calls from the instance into the embedder, so there may not be anything relevant to the case we're talking about here.

1. Invoke the catch block with |payload| and |opaqueData|.
1. Invoke the catch block with |payload|.

Note: This algorithm is expected to be moved into the core specification.
Note: The identity represented by |opaqueData| is not exposed to core wasm code, but remains with the exception as it propagates through the wasm frames on the stack.
dschuff marked this conversation as resolved.
Show resolved Hide resolved
If the exception propagates back to JavaScript (by returning from [=func_invoke=]), the same |opaqueData| is returned with it.
If the exception is caught and rethrown with the rethrow instruction, the identity remains the same.
Exceptions that originate from core WebAssembly (using the WebAssembly throw instruction) have a |ref.null| identity returned from [=func_invoke=]

</div>

Expand Down