[webkit-reviews] review denied: [Bug 169773] WebAssembly: add fallback to use pinned register to load/store state : [Attachment 305491] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 169773: WebAssembly: add fallback to use pinned register to load/store
state
https://bugs.webkit.org/show_bug.cgi?id=169773

Attachment 305491: Patch

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




--- Comment #26 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 305491
  --> https://bugs.webkit.org/attachment.cgi?id=305491
Patch

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

r- because we're moving to a PIC architecture. We should ensure that this patch
doesn't add more things that need to be changed as we move Wasm runtime above
VM. See comments below.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:1235
> +    // FIXME: JSWebAssemblyCallee should have a pointer to
JSWebAssemblyInstance instead.
> +    GPRReg wasmContext = pinnedRegs.wasmContextPointer;
> +    if (!useFastTLS()) {
> +	   jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister,
CallFrameSlot::callee * static_cast<int>(sizeof(Register))), wasmContext);
> +	   jit.andPtr(CCallHelpers::TrustedImmPtr(MarkedBlock::blockMask),
wasmContext);
> +	   jit.loadPtr(CCallHelpers::Address(wasmContext,
MarkedBlock::offsetOfVM()), wasmContext);
> +	   jit.loadPtr(CCallHelpers::Address(wasmContext,
VM::wasmContextOffset()), wasmContext);
> +    }

This is going to make it harder to do PIC. We need a way to get the context
outside the VM. Maybe the first argument to every JS wrapper can be a VM*? Or
maybe there is another solution