Bug 164876 - WebAssembly JS API: check and test in-call / out-call values
Summary: WebAssembly JS API: check and test in-call / out-call values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords: InRadar
Depends on: 164757
Blocks: 161709
  Show dependency treegraph
 
Reported: 2016-11-17 10:53 PST by JF Bastien
Modified: 2017-01-03 12:25 PST (History)
7 users (show)

See Also:


Attachments
patch (19.28 KB, patch)
2017-01-02 22:29 PST, JF Bastien
no flags Details | Formatted Diff | Diff
patch (23.26 KB, patch)
2017-01-02 23:50 PST, JF Bastien
saam: review+
saam: commit-queue-
Details | Formatted Diff | Diff
patch (25.37 KB, patch)
2017-01-03 11:49 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-11-17 10:53:25 PST
Bug #164757 adds support for WebAssemblyFunction to call into and out of WebAssembly from JavaScript.

The design [1] isn't super clear on what the conversion rules are calling with mismatched signatures (coercion, etc), number of arguments, and return type. I implemented what seems to make sense, but we should clarify, update the implementation, and test it thoroughly.

  [1]: https://github.com/WebAssembly/design/blob/master/JS.md
Comment 1 JF Bastien 2017-01-02 18:00:50 PST
From this: https://github.com/WebAssembly/design/blob/master/JS.md#exported-function-exotic-objects

We should follow this behavior:

WebAssembly Exported Functions have a [[Call]](this, argValues) method defined as:

1. Let args be an empty list of coerced values.
2. Let inArity be the number of arguments and outArity be the number of results in the function type of the function's [[Closure]].
3. For all values v in argValues, in the order of their appearance:
  i. If the length ofargs is less than inArity, append ToWebAssemblyValue(v) to args.
4. While the length of args is less than inArity, append ToWebAssemblyValue(undefined) to args.
5. Let ret be the result of calling Eval.invoke passing [[Closure]], and args.
6. If outArity is 0, return undefined.
7. Otherwise, return ToJSValue(v), where v is the singular element of ret.

I'll fix this.
Comment 2 Radar WebKit Bug Importer 2017-01-02 22:08:00 PST
<rdar://problem/29844107>
Comment 3 JF Bastien 2017-01-02 22:20:25 PST
This is a correctness issue which seems to cause test failures from the following waterfall tests:

Similar unexpected failures, average 100.0% similarity with stddev 0.0: 20000412-2.c.s.wast.wasm 20000412-4.c.s.wast.wasm 20001124-1.c.s.wast.wasm 20010915-1.c.s.wast.wasm 20020206-2.c.s.wast.wasm 20020402-2.c.s.wast.wasm 20021204-1.c.s.wast.wasm 20031012-1.c.s.wast.wasm 20041113-1.c.s.wast.wasm 20041114-1.c.s.wast.wasm 20050125-1.c.s.wast.wasm 20080122-1.c.s.wast.wasm 920501-6.c.s.wast.wasm 980506-3.c.s.wast.wasm 990106-2.c.s.wast.wasm anon-1.c.s.wast.wasm bitfld-1.c.s.wast.wasm const-addr-expr-1.c.s.wast.wasm ipa-sra-1.c.s.wast.wasm loop-9.c.s.wast.wasm pending-4.c.s.wast.wasm pr23047.c.s.wast.wasm pr28651.c.s.wast.wasm pr32500.c.s.wast.wasm pr36321.c.s.wast.wasm pr40493.c.s.wast.wasm pr61375.c.s.wast.wasm switch-1.c.s.wast.wasm vrp-1.c.s.wast.wasm vrp-2.c.s.wast.wasm vrp-3.c.s.wast.wasm vrp-5.c.s.wast.wasm vrp-6.c.s.wast.wasm
Sample failure: FAILED 20000412-2.c.s.wast.wasm

Unknown exception of type `object`: TypeError: Not enough arguments (evaluating 'modules[0].exports.main()')
Exception: TypeError: Not enough arguments (evaluating 'modules[0].exports.main()')
main@[native code]
global code@/Volumes/dev/waterfall/src/work/wasm-install/lib/wasm.js:974:36


These failure could be hiding others though.
Comment 4 JF Bastien 2017-01-02 22:29:40 PST
Created attachment 297923 [details]
patch
Comment 5 JF Bastien 2017-01-02 23:50:32 PST
Created attachment 297924 [details]
patch

Add a parameter / argument fuzzer.
Comment 6 Saam Barati 2017-01-02 23:55:04 PST
Comment on attachment 297923 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297923&action=review

r=me

> Source/JavaScriptCore/wasm/WasmBinding.cpp:83
>          case Void:
>          case Func:
>          case Anyfunc:

It seems like these 3 should be validation errors, right? So maybe RELEASE_ASSERT_NOT_REACHED for them?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:84
> +        case I64: {

I forget what the spec decided here. I think we're supposed to throw an exception at the very beginning of the call, but maybe it should be a link error?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:147
> +        auto materializeDoubleEncodeOffset = [&hasMaterializedDoubleEncodeOffset] (JIT& jit, GPRReg dest) {

Nit: This doesn't really need to take any arguments since it can just capture the arguments its always called with.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:149
> +                ASSERT(DoubleEncodeOffset == 1ll << 48);

If you decide to keep the move/lshift, then make this a static assert.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:151
> +                jit.move(JIT::TrustedImm32(1), dest);
> +                jit.lshift64(JIT::TrustedImm32(48), dest);

Is this actually faster than just materializing the constant?

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:79
> +        // Arity could mismatch, this will take care of the defined behavior: https://github.com/WebAssembly/design/blob/master/JS.md#exported-function-exotic-objects

Nit: I don't think this comment is needed. The semantics of toNumber/toInt(argument(idx)) are well known.
Comment 7 Saam Barati 2017-01-02 23:56:19 PST
Comment on attachment 297924 [details]
patch

r=me (with same comments as before)
Comment 8 Saam Barati 2017-01-03 00:11:10 PST
It looks like i64 behavior should be to throw an exception immediately:
https://github.com/WebAssembly/design/issues/913
Comment 9 JF Bastien 2017-01-03 11:49:12 PST
Created attachment 297938 [details]
patch

Address comments.

> > Source/JavaScriptCore/wasm/WasmBinding.cpp:83
> >          case Void:
> >          case Func:
> >          case Anyfunc:
> 
> It seems like these 3 should be validation errors, right? So maybe
> RELEASE_ASSERT_NOT_REACHED for them?

It's still open according to the linked bug, so I left it as-is.


> > Source/JavaScriptCore/wasm/WasmBinding.cpp:84
> > +        case I64: {
> 
> I forget what the spec decided here. I think we're supposed to throw an
> exception at the very beginning of the call, but maybe it should be a link
> error?

Same: IIUC it's still open so I left it as-is. We have open bugs, so we'll definitely get back to it and add appropriate tests when we fix.


> > Source/JavaScriptCore/wasm/WasmBinding.cpp:147
> > +        auto materializeDoubleEncodeOffset = [&hasMaterializedDoubleEncodeOffset] (JIT& jit, GPRReg dest) {
> 
> Nit: This doesn't really need to take any arguments since it can just
> capture the arguments its always called with.

OK I removed JIT. I like keeping the register though, because it makes the usage of the function below more explicit about where the value ends up.


> > Source/JavaScriptCore/wasm/WasmBinding.cpp:149
> > +                ASSERT(DoubleEncodeOffset == 1ll << 48);
> 
> If you decide to keep the move/lshift, then make this a static assert.
> 
> > Source/JavaScriptCore/wasm/WasmBinding.cpp:151
> > +                jit.move(JIT::TrustedImm32(1), dest);
> > +                jit.lshift64(JIT::TrustedImm32(48), dest);
> 
> Is this actually faster than just materializing the constant?

Materializing it uses registers which we don't control as part of the wasm CC, IIUC. Is that not the case? On ARM we could use "CMP (immediate)" instead, that would be better, but from the assembler's code I didn't see a way to do this while behaving on x86. This is pretty cheap and encodes pretty tightly. Perf-wise I'm more worried about saturating the number of memops we can issue than doing this arithmetic.
Comment 10 WebKit Commit Bot 2017-01-03 12:25:32 PST
Comment on attachment 297938 [details]
patch

Clearing flags on attachment: 297938

Committed r210244: <http://trac.webkit.org/changeset/210244>
Comment 11 WebKit Commit Bot 2017-01-03 12:25:36 PST
All reviewed patches have been landed.  Closing bug.