[webkit-reviews] review granted: [Bug 207727] [WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention. : [Attachment 390829] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Tadeu Zagallo <tzagallo at apple.com>'s
request for review:
Bug 207727: [WASM] Wasm interpreter's calling convention doesn't match Wasm
JIT's convention.
https://bugs.webkit.org/show_bug.cgi?id=207727

Attachment 390829: Patch

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




--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 390829
  --> https://bugs.webkit.org/attachment.cgi?id=390829
Patch

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

r=me with fixes.

> Source/JavaScriptCore/ChangeLog:13
> +	   - When entering Wasm from JS, memory registers must be preserved.

Please add a comment after this to say: "Previously, we don't do this for
MemoryMode::Signalling, but with the introduction of the Wasm LLInt, this is
now always necessary.  This patch fixes this."

I think it's important to call out what this patch actually changes.

> Source/JavaScriptCore/ChangeLog:15
> +	   - Both tiers must preserve every *other* register they use. e.g. the
LLInt must preserve PB
> +	     and wasmInstance, but must *not* preserve memoryBase and
memorySize.

Please add a comment after this to say: "Previously, the LLInt was also
preserving and restoring memoryBase and memorySize.  This patch fixes this."

> Source/JavaScriptCore/ChangeLog:23
> +	       - A Signaling JIT caller must be aware that the LLInt may trash
the size register, since
> +		 it always bounds checks.

Please add a comment after this to say: "Previously, the JITs did not
explicitly call out that the size registers as potentially being clobbered. 
This patch fixes this."

> Source/JavaScriptCore/llint/WebAssembly.asm:186
>      if ARM64 or ARM64E
> -	   emit "stp x23, x26, [x29, #-16]"
> -	   emit "stp x19, x22, [x29, #-32]"
> +	   emit "stp x19, x26, [x29, #-16]"
>      elsif X86_64
> -	   storep memorySize, -0x08[cfr]
> -	   storep memoryBase, -0x10[cfr]
> -	   storep PB, -0x18[cfr]
> -	   storep wasmInstance, -0x20[cfr]
> +	   storep PB, -0x8[cfr]
> +	   storep wasmInstance, -0x10[cfr]

I think this macro deserves a comment to indicate that: "We intentionally do
not preserve the memoryBase and memorySize here. See the comment in
restoreCalleeSavesUsedByWasm() below for why."

> Source/JavaScriptCore/llint/WebAssembly.asm:192
>  macro restoreCalleeSavesUsedByWasm()

I think this macro deserves a comment to indicate that: "We intentionally do
not restore the memoryBase and memorySize here because these registers are
treated as globals within the same Wasm module, and we want any changes to
their values to be passed thru to the caller."

This may save us from someone naively re-adding the preserving and restoration
of memoryBase and memorySize simply because they are callee saves.

> JSTests/wasm/regress/llint-callee-saves.js:1
> +//@ requireOptions("--useWebAssemblyFastMemory=false")

I think for now, you should make 2 copies of this test:
llint-callee-saves-with-fast-memory.js and
llint-callee-saves-without-fast-memory.js.  Otherwise, we won't catch the other
case of caller register corruption.