Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update JS API for exnref #301
Update JS API for exnref #301
Changes from all commits
587dafb
15a47cf
904a8fb
ec302e1
fdf1b7c
4799452
f57cdb8
ccbee0f
4d08a58
74a92cb
b17459a
fa07dea
ea5217b
1c958c2
46de060
5b8380a
4d03423
d073cfe
b6a79f9
51338d7
dfdd0b9
624bcca
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There is an optimization that was discussed before where the engine does not wrap/unwrap the exception at the boundary, and instead implements the JSTag behavior as a special-case in the catch handler. But with that optimization, if the user code creates a
WebAssembly.Exception(WebAssembly.JSTag, ...)
object explicitly and throws it, it would not get unwrapped automatically at the export, which would violate this spec, right?We can still benefit from the optimization and add a check when the object crosses the JS/Wasm boundary to cover this special case, but I just wanted to make sure that this is indeed what this spec requires us to do.
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 was thinking about that issue as well while porting the
JSTag
behavior to theexnref
implementation in V8. I was wondering if we could disallow creating aWebAssembly.Exception(JSTag, ...)
in the first place? As in, throw aRangeError
or some other exception from the constructor ofWA.Exception
. That would probably simplify a number of things, both at the spec and implementation level.Would that behavior be acceptable?
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 would describe the spec as currently written as saying that regular JS exceptions just never get wrapped or unwrapped, the payload just passes through (although like all exceptions the payload is paired with a tag, even if that tag is not exposed to the user). I think as written today, if you created a
WebAssembly.Exception(WebAssembly.JSTag, payload)
and threw it into wasm, it would be treated as a wasm exception on entry and unrapped; and then when it came back out to JS it would not be rewrapped but come out as justpayload
. If I'm understanding the optimization you describe, it would naturally have exactly this behavior, so there wouldn't need to be special cases.Having said that, that behavior is a little weird and may be unexpected since it's asymmetric. I don't really see a use case for an explicitly-wrapped
WebAssembly.Exception
with the JS tag, so maybe it does make sense to disallow creation of such an object.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 also realized that this discussion should probably go in #269 since that PR is where the JS Tag object actually gets exposed to JS explicitly and makes that behavior possible.
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.
Does a
WebAssembly.Exception
object haveAddress
?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.
Yes,
Address
is initialized in "initialize an Exception object". Exception is now just like all the other JS-exposed objects which also have their wasm Addresses stored in an internal slot in the JS object. (Although I just noticed that the naming convention isn't consistent right now; TheException
andTag
objects have the address in a field namedAddress
whereas the rest have that field named after the object type, e.g.Global
. Maybe I'll fix that in a followup change).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 looks
v
containsType
andValue
. Shouldn't we only convert theValue
part, rather than the wholev
?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.
The
Type
andValue
fields come from ECMAScript completion record, andresult
is the completion record.On lines 1086 - 1087 above,
result.Type
isTHROW
and thenv
isresult.Value
, i.e. the value returned by the JS host function called on line 1081, so it's already a value. So I think that's the thing that we need to convert to a wasm externref and throw back into wasm.... actually though, I just noticed that the handling of completion records between 'run a host function' and 'create a host function' wasn't quite right even before the EH spec. I just filed WebAssembly/spec#1743 about that.
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.
Does this mean tag == tagAddress? Somewhere above in this doc contains
So they seem to be different.
And this PR doesn't include how Wasm should handle the JS exception tag, 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.
Line 1208 is where you're referring to. There
tag
andtagAddress
are both basically function parameters, andtag
is a JS object andtagAddress
is an address as defined in the wasm runtime spec. HeretagAddress
is basically a local variable (a wasm address), a return value oftag_alloc
; andtag
isn't a variable at all. This function just returns the address.And yes, #269 is the PR that allows actually exposing the JS exception tag (which by extension allows it to be imported into a wasm module).