[webkit-reviews] review denied: [Bug 200952] [WASM-References] Do not overwrite argument registers in jsCallEntrypoint : [Attachment 376966] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 21 19:38:34 PDT 2019


Saam Barati <sbarati at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 200952: [WASM-References] Do not overwrite argument registers in
jsCallEntrypoint
https://bugs.webkit.org/show_bug.cgi?id=200952

Attachment 376966: Patch

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




--- Comment #10 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 376966
  --> https://bugs.webkit.org/attachment.cgi?id=376966
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.


More information about the webkit-reviews mailing list