Bug 116889 – fourthTier: The baseline jit and LLint should use common slow paths

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

[Michael Saboff]

To reduce the number of trampoline methods and generally clean up the code in preparation for utilizing the C stack, we should reduce the number of slow paths.  This change is to eliminate nearly identical slow paths.

[Michael Saboff]

Created attachment 203095 [details]
Work in progress

This patch compiles and runs correctly on X86 and X86_64.  Working through ARMv7 issues.

[Michael Saboff]

Created attachment 203106 [details]
Patch

Builds and passes JSC tests on X86_64 and X86.  Speculative fixes for other platforms.

[Mark Lam]

Comment on attachment 203106 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203106&action=review

Do you have benchmark numbers to confirm that this does not impact perf?

> Source/JavaScriptCore/interpreter/Interpreter.cpp:716
> +//    ASSERT(callFrame == vm->topCallFrame || callFrame == callFrame->lexicalGlobalObject()->globalExec() || callFrame == callFrame->dynamicGlobalObject()->globalExec());

Why comment out this assert?

> Source/JavaScriptCore/jit/JITExceptions.h:43
> +    // 

Why the blank comment?

[Michael Saboff]

Created attachment 203289 [details]
Updated patch addressing reviewer comments

I had commented out the ASSERT during development.  Fixed the reason that it was failing on 32_64 builds by adding in the storePtr(callFrameRegister, &m_vm->topCallFrame); back in at the end of privateCompileCTINativeCall().

Also removed the bogus empty comment.

[Filip Pizlo]

Comment on attachment 203289 [details]
Updated patch addressing reviewer comments

Shouldn't there be a bunch of stuff removed from JITStubs.cpp?

[Michael Saboff]

Created attachment 203617 [details]
Patch with redundant hit stubs removed

(In reply to comment #5)
> (From update of attachment 203289 [details])
> Shouldn't there be a bunch of stuff removed from JITStubs.cpp?

Removed.

[Michael Saboff]

Created attachment 204074 [details]
Patch for landing with minor merge up changes.

[Michael Saboff]

Committed r151342: <http://trac.webkit.org/changeset/151342>

[Filip Pizlo]

I'm pretty sure this broke Kraken in debug build.

[Filip Pizlo]

Rolled out in http://trac.webkit.org/changeset/151345

[Michael Saboff]

(In reply to comment #9)
> I'm pretty sure this broke Kraken in debug build.

Looks like there were a couple of emitSlow_op_*() functions where I didn't load the return register (rax on X86_64) with the result of the operation after the call.  I fixed the ones I found and kraken works.  I'll spend a little more time testing before relanding.

[Geoffrey Garen]

This is blocking the patch I'm working on, since I built on top of JITSlowPathCall :(.

[Geoffrey Garen]

I took your diagnosis and fixed and re-landed this patch in <http://trac.webkit.org/changeset/151362>.

[Geoffrey Garen]

Darn, looks like this is still crashing at r151362:

run-webkit-tests --debug --verbose fast/workers/stress-js-execution.html fast/workers/termination-early.html fast/workers/stress-js-execution.html fast/workers/termination-early.html

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef

Thread 14 Crashed:: WebCore: Worker
0   com.apple.JavaScriptCore      	0x000000010954c9d0 JSC::JSActivation::tearOff(JSC::VM&) + 96 (JSActivation.h:154)
1   com.apple.JavaScriptCore      	0x0000000109547466 JSC::Interpreter::unwindCallFrame(JSC::ExecState*&, JSC::JSValue, unsigned int&, JSC::CodeBlock*&) + 550 (Interpreter.cpp:499)
2   com.apple.JavaScriptCore      	0x0000000109548841 JSC::Interpreter::throwException(JSC::ExecState*&, JSC::JSValue&, unsigned int) + 1137 (Interpreter.cpp:779)
3   com.apple.JavaScriptCore      	0x000000010956be06 JSC::genericThrow(JSC::VM*, JSC::ExecState*, JSC::JSValue, unsigned int) + 166 (JITExceptions.cpp:56)
4   com.apple.JavaScriptCore      	0x000000010956bf6c JSC::jitThrowNew(JSC::VM*, JSC::ExecState*, JSC::JSValue) + 60 (JITExceptions.cpp:80)
5   com.apple.JavaScriptCore      	0x000000010958f37c cti_vm_throw_slowpath + 60 (JITStubs.cpp:2230)
6   com.apple.JavaScriptCore      	0x0000000109584db9 ctiVMThrowTrampolineSlowpath + 8

[Geoffrey Garen]

Rolled out in r151401.

[Michael Saboff]

Fixed test failure and rolled back in with change set <https://trac.webkit.org/changeset/151504>

Fixed involved NOT processing exceptions through the llint path when we call a common slow path from the JIT.