Bug 164876 – WebAssembly JS API: check and test in-call / out-call values

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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

[JF Bastien]

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.

[Radar WebKit Bug Importer]

<rdar://problem/29844107>

[JF Bastien]

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.

[JF Bastien]

Created attachment 297923 [details]
patch

[JF Bastien]

Created attachment 297924 [details]
patch

Add a parameter / argument fuzzer.

[Saam Barati]

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.

[Saam Barati]

Comment on attachment 297924 [details]
patch

r=me (with same comments as before)

[Saam Barati]

It looks like i64 behavior should be to throw an exception immediately:
https://github.com/WebAssembly/design/issues/913

[JF Bastien]

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.

[WebKit Commit Bot]

Comment on attachment 297938 [details]
patch

Clearing flags on attachment: 297938

Committed r210244: <http://trac.webkit.org/changeset/210244>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.