[WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

It appears that a Wasm JIT function doesn't get an updated memory base and size. when we grow memory.

[Michael Saboff]

<rdar://problem/59234854>

[Michael Saboff]

Created attachment 390699 [details]
Patch

[Mark Lam]

Comment on attachment 390699 [details]
Patch

The preservation and restoration of callee saved registers with this patch (when calling from JS to wasm or wasm to wasm) is broken.  Tadeu's working on a fix.

[Tadeu Zagallo]

Created attachment 390829 [details]
Patch

[Mark Lam]

Comment on attachment 390829 [details]
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.

[Mark Lam]

(In reply to Mark Lam from comment #5)
> > 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."

typo: /size registers/size register/.

[Tadeu Zagallo]

Created attachment 390835 [details]
Patch for landing

[Tadeu Zagallo]

Created attachment 390836 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390836 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390836 [details]
Patch for landing

Rejecting attachment 390836 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 390836, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=390836&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=207727&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 390836 from bug 207727.
Fetching: https://bugs.webkit.org/attachment.cgi?id=390836
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js
	A	JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js
	M	JSTests/ChangeLog
	M	Source/JavaScriptCore/ChangeLog
	M	Source/JavaScriptCore/llint/WebAssembly.asm
	M	Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp
	M	Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
	M	Source/JavaScriptCore/wasm/WasmCallee.cpp
	M	Source/JavaScriptCore/wasm/WasmCallingConvention.h
	M	Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp
	M	Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/Source/JavaScriptCore/ChangeLog

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
W: c17d27071776d4c1366a574be5116c8385a441fb and refs/remotes/origin/master differ, using rebase:
:040000 040000 101347839c6ff738e01da905a47effd032f6ce01 6dc63a9063f89f58843cdd62ecebaef41809a1e3 M	JSTests
:040000 040000 783432c83ce2b4992cfb5a54fcc357e038af7acf d4d6d1a748d902bdd47fc11fe5cbeee1b517e7af M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	A	JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js
	A	JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js
	M	JSTests/ChangeLog
	M	Source/JavaScriptCore/ChangeLog
	M	Source/JavaScriptCore/llint/WebAssembly.asm
	M	Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp
	M	Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
	M	Source/JavaScriptCore/wasm/WasmCallee.cpp
	M	Source/JavaScriptCore/wasm/WasmCallingConvention.h
	M	Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp
	M	Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:

    The following files contain tab characters:

        trunk/Source/JavaScriptCore/ChangeLog

    Please use spaces instead to indent.
    If you must commit a file with tabs, use svn propset to set the "allow-tabs" property.
W: c17d27071776d4c1366a574be5116c8385a441fb and refs/remotes/origin/master differ, using rebase:
:040000 040000 101347839c6ff738e01da905a47effd032f6ce01 6dc63a9063f89f58843cdd62ecebaef41809a1e3 M	JSTests
:040000 040000 783432c83ce2b4992cfb5a54fcc357e038af7acf d4d6d1a748d902bdd47fc11fe5cbeee1b517e7af M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From git://git.webkit.org/WebKit
   95f1f31e946..668ca0464f1  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 256659 = 95f1f31e94601acc0d0cc684c6b1b6cd5683dafd
r256660 = 8ef01ff76e352dde3454b9ac771accddea4762f0
r256661 = 668ca0464f108dda860c55e3b6f80a41db119ada
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: https://webkit-queues.webkit.org/results/13323149

[Tadeu Zagallo]

Created attachment 390846 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 390846 [details]
Patch for landing

Clearing flags on attachment: 390846

Committed r256665: <https://trac.webkit.org/changeset/256665>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tadeu Zagallo]

Committed r256698: <https://trac.webkit.org/changeset/256698>

[Mark Lam]

*** Bug 207336 has been marked as a duplicate of this bug. ***

[Walter Stumpf]

Thanks for the quick fix!  You guys have been extremely responsive to my wasm reports; it's always been a great experience! 

If I understand this bug correctly, can I just workaround the issue by disabling memory growth and using a large static memory value (basically what had to be done with asm.js)?  Is any memory growth problematic or can we simply workaround the issue by having a larger initial starting value (and keep growth enabled)?  Any advice or guidance here would be greatly appreciated. 

We were hoping to have a workaround prepared if this doesn't land before 13.4 and it seems like that's possible!  Thanks again! :) 

-Walter

[Mark Lam]

(In reply to Walter Stumpf from comment #16)
> Thanks for the quick fix!  You guys have been extremely responsive to my
> wasm reports; it's always been a great experience! 
> 
> If I understand this bug correctly, can I just workaround the issue by
> disabling memory growth and using a large static memory value (basically
> what had to be done with asm.js)?  Is any memory growth problematic or can
> we simply workaround the issue by having a larger initial starting value
> (and keep growth enabled)?  Any advice or guidance here would be greatly
> appreciated. 
> 
> We were hoping to have a workaround prepared if this doesn't land before
> 13.4 and it seems like that's possible!  Thanks again! :) 
> 
> -Walter

I think on iOS, not growing memory will probably work around the issue for you.  On Mac, there are other issues that you won't be able to work around.

[Walter Stumpf]

Well at least I can confirm disabling memory growth and setting a static memory value gets the application to load on iOS 13.4 beta!  Obviously no thorough QA going on but at first glance it seems to work. 

And I know this isn't really related to this bug, but thank you guys big time for the wasm interpreter!!!  I just refreshed the page 7 times in a row and our wasm application loaded every time on my iPad!  We'd be lucky if it would reload once without running out of compiler memory on previous iOS versions.  This is a HUGE improvement for our use case (not to mention it loads much quicker!).  I can't express our gratitude enough!     

-Walter

[Walter Stumpf]

Just wanted to confirm 13.4 beta 4 appears to fully resolve this issue.  Thanks again for the insanely quick turnaround, you guys are the best!

-Walter