Bug 165591 - WebAssembly: JSC::link* shouldn't need a CodeBlock
Summary: WebAssembly: JSC::link* shouldn't need a CodeBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on: 165118
Blocks: 161709 165639
  Show dependency treegraph
 
Reported: 2016-12-07 21:33 PST by JF Bastien
Modified: 2016-12-08 22:53 PST (History)
8 users (show)

See Also:


Attachments
patch (6.38 KB, patch)
2016-12-08 10:36 PST, JF Bastien
no flags Details | Formatted Diff | Diff
WIP (9.72 KB, patch)
2016-12-08 13:42 PST, JF Bastien
jfbastien: commit-queue-
Details | Formatted Diff | Diff
patch (19.10 KB, patch)
2016-12-08 19:24 PST, JF Bastien
keith_miller: review+
Details | Formatted Diff | Diff
patch (19.21 KB, patch)
2016-12-08 19:59 PST, JF Bastien
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch (19.20 KB, patch)
2016-12-08 20:27 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-12-07 21:33:46 PST
test_Imports.js (and all imports) are currently blocked on this being fixed because the wasm -> JS stub has an IC, and on first call it fails because the CodeBlock for the stub's caller frame is currently 0.

As a follow-up to but #165118, teach JSC::linkFor to live without a CodeBlock: wasm doesn't have one and the IC patching is sad. We'll switch on the callee (or its type?) and then use that as the owner (because the callee is alive if the instance is alive, ditto module, and module owns the CallLinkInfo).

This means an extra test in JSC::linkFor, but then all wasm frames are free to stop having space for CodeBlock, and we don't need to allocate a wasm CodeBlock at all.
Comment 1 Saam Barati 2016-12-08 00:04:19 PST
FWIW, you might have to teach more than linkFor about not having a caller CodeBlock. I would leave this bug more general for now until you audit all the JS call related machinery to ensure nothing other than linkFor expects a valid caller CodeBlock
Comment 2 JF Bastien 2016-12-08 10:36:40 PST
Created attachment 296533 [details]
patch

Is there more code that needs to get audited? This seems to work: I'm calling JS -> wasm -> JS in a loop, and then back.

Before fixing this, the crash was:

Crash before fixing JSC::linkFor:

* thread #1: tid = 0x6c58df, 0x000000010069deec JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
    frame #0: 0x000000010069deec JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355
   352 	    ExecutableBase* ownerExecutable() const { return m_ownerExecutable.get(); }
   353 	    ScriptExecutable* ownerScriptExecutable() const { return jsCast<ScriptExecutable*>(m_ownerExecutable.get()); }
   354 	
-> 355 	    VM* vm() const { return m_vm; }
   356 	
   357 	    void setThisRegister(VirtualRegister thisRegister) { m_thisRegister = thisRegister; }
   358 	    VirtualRegister thisRegister() const { return m_thisRegister; }
(lldb) bt
* thread #1: tid = 0x6c58df, 0x000000010069deec JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
  * frame #0: 0x000000010069deec JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355
    frame #1: 0x0000000100f86c2a JavaScriptCore`JSC::linkFor(exec=0x00007fff5fbfd8f0, callLinkInfo=0x0000000104874880, calleeCodeBlock=0x0000000105caefe0, callee=0x0000000105cc2920, codePtr=(m_value = 0x000049a08f801640)) + 122 at Repatch.cpp:567
    frame #2: 0x0000000100f4901c JavaScriptCore`::operationLinkCall(execCallee=0x00007fff5fbfd8f0, callLinkInfo=0x0000000104874880) + 1900 at JITOperations.cpp:927
Comment 3 JF Bastien 2016-12-08 10:48:12 PST
This patch is based on changes from #165118. I can rebase from master instead if you think I should commit this before #165118.
Comment 4 JF Bastien 2016-12-08 11:49:58 PST
Saam points out that I should also test the polymorphic and virtual IC call paths, as they may have the same issue as linkFor. I'll do this by creating multiple instances (2 and 3 for poly, more for virt). They can't be with the same instance since imports are captures in the closure at instance creation time, but stubs are shared between instances (so they'll hit poly / virt).
Comment 5 JF Bastien 2016-12-08 13:42:43 PST
Created attachment 296560 [details]
WIP

Bug #165118 made it through. Here's a rebased patch, with the test updated as suggested. Saam was right, there's more code to audit. Current failure is:

* thread #1: tid = 0x70995a, 0x000000010069d92c JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
    frame #0: 0x000000010069d92c JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355
   352 	    ExecutableBase* ownerExecutable() const { return m_ownerExecutable.get(); }
   353 	    ScriptExecutable* ownerScriptExecutable() const { return jsCast<ScriptExecutable*>(m_ownerExecutable.get()); }
   354 	
-> 355 	    VM* vm() const { return m_vm; }
   356 	
   357 	    void setThisRegister(VirtualRegister thisRegister) { m_thisRegister = thisRegister; }
   358 	    VirtualRegister thisRegister() const { return m_thisRegister; }
(lldb) bt
* thread #1: tid = 0x70995a, 0x000000010069d92c JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x38)
  * frame #0: 0x000000010069d92c JavaScriptCore`JSC::CodeBlock::vm(this=0x0000000000000000) const + 12 at CodeBlock.h:355
    frame #1: 0x0000000100f87580 JavaScriptCore`JSC::linkPolymorphicCall(exec=0x00007fff5fbfd950, callLinkInfo=0x00000001048a2780, newVariant=CallVariant @ 0x00007fff5fbfce70) + 208 at Repatch.cpp:691
    frame #2: 0x0000000100f49e08 JavaScriptCore`::operationLinkPolymorphicCall(execCallee=0x00007fff5fbfd950, callLinkInfo=0x00000001048a2780) + 152 at JITOperations.cpp:1034


I'll get on that now!
Comment 6 JF Bastien 2016-12-08 14:05:58 PST
I'll need to fix at least linkPolymorphicCall and linkVirtualFor.
Comment 7 JF Bastien 2016-12-08 19:24:37 PST
Created attachment 296618 [details]
patch

🎉 It works. wasm can call into JS polymorphically and virtually, get all that great IC benefit, amazing speed. 🎉
Comment 8 Keith Miller 2016-12-08 19:43:44 PST
Comment on attachment 296618 [details]
patch

r=me.
Comment 9 JF Bastien 2016-12-08 19:59:48 PST
Created attachment 296626 [details]
patch

Fix build when ENABLE(WEBASSEMBLY) == false.
Comment 10 WebKit Commit Bot 2016-12-08 20:20:17 PST
Comment on attachment 296626 [details]
patch

Rejecting attachment 296626 [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-01', 'validate-changelog', '--check-oops', '--non-interactive', 296626, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in JSTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/2660694
Comment 11 JF Bastien 2016-12-08 20:27:53 PST
Created attachment 296630 [details]
patch

Fix OOPS.
Comment 12 WebKit Commit Bot 2016-12-08 22:53:20 PST
Comment on attachment 296630 [details]
patch

Clearing flags on attachment: 296630

Committed r209597: <http://trac.webkit.org/changeset/209597>
Comment 13 WebKit Commit Bot 2016-12-08 22:53:24 PST
All reviewed patches have been landed.  Closing bug.