Bug 207727

<rdar://problem/59234854>

Created attachment 390699 [details] Patch

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.

Created attachment 390829 [details] Patch

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.

(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/.

Created attachment 390835 [details] Patch for landing

Created attachment 390836 [details] Patch for landing

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 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

Created attachment 390846 [details] Patch for landing

Comment on attachment 390846 [details] Patch for landing Clearing flags on attachment: 390846 Committed r256665: <https://trac.webkit.org/changeset/256665>

All reviewed patches have been landed. Closing bug.

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

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

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

(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.

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

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