Bug 165480

<rdar://problem/29760318>

Created attachment 297959 [details] patch

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?

Created attachment 297963 [details] patch rebased

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 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.

Created attachment 297986 [details] patch Addressed JF's comments

(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 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.

> >> 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.

(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 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?

(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.

(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.

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.

Created attachment 297999 [details] patch

Created attachment 298000 [details] patch

Created attachment 298002 [details] patch

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 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.

Is this patch missed?

(In reply to comment #21) > Is this patch missed? Yeah! I'll land today, thanks for the review.

Created attachment 299725 [details] patch for landing

(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.

Created attachment 299775 [details] patch for landing

Comment on attachment 299775 [details] patch for landing Clearing flags on attachment: 299775 Committed r211195: <http://trac.webkit.org/changeset/211195>

All reviewed patches have been landed. Closing bug.