-
Notifications
You must be signed in to change notification settings - Fork 7
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
Initial addition of builtin steps #39
Conversation
Needs more detail to properly integrate with GC array ops.
This avoids having to refer to actual Wasm instructions, since after all this is a host function.
@eqrion this is now ready for review |
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 looks good! There are probably some small improvements to the formalization that we could make, but this is much better than the JS we had before.
Two high level questions, that can get addressed later:
- Are we always using the 'original' value of the JS builtin functions (like String.charCodeAt)? Or do we need to add extra language to this effect.
- How do we handle the cases where we 'trap'? I looked through the wasm3.0 branch (which has EH), and it's not clear to me how wasm traps are uncatchable by wasm. But I think it's useful for performance to keep that the case here for these new builtins.
Note: This function only takes a mutable i16 array defined in its own recursion group. | ||
If this is an issue for toolchains, we can look into how to relax the function type | ||
while still maintaining good performance. |
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.
Lets drop the second line of "If this is an issue...good performance". It's not clear how we could do that at this point, and no one has seemed to complain.
The |funcType| of this builtin is `(func (param externref (ref null (array (mut i16))) i32) (result i32))`. | ||
|
||
Note: This function only takes a mutable i16 array defined in its own recursion group. | ||
If this is an issue for toolchains, we can look into how to relax the function type |
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.
same as above.
1. Let |string| be [=?=] [$UnwrapString$](|string|). | ||
1. Let |stringLength| be the [=string/length=] of |string|. | ||
1. Let |arrayLength| be the number of elements in |array|. | ||
1. If |start| + |length| > |arrayLength| |
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 think this needs to be |start| + |stringLength|
here.
I'm just going to merge this and then do the fixes myself, plus some extra stuff. |
Thanks for taking a look and merging.
I talked to @syg about this, and I think what we have will be clear enough for now. But there some improvements we could make include:
Agreed that the trap behavior continues to be the oddest part of this. I think we probably want to avoid the "throw" language altogether and specify that these trap "as if" they were implemented in Wasm. But I don't think there's any precedent for this today, so we may have to make something up. Curious if @rossberg, @conrad-watt, or @tlively have thoughts here. |
How bad would it be to have the builtin operations throw normal JS errors instead of trapping? I hope the performance impact would be negligible if the erroring path could be placed with other cold code. Throwing a normal JS error would also make polyfills simpler, and would even allow implementing the error path by calling out to JS to redo the operation using the canonical implementation. Looking at the JS Wasm spec updated for EH, it does contain this line:
If we can just arbitrarily execute WebAssembly instructions from the JS spec (which is more than a little questionable IMO; instructions only have semantics within a context), then we can get the trap by writing "Execute the WebAssembly instruction (unreachable)." |
Technically, by not being exceptions at all. In the Wasm semantics, they are a completely separate form of result that exception handlers don't recognise. They are only converted to (JavaScript) exceptions at the JS API boundary, by means of hand-wavy words. And as far as I can tell, they are never converted the other direction. That is, in a sandwich scenario, if a Wasm trap reaches JS, it converts to a JS exception, and if that reaches Wasm again, then it just materialises as a random wrapped JS exception with the JS exception tag. At least that's what the JS API currently seems to imply. To be honest, I'm not sure if that was intended, implementations actually agree, or whether we have any tests for that behaviour. Before EH, that was not observable, but now it is.
Agreed on both accounts. :) Maybe dealing with traps generally needs to be made more precise in the JS API at this point. |
+1 on all of these: And yes executing random wasm instructions from the JS API, (or this proposed "just unwind instead of throwing" idea which is equally hand-wavy) would be good to improve. Probably we'd want to augment the embedder API, but what we're talking about isn't exactly an API call, so it seems nontrivial. |
Adds steps for all string builtins.
Several different approaches are taken for referencing JS
operations, depending on what the JS spec exposes:
Call
operation and a reference to the functionAlso clean up some of the underlying infra:
UnwrapString
an abstract opMajor TODOs include:
i16
arrays