Bug 200952 – [WASM-References] Do not overwrite argument registers in jsCallEntrypoint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Justin Michaud]

Do not overwrite argument registers / callee saves in jsCallEntrypoint

[Justin Michaud]

Created attachment 376828 [details]
Patch

[Saam Barati]

Comment on attachment 376828 [details]
Patch

How does exception handling restore these? Why is this needed? I thought it was already handled?

[Saam Barati]

Comment on attachment 376828 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:320
> +                // Save callee saves before call. We can remove this when inlined.

we already do this! Read above "usedCalleeSaveRegisters"

[Saam Barati]

Comment on attachment 376828 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:330
> +                    jit.storePtr(reg, calleeFrame.withOffset(-(i+1) * sizeof(int64_t*)));

if you actually needed to do this, which I don't think you do, since we do it above, you'd need to adjust the frame size calculation

[Saam Barati]

I misunderstood what's going on.

It's pretty crazy we're calling a C function in the middle of loading up the argument registers.

An easy way to fix this would be to emit checks on the funcrefs before any argument is loaded into the corresponding argument register

And even better solution would just to emit inline asm to type check the thing

[Saam Barati]

Comment on attachment 376828 [details]
Patch

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

> JSTests/wasm/references/func_ref.js:461
> +        .Function("test", { params: ["i32", "funcref"], ret: "i32" })

please add a test where you have a bunch of i32s followed by a funcref, that should fail even with your current patch

[Justin Michaud]

Created attachment 376950 [details]
Patch

[Saam Barati]

Comment on attachment 376950 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        We just inline the call to isWebassemblyHostFunction.

can you explain what the issue before was in some detail here?

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:330
> +                static_assert(std::is_final<WebAssemblyFunction>::value, "This loop is untested");
> +                static_assert(std::is_final<WebAssemblyWrapperFunction>::value, "This loop is untested");
> +
> +                jit.move(CCallHelpers::TrustedImmPtr(WebAssemblyFunction::info()), scratch2GPR);
> +                auto isWasmFunction = jit.branch64(CCallHelpers::Equal, scratchGPR, scratch2GPR);
> +                jit.move(CCallHelpers::TrustedImmPtr(WebAssemblyWrapperFunction::info()), scratch2GPR);
> +                auto isWasmFunctionWrapper = jit.branch64(CCallHelpers::Equal, scratchGPR, scratch2GPR);

Please let's not add a loop that's untested. We should just have a single branch, which should be ensured by the is_final check above.

[Justin Michaud]

Created attachment 376966 [details]
Patch

[Saam Barati]

Comment on attachment 376966 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:317
> +                slowPath.append(jit.branchIfNotObject(scratchGPR));

don't think this is necessary. All cell's have structures. All structure's have class infos. So this is an unnecessary branch.

Can you add tests where you try to pass non-object cells. And other JSObject types (like an array or something).

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:326
> +                jit.move(CCallHelpers::TrustedImmPtr(WebAssemblyFunction::info()), scratch2GPR);
> +                auto isWasmFunction = jit.branch64(CCallHelpers::Equal, scratchGPR, scratch2GPR);

let's use inline branch64 with immediate.

Also, it's wrong to use scratch2GPR in this loop. I did a terrible job naming scratch2GPR. Maybe it's worth naming it cachedStackLimit or something like that so we don't make this mistake in the future.

This wrong value in stack limit should be testable.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:328
> +                jit.move(CCallHelpers::TrustedImmPtr(WebAssemblyWrapperFunction::info()), scratch2GPR);
> +                slowPath.append(jit.branch64(CCallHelpers::NotEqual, scratchGPR, scratch2GPR));

ditto

> JSTests/wasm/references/func_ref.js:469
> +    const myfun = makeExportedFunction(1337);
> +    assert.eq(myfun(), 1337)
> +    assert.eq(42, $1.exports.test(42, myfun))
> +    assert.throws(() => $1.exports.test(42, () => 5), Error, "Funcref must be an exported wasm function (evaluating 'func(...args)')")

let's run this in a loop.

[Justin Michaud]

Created attachment 377031 [details]
Patch

[Saam Barati]

Comment on attachment 377031 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:321
> +                jit.loadPtr(vm.addressOfSoftStackLimit(), stackLimitGPR);

please remove.

[Justin Michaud]

Created attachment 377041 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 377041 [details]
Patch

Clearing flags on attachment: 377041

Committed r249069: <https://trac.webkit.org/changeset/249069>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54654491>