Bug 207727

Summary: [WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention.
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, sbarati, tzagallo, walter, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Michael Saboff 2020-02-13 15:38:15 PST
It appears that a Wasm JIT function doesn't get an updated memory base and size. when we grow memory.
Comment 1 Michael Saboff 2020-02-13 15:38:28 PST
<rdar://problem/59234854>
Comment 2 Michael Saboff 2020-02-13 15:44:18 PST
Created attachment 390699 [details]
Patch
Comment 3 Mark Lam 2020-02-13 17:30:24 PST
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.
Comment 4 Tadeu Zagallo 2020-02-14 15:28:46 PST
Created attachment 390829 [details]
Patch
Comment 5 Mark Lam 2020-02-14 15:57:00 PST
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.
Comment 6 Mark Lam 2020-02-14 15:59:51 PST
(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/.
Comment 7 Tadeu Zagallo 2020-02-14 16:44:56 PST
Created attachment 390835 [details]
Patch for landing
Comment 8 Tadeu Zagallo 2020-02-14 16:46:09 PST
Created attachment 390836 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2020-02-14 17:52:26 PST
The commit-queue encountered the following flaky tests while processing attachment 390836 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2020-02-14 17:53:02 PST
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 commit-queue@webkit.org...
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
Comment 11 Tadeu Zagallo 2020-02-14 18:15:31 PST
Created attachment 390846 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2020-02-14 18:57:54 PST
Comment on attachment 390846 [details]
Patch for landing

Clearing flags on attachment: 390846

Committed r256665: <https://trac.webkit.org/changeset/256665>
Comment 13 WebKit Commit Bot 2020-02-14 18:57:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Tadeu Zagallo 2020-02-14 19:19:22 PST
Committed r256698: <https://trac.webkit.org/changeset/256698>
Comment 15 Mark Lam 2020-02-15 13:58:47 PST
*** Bug 207336 has been marked as a duplicate of this bug. ***
Comment 16 Walter Stumpf 2020-02-15 16:17:45 PST
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
Comment 17 Mark Lam 2020-02-15 16:38:31 PST
(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.
Comment 18 Walter Stumpf 2020-02-15 17:32:54 PST
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
Comment 19 Walter Stumpf 2020-03-04 09:42:00 PST
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