Bug 165591

Summary:

WebAssembly: JSC::link* shouldn't need a CodeBlock

Product:

WebKit

Reporter:

JF Bastien <jfbastien>

Component:

JavaScriptCore

Assignee:

JF Bastien <jfbastien>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, fpizlo, ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

165118

Bug Blocks:

161709, 165639

Attachments:

Description	Flags
patch	none
WIP	jfbastien: commit-queue-
patch	keith_miller: review+
patch	commit-queue: commit-queue-
patch	none

JF Bastien

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

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
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Saam Barati

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

JF Bastien

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

JF Bastien

Comment 3 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.

JF Bastien

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

JF Bastien

Comment 5 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!

JF Bastien

Comment 6 2016-12-08 14:05:58 PST

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

JF Bastien

Comment 7 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. 🎉

Keith Miller

Comment 8 2016-12-08 19:43:44 PST

Comment on attachment 296618 [details] patch r=me.

JF Bastien

Comment 9 2016-12-08 19:59:48 PST

Created attachment 296626 [details] patch Fix build when ENABLE(WEBASSEMBLY) == false.

WebKit Commit Bot

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

JF Bastien

Comment 11 2016-12-08 20:27:53 PST

Created attachment 296630 [details] patch Fix OOPS.

WebKit Commit Bot

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

WebKit Commit Bot

Comment 13 2016-12-08 22:53:24 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.