Do not overwrite argument registers / callee saves in jsCallEntrypoint
Created attachment 376828 [details] Patch
Comment on attachment 376828 [details] Patch How does exception handling restore these? Why is this needed? I thought it was already handled?
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"
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
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
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
Created attachment 376950 [details] Patch
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.
Created attachment 376966 [details] Patch
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.
Created attachment 377031 [details] Patch
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.
Created attachment 377041 [details] Patch
Comment on attachment 377041 [details] Patch Clearing flags on attachment: 377041 Committed r249069: <https://trac.webkit.org/changeset/249069>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54654491>