Bug 165480 - WebAssembly JS API: coerce return values from imports
Summary: WebAssembly JS API: coerce return values from imports
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on: 165118
Blocks: 161709
  Show dependency treegraph
 
Reported: 2016-12-06 10:54 PST by JF Bastien
Modified: 2017-01-25 18:40 PST (History)
9 users (show)

See Also:


Attachments
patch (24.81 KB, patch)
2017-01-03 15:14 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.57 KB, patch)
2017-01-03 15:27 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.38 KB, patch)
2017-01-03 19:21 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.23 KB, patch)
2017-01-04 01:21 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.23 KB, patch)
2017-01-04 01:26 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.22 KB, patch)
2017-01-04 01:27 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch (25.48 KB, patch)
2017-01-04 01:30 PST, Saam Barati
ysuzuki: review+
Details | Formatted Diff | Diff
patch for landing (24.94 KB, patch)
2017-01-25 12:19 PST, Saam Barati
no flags Details | Formatted Diff | Diff
patch for landing (26.68 KB, patch)
2017-01-25 17:16 PST, Saam Barati
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-12-06 10:54:17 PST
I'll start off with aborting if the return values are of the wrong type. Let's add coercions after.
Comment 1 Radar WebKit Bug Importer 2016-12-20 14:29:24 PST
<rdar://problem/29760318>
Comment 2 Saam Barati 2017-01-03 15:14:58 PST
Created attachment 297959 [details]
patch
Comment 3 JF Bastien 2017-01-03 15:26:30 PST
Comment on attachment 297959 [details]
patch

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

(will look more after rebase)

> JSTests/wasm/function-tests/function-import-return-value.js:19
> +        .End();

You repeat this code below with just the type changing. I'd make it a function, takes in the return type.

> JSTests/wasm/function-tests/function-import-return-value.js:25
> +    let tests = [

Could you use a dictionary per entry, with { returned: 20, check: () => ... } ? It's easier to read IMO.

> JSTests/wasm/function-tests/function-import-return-value.js:29
> +        [{valueOf() { assert.truthy(!called); called = true; return 25; } }, (x) => { assert.truthy(called); assert.eq(x, 25) }],

Also test: MAX_SAFE_INTEGER, Infinity, -Infinity, NaN, undefined, null, new Boolean(true), new Boolean(false), 'hello', [].

> JSTests/wasm/function-tests/function-import-return-value.js:68
> +        [{valueOf() { assert.truthy(!called); called = true; return 25.82; } }, (x) => { assert.truthy(called); assert.eq(x, Math.fround(25.82)) }],

I'd fold all of these into the other tests: test all the same value, but have one function per expected result type.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:979
> +                AllowMacroScratchRegisterUsage allowScratch(jit);

Why does it need a scratch here?
Comment 4 Saam Barati 2017-01-03 15:27:51 PST
Created attachment 297963 [details]
patch

rebased
Comment 5 JF Bastien 2017-01-03 17:09:25 PST
Comment on attachment 297963 [details]
patch

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

> Source/JavaScriptCore/wasm/WasmBinding.cpp:69
> +    {

Is this the right place to do it, or after adjusting SP below? I thought unwinding would want to see this as a full-fledged function with its own stack, before throwing?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:103
> +                    auto* error = ErrorInstance::create(exec, *vm, globalObject->typeErrorConstructor()->errorStructure(), ASCIILiteral("i64 not allowed as return type or argument to an imported function"));

createTypeError(exec, ASCIILiteral(...), defaultSourceAppender, TypeFunction);

> Source/JavaScriptCore/wasm/WasmBinding.cpp:113
> +            return FINALIZE_CODE(linkBuffer, ("WebAssembly->JavaScript import[%i]", importIndex));

Maybe "WebAssembly->Javascript invalid import[%i]"? We could unique all of them later.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:338
> +        float (*convertToF32)(ExecState*, JSValue) = [] (ExecState* exec, JSValue v) -> float { 

Do we always assume that this call returns in returnValueFPR? e.g. on A32 soft-float it doesn't (hard-float does).

> Source/JavaScriptCore/wasm/WasmBinding.cpp:344
> +        jit.move(GPRInfo::returnValueGPR, GPRInfo::argumentGPR1);

On A32 and A64 this is wrong because argGPR0 == retGPR0, so the first line clobbers the second.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:370
> +        double (*convertToF64)(ExecState*, JSValue) = [] (ExecState* exec, JSValue v) -> double { 

Ditto on calling convention.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:375
> +        jit.move(GPRInfo::returnValueGPR, GPRInfo::argumentGPR1);

Ditto on clobber.
Comment 6 Saam Barati 2017-01-03 18:49:43 PST
Comment on attachment 297963 [details]
patch

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

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:69
>> +    {
> 
> Is this the right place to do it, or after adjusting SP below? I thought unwinding would want to see this as a full-fledged function with its own stack, before throwing?

It will still be seeing a valid frame with a valid SP/FP, but it will just be a smaller frame (specifically, it'll be 16 bytes large containing return PC/previous FP).

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:103
>> +                    auto* error = ErrorInstance::create(exec, *vm, globalObject->typeErrorConstructor()->errorStructure(), ASCIILiteral("i64 not allowed as return type or argument to an imported function"));
> 
> createTypeError(exec, ASCIILiteral(...), defaultSourceAppender, TypeFunction);

This won't work. createTypeError uses lexicalGlobalobject which won't work since we don't have a JSObject as our callee inside this thunk. That's why I specifically grab JSGlobalObject from the instance.

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:113
>> +            return FINALIZE_CODE(linkBuffer, ("WebAssembly->JavaScript import[%i]", importIndex));
> 
> Maybe "WebAssembly->Javascript invalid import[%i]"? We could unique all of them later.

SGTM

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:338
>> +        float (*convertToF32)(ExecState*, JSValue) = [] (ExecState* exec, JSValue v) -> float { 
> 
> Do we always assume that this call returns in returnValueFPR? e.g. on A32 soft-float it doesn't (hard-float does).

I believe we do assume this (for example, B3 assumes this). What about A64 since that's what we're supporting? Does that support soft-float?

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:344
>> +        jit.move(GPRInfo::returnValueGPR, GPRInfo::argumentGPR1);
> 
> On A32 and A64 this is wrong because argGPR0 == retGPR0, so the first line clobbers the second.

Nice catch.
Comment 7 Saam Barati 2017-01-03 19:21:18 PST
Created attachment 297986 [details]
patch

Addressed JF's comments
Comment 8 Saam Barati 2017-01-03 19:21:48 PST
(In reply to comment #7)
> Created attachment 297986 [details]
> patch
> 
> Addressed JF's comments

That's actually not true. I still need to add JF's suggested tests.
Comment 9 Saam Barati 2017-01-03 19:30:52 PST
Comment on attachment 297959 [details]
patch

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

>> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:979
>> +                AllowMacroScratchRegisterUsage allowScratch(jit);
> 
> Why does it need a scratch here?

When we properly implement cross-instance call-indirect, the stub might use a scratch.
Comment 10 JF Bastien 2017-01-03 20:04:40 PST
> >> Source/JavaScriptCore/wasm/WasmBinding.cpp:338
> >> +        float (*convertToF32)(ExecState*, JSValue) = [] (ExecState* exec, JSValue v) -> float { 
> > 
> > Do we always assume that this call returns in returnValueFPR? e.g. on A32 soft-float it doesn't (hard-float does).

Not that I know: AFAIK soft-float exists on A32 to bridge the ABI between platforms with and without VFP. It's an ABI that works between both by basically ignoring all S/D/Q regs.

So if B3 assumes this then we're OK with the same assumption. x64-64 and A64 are OK here, I was just worried other platforms may have a slight gotcha, but since it's pervasive then it's not worth playing special.
Comment 11 JF Bastien 2017-01-03 20:05:13 PST
(In reply to comment #7)
> Created attachment 297986 [details]
> patch
> 
> Addressed JF's comments

Oh can you also test -0.0? I just updated assert.eq to detect it properly (versus 0.0).
Comment 12 JF Bastien 2017-01-03 20:15:54 PST
Comment on attachment 297986 [details]
patch

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

Took a second look, I think this is good to go after suggested tests.

> Source/JavaScriptCore/wasm/WasmBinding.cpp:292
> +        auto isDouble = jit.branchIfNotInt32(JSValueRegs(GPRInfo::returnValueGPR), DoNotHaveTagRegisters);

Actually: for the return side (after the call) don't we have the tag registers since we're coming from JS?
Comment 13 Saam Barati 2017-01-03 22:13:27 PST
(In reply to comment #12)
> Comment on attachment 297986 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=297986&action=review
> 
> Took a second look, I think this is good to go after suggested tests.
> 
> > Source/JavaScriptCore/wasm/WasmBinding.cpp:292
> > +        auto isDouble = jit.branchIfNotInt32(JSValueRegs(GPRInfo::returnValueGPR), DoNotHaveTagRegisters);
> 
> Actually: for the return side (after the call) don't we have the tag
> registers since we're coming from JS?

Nope. The tags are callee saves, so a JS callee would have to save our potentially non-tag callee saves before populating the tags.

It might be worth populating the tags here and restoring them before we return. I'll leave that for another time.
Comment 14 JF Bastien 2017-01-03 22:14:54 PST
(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 297986 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=297986&action=review
> > 
> > Took a second look, I think this is good to go after suggested tests.
> > 
> > > Source/JavaScriptCore/wasm/WasmBinding.cpp:292
> > > +        auto isDouble = jit.branchIfNotInt32(JSValueRegs(GPRInfo::returnValueGPR), DoNotHaveTagRegisters);
> > 
> > Actually: for the return side (after the call) don't we have the tag
> > registers since we're coming from JS?
> 
> Nope. The tags are callee saves, so a JS callee would have to save our
> potentially non-tag callee saves before populating the tags.
> 
> It might be worth populating the tags here and restoring them before we
> return. I'll leave that for another time.

Oh OK, probably not worth doing here then.
Comment 15 Saam Barati 2017-01-04 01:21:29 PST
Created attachment 297997 [details]
patch

I found a couple of interesting bug as I wrote more tests.
For
- Boxed Int32 JSValue => float/double, we were zero extending instead of sign extending before converting to float/double.
- the truncateDoubleToInt32 MacroAssembler function isn't what we want for all doubles. For example, it breaks on various numbers not representable as int32. We could probably detect the breakage and branch around it, but for now, converting double to int32 goes to the slow path.

The patch fixes these bugs, and has tests for them. Because of these bugs, the conversion code has changed a bit since the last patch.
Comment 16 Saam Barati 2017-01-04 01:26:13 PST
Created attachment 297999 [details]
patch
Comment 17 Saam Barati 2017-01-04 01:27:30 PST
Created attachment 298000 [details]
patch
Comment 18 Saam Barati 2017-01-04 01:30:49 PST
Created attachment 298002 [details]
patch
Comment 19 JF Bastien 2017-01-04 10:17:44 PST
Comment on attachment 298002 [details]
patch

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

A few nits, lgtm otherwise.

> JSTests/wasm/function-tests/function-import-return-value.js:28
> +            [-0.0, (x) => assert.eq(1/x, -Infinity)],

assert.eq already handles it: it differentiates -0.0 from 0.0. You can therefore just do assert.eq(x, -0.0);

> JSTests/wasm/function-tests/function-import-return-value.js:42
> +            [-0.0, (x) => assert.eq(1/x, -Infinity)],

Ditto.

> JSTests/wasm/function-tests/function-import-return-value.js:126
> +        }

Can't this be assert.throws(() => instance.exports.foo(), Error, "");

> JSTests/wasm/function-tests/function-import-return-value.js:156
> +}

I think you need a test with a parameter that has valueOf, and check that valueOf wasn't called (the exception occurs before the call).
Same below: if the first param isn't i64 but the second is, then its valueOf shouldn't be called.

We could just leave a FIXME + bug and do it later.
Comment 20 Yusuke Suzuki 2017-01-09 06:58:18 PST
Comment on attachment 298002 [details]
patch

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

r=me

> Source/JavaScriptCore/wasm/WasmFunctionParser.h:330
> +        return { };

Nice.
Comment 21 Yusuke Suzuki 2017-01-23 04:42:48 PST
Is this patch missed?
Comment 22 Saam Barati 2017-01-23 09:24:40 PST
(In reply to comment #21)
> Is this patch missed?

Yeah! I'll land today, thanks for the review.
Comment 23 Saam Barati 2017-01-25 12:19:16 PST
Created attachment 299725 [details]
patch for landing
Comment 24 Saam Barati 2017-01-25 12:21:46 PST
(In reply to comment #19)
> Comment on attachment 298002 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298002&action=review
> 
> A few nits, lgtm otherwise.
> 
> > JSTests/wasm/function-tests/function-import-return-value.js:28
> > +            [-0.0, (x) => assert.eq(1/x, -Infinity)],
> 
> assert.eq already handles it: it differentiates -0.0 from 0.0. You can
> therefore just do assert.eq(x, -0.0);
> 
> > JSTests/wasm/function-tests/function-import-return-value.js:42
> > +            [-0.0, (x) => assert.eq(1/x, -Infinity)],
> 
> Ditto.
> 
> > JSTests/wasm/function-tests/function-import-return-value.js:126
> > +        }
> 
> Can't this be assert.throws(() => instance.exports.foo(), Error, "");
> 
> > JSTests/wasm/function-tests/function-import-return-value.js:156
> > +}
> 
> I think you need a test with a parameter that has valueOf, and check that
> valueOf wasn't called (the exception occurs before the call).
> Same below: if the first param isn't i64 but the second is, then its valueOf
> shouldn't be called.
Oops, I haven't done this yet. Will add a test for this.

> 
> We could just leave a FIXME + bug and do it later.
Comment 25 Saam Barati 2017-01-25 17:16:06 PST
Created attachment 299775 [details]
patch for landing
Comment 26 WebKit Commit Bot 2017-01-25 18:39:59 PST
Comment on attachment 299775 [details]
patch for landing

Clearing flags on attachment: 299775

Committed r211195: <http://trac.webkit.org/changeset/211195>
Comment 27 WebKit Commit Bot 2017-01-25 18:40:04 PST
All reviewed patches have been landed.  Closing bug.