Bug 165591 – WebAssembly: JSC::link* shouldn't need a CodeBlock

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

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.

[Saam Barati]

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

[JF Bastien]

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

[JF Bastien]

This patch is based on changes from #165118. I can rebase from master instead if you think I should commit this before #165118.

[JF Bastien]

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

[JF Bastien]

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!

[JF Bastien]

I'll need to fix at least linkPolymorphicCall and linkVirtualFor.

[JF Bastien]

Created attachment 296618 [details]
patch

🎉 It works. wasm can call into JS polymorphically and virtually, get all that great IC benefit, amazing speed. 🎉

[Keith Miller]

Comment on attachment 296618 [details]
patch

r=me.

[JF Bastien]

Created attachment 296626 [details]
patch

Fix build when ENABLE(WEBASSEMBLY) == false.

[WebKit Commit Bot]

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

[JF Bastien]

Created attachment 296630 [details]
patch

Fix OOPS.

[WebKit Commit Bot]

Comment on attachment 296630 [details]
patch

Clearing flags on attachment: 296630

Committed r209597: <http://trac.webkit.org/changeset/209597>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.