-
Notifications
You must be signed in to change notification settings - Fork 35
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 specification. #86
Conversation
This is based on what I understand the Chromium behaviour to be; it's not clear to me if that is the current consensus. Probably this will need some text changes after the changes to |
Any more thoughts on this, anyone? |
Could you split this PR into adding the existing base JS API and the modified part? It is hard to see the difference related to exceptions. |
The exceptions-related change is in the last commit. |
Thank you! But I still think splitting this is worth it, because it's possible we end up adding more commits to this, in which case it is hard to see only the related changes later. What do you think? @mstarzinger |
+1 on splitting out the unmodified import of of the js-api documents. Those should be uncontroversial and we could land them right away. All further delta are then easier to see and review I think. |
58675fc
to
1fe9cff
Compare
I've rebased and updated the PR to what I think the behaviour is; it's not very formal, but that'll need to wait until the core spec exists. |
I'm very sorry for long radio silence 😢 I think there are some issues to be resolved (regarding traps and such) before we finalize the JS API. Does this PR describe the current behavior of V8? If so, it is likely subject to change, how about waiting on landing this? I opened #89 to discuss some of the issues. |
e841f94
to
789acfa
Compare
…embly#86) * Fix expected trap messages. The spec interpreter says "element segment dropped", rather than "elements segment dropped". * Fix "zero len, but dst offset out of bounds" test. Fix this test to test what it's comment says it's testing. * Add more tests for zero-length operations. * Update the Overview text to reflect the zero-length at-the-end semantics.
… (WebAssembly#87) Change the test generators to use `ref.func` and remove `passive`. At some point we'll want to remove the generators, but for let's try to maintain them.
114f619
to
0326ded
Compare
Is this up-to-date with the current spec? (It doesn't look like it) Are you planning to update this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the two possible changes suggested, it LGTM.
Co-authored-by: Ioanna M Dimitriou H <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I wasn't able to review this at the time of merging! I'm not very familiar with bikeshed and WebIDL syntax, so I'm trying to understand the current EH JS API as written. Here are some questions:
1. If |result|.\[[Type]] is <emu-const>throw</emu-const>, then: | ||
1. If |v| [=implements=] {{RuntimeException}}, | ||
1. Let |type| be |v|.\[[Type]]. | ||
1. Let |payload| be |v|.\[[Payload]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Where is this
[[Payload]]
defined? I can see[[Value]]
,[[Type]]
, and[[Target]]
in completion record specification type doc in ECMAScript spec but not[[Payload]]
. But I also see this note:
exception-handling/document/js-api/index.bs
Line 1043 in ca2b715
1. Note: The expectation is that [=func_invoke=] will be updated to return (|store|, <var ignore>val</var>* | [=error=] | (exception |exntag| |payload|)).
So what should we change to reflect this change? - Should we add that to the ECMAScript spec ourselves?
- As we discussed in Preserve identity of exceptions from JS when passing through wasm #189 we need to preserve the identity of exceptions by attaching an
externref
. Should we extendfunc_invoke
's return value from
(|store|, <var ignore>val</var>* | [=error=] | (exception |exntag| |payload|)
to
(|store|, <var ignore>val</var>* | [=error=] | (exception |exntag| |payload| |stack|)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have time today to page #189 back in, but I'll try to work on it next week.
for: externtype | ||
text: tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this externaltype/tag
special and not just
text:tag
? And because they are differently defined they are used in different ways too:
externtype/table
:
exception-handling/document/js-api/index.bs
Line 402 in ca2b715
1. If |externtype| is of the form [=table=] <var ignore>tabletype</var>, |
externtype/tag
:exception-handling/document/js-api/index.bs
Line 407 in ca2b715
1. If |externtype| is of the form [=externtype/tag=] |attribute| <var ignore>functype</var>, |
I think either way is fine, but do you think it'd be better to make them consistent? The same for external value/tag
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically namespacing - I don't recall if I had an actual conflict with "tag" or if I just thought it made more sense. I'd be happy to change the others as well, but I'd rather do that in the main spec repo directly.
1. If |externtype| is of the form [=externtype/tag=] |attribute| <var ignore>functype</var>, | ||
1. Assert: |attribute| is [=tagtype/attribute/exception=]. | ||
1. If |v| does not [=implement=] {{Tag}}, throw a {{LinkError}} exception. | ||
1. Let |tagaddr| be |v|.\[[Address]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which address is this? For tables and globals, it says |v|.\[[Table]]
and |v|.[[Global]]
. Is there a set of values that v
contains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the field set in "create a Tag object" algorithm below. The supported fields depend on the interface that the value implements, which is checked in the step above.
1. Let « [=ref.extern=] |externaddr| » be |payload|. | ||
1. Throw the result of [=retrieving an extern value=] from |externaddr|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these steps? Aren't these two steps cancelling each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand your question. The goal of the "JavaScript exception tag" check is to ensure an exception thrown from js → wasm → js maintains its identity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I think I was confused. I thought you apply ref.extern
to the returned payload; I guess this means if the payload is the form « [=ref.extern=] |externaddr| »
, then we retrieve an extern value from externaddr
. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, exactly. If you have thoughts on how to make this destructuring assignment pattern more readable, I'd love to hear them.
urlPrefix: https://heycam.github.io/webidl/; spec: WebIDL | ||
type: dfn | ||
text: create a namespace object; url: create-a-namespace-object | ||
urlPrefix: https://webassembly.github.io/js-types/js-api/; spec: WebAssembly JS API (JS Type Reflection) | ||
type: abstract-op; text: FromValueType; url: abstract-opdef-fromvaluetype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this FromValueType
? I can't find it in https://webassembly.github.io/js-types/js-api/#abstract-opdef-fromvaluetype. Also ToValueType
is defined within this file; then why is only this method from the type reflection proposal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the publishing is broken - WebAssembly/js-types#23. js-types only needs one direction, but it seems risky to just copy/paste. I can turn the dependency in the other direction if that turns out to be helpful.
To <dfn>create a RuntimeException object</dfn> from a [=tag address=] |tagAddress| and [=list=] | ||
of WebAssembly values |payload|, perform the following steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are "create an Exception object" algorithm and "new Exception(exceptionTag, payload)
constructor steps" algorithm different?
exception-handling/document/js-api/index.bs
Lines 1272 to 1283 in ca2b715
To <dfn>create an Exception object</dfn> from a [=tag address=] |tagAddress| and [=list=] | |
of WebAssembly values |payload|, perform the following steps: | |
1. Let |store| be the [=surrounding agent=]'s [=associated store=]. | |
1. Let |types| be [=tag_parameters=](|store|, |tagAddress|). | |
1. Assert: |types|'s [=list/size=] is |payload|'s [=list/size=]. | |
1. [=list/iterate|For each=] |value| and |resultType| of |payload| and |types|, paired linearly, | |
1. Assert: |value|'s type matches |resultType|. | |
1. Let |exception| be a [=new=] {{Exception}}. | |
1. Set |exception|.\[[Type]] to |tagAddress|. | |
1. Set |exception|.\[[Payload]] to |payload|. | |
1. Return |exception|. |
exception-handling/document/js-api/index.bs
Lines 1289 to 1301 in ca2b715
The <dfn constructor for=Exception | |
lt="Exception(exceptionTag, payload)">new Exception(|exceptionTag|, |payload|)</dfn> | |
constructor steps are: | |
1. Let |store| be the [=surrounding agent=]'s [=associated store=]. | |
1. Let |types| be [=tag_parameters=](|store|, |exceptionTag|.\[[Address]]). | |
1. If |types|'s [=list/size=] is not |payload|'s [=list/size=], | |
1. Throw a {{TypeError}}. | |
1. Let |wasmPayload| be « ». | |
1. [=list/iterate|For each=] |value| and |resultType| of |payload| and |types|, paired linearly, | |
1. [=list/Append=] ? [=ToWebAssemblyValue=](|value|, |resultType|) to |wasmPayload|. | |
1. Set **this**.\[[Type]] to |exceptionTag|.\[[Address]]. | |
1. Set **this**.\[[Payload]] to |wasmPayload|. |
Why do they exist separately?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"create an Exception object" is when you already have the wasm values; "new Exception" is the js-side constructor called from webidl. I don't see a super obvious way to share more code between them, unfortunately.
1. Assert: |types|'s [=list/size=] is |payload|'s [=list/size=]. | ||
1. [=list/iterate|For each=] |value| and |resultType| of |payload| and |types|, paired linearly, | ||
1. Assert: |value|'s type matches |resultType|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we assert for invariants, but in the constructor algorithm below we throw. What's the difference?
1. Assert: |value|'s type matches |resultType|. | ||
1. Let |exception| be a [=new=] {{RuntimeException}}. | ||
1. Set |exception|.\[[Type]] to |tagAddress|. | ||
1. Set |exception|.\[[Payload]] to |payload|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the constructor algorithm below, we run ToWebAssemblyValue
on each payload value, but here we do not. What's the difference?
For any [=associated store=] |store|, the result of | ||
[=tag_parameters=](|store|, [=JavaScript exception tag=]) must be « [=externref=] ». | ||
|
||
Issue: Should it be possible for `catch <JS-exception-tag>` to extract the payload from an exception with this tag? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should it be not possible? Wasm's catch
wouldn't be able to examine the internals of the externref
payload, but that's true for all externref
s, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly either way, but to support it, we need to expose the tag to core wasm somehow, right? I'm a bit fuzzy on how that would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the only way wasm can catch JS exceptions is to import the JS exception tag. Can we define the tag in the JS side and make wasm import it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ping in case this was missed :) Do you think this would work?
To <dfn for=WebAssembly>throw</dfn> with a [=tag address=] |type| and matching [=list=] of WebAssembly values |payload|, perform the following steps: | ||
|
||
1. Unwind the stack until reaching the *catching try block* given |type|. | ||
1. Invoke the catch block with |payload|. | ||
|
||
Note: This algorithm is expected to be moved into the core specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this specific to JS exceptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is supposed to be basically equivalent to the throw instruction in core wasm. The core spec wasn't finished when I wrote this, so I put this here so I had an api to use in the rest of the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reply, I hope this clarifies. Let me know if anything is still confusing.
for: externtype | ||
text: tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's basically namespacing - I don't recall if I had an actual conflict with "tag" or if I just thought it made more sense. I'd be happy to change the others as well, but I'd rather do that in the main spec repo directly.
urlPrefix: https://heycam.github.io/webidl/; spec: WebIDL | ||
type: dfn | ||
text: create a namespace object; url: create-a-namespace-object | ||
urlPrefix: https://webassembly.github.io/js-types/js-api/; spec: WebAssembly JS API (JS Type Reflection) | ||
type: abstract-op; text: FromValueType; url: abstract-opdef-fromvaluetype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the publishing is broken - WebAssembly/js-types#23. js-types only needs one direction, but it seems risky to just copy/paste. I can turn the dependency in the other direction if that turns out to be helpful.
1. If |externtype| is of the form [=externtype/tag=] |attribute| <var ignore>functype</var>, | ||
1. Assert: |attribute| is [=tagtype/attribute/exception=]. | ||
1. If |v| does not [=implement=] {{Tag}}, throw a {{LinkError}} exception. | ||
1. Let |tagaddr| be |v|.\[[Address]]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the field set in "create a Tag object" algorithm below. The supported fields depend on the interface that the value implements, which is checked in the step above.
1. Let « [=ref.extern=] |externaddr| » be |payload|. | ||
1. Throw the result of [=retrieving an extern value=] from |externaddr|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand your question. The goal of the "JavaScript exception tag" check is to ensure an exception thrown from js → wasm → js maintains its identity.
To <dfn>create a RuntimeException object</dfn> from a [=tag address=] |tagAddress| and [=list=] | ||
of WebAssembly values |payload|, perform the following steps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"create an Exception object" is when you already have the wasm values; "new Exception" is the js-side constructor called from webidl. I don't see a super obvious way to share more code between them, unfortunately.
For any [=associated store=] |store|, the result of | ||
[=tag_parameters=](|store|, [=JavaScript exception tag=]) must be « [=externref=] ». | ||
|
||
Issue: Should it be possible for `catch <JS-exception-tag>` to extract the payload from an exception with this tag? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly either way, but to support it, we need to expose the tag to core wasm somehow, right? I'm a bit fuzzy on how that would work.
To <dfn for=WebAssembly>throw</dfn> with a [=tag address=] |type| and matching [=list=] of WebAssembly values |payload|, perform the following steps: | ||
|
||
1. Unwind the stack until reaching the *catching try block* given |type|. | ||
1. Invoke the catch block with |payload|. | ||
|
||
Note: This algorithm is expected to be moved into the core specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is supposed to be basically equivalent to the throw instruction in core wasm. The core spec wasn't finished when I wrote this, so I put this here so I had an api to use in the rest of the spec.
@Ms2ger Thanks for the explanation! |
No description provided.