Bug 123848

Summary: crash when executing Thunk checks on ARM
Product: WebKit Reporter: Mandeep Singh Baines <mandeep.baines>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, fpizlo, jbriance, msaboff, ossy, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
the patch
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 none

Description Mandeep Singh Baines 2013-11-05 19:06:00 PST
I'm getting a crash when executing the thunk checks from code generated by virtualForThunkGenerator.

I think this is a regression introduced when JIT was migrated to use the same thunks as DFG:

Author: msaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc
>
Date:   Wed Oct 9 13:29:00 2013 +0000

    Transition call and construct JITStubs to CCallHelper functions
    https://bugs.webkit.org/show_bug.cgi?id=122453
    
    Reviewed by Geoffrey Garen.
    
    Transitioned cti_op_call_eval to operationCallEval.  Migrated baseline JIT to
 use the same
    call thunks as the DFG.  Eliminated all of the "oldStyle" thunks and related 
functions.
    
...    
    
    git-svn-id: http://svn.webkit.org/repository/webkit/trunk@157164 268f45cc-cd0
9-0410-ab3c-d52691b4dbfc

The check is generated here:

<snip>
static MacroAssemblerCodeRef virtualForThunkGenerator(
    VM* vm, CodeSpecializationKind kind)
{
    // The return address is on the stack, or in the link register. We will henc\
e                                                                                
    // jump to the callee, or save the return address to the call frame while we
    // make a C++ function call to the appropriate JIT operation.                

    CCallHelpers jit(vm);

    CCallHelpers::JumpList slowCase;

    // FIXME: we should have a story for eliminating these checks. In many cases\
,                                                                                
    // the DFG knows that the value is definitely a cell, or definitely a functi\
on.                                                                              

#if USE(JSVALUE64)
    slowCase.append(
        jit.branchTest64(
            CCallHelpers::NonZero, GPRInfo::nonArgGPR0, GPRInfo::tagMaskRegister\
));
#else
    slowCase.append(
        jit.branch32(
            CCallHelpers::NotEqual, GPRInfo::nonArgGPR1,
            CCallHelpers::TrustedImm32(JSValue::CellTag)));
#endif
    jit.loadPtr(CCallHelpers::Address(GPRInfo::nonArgGPR0, JSCell::structureOffs\
et()), GPRInfo::nonArgGPR2);
    slowCase.append(
        jit.branchPtr(
            CCallHelpers::NotEqual,
            CCallHelpers::Address(GPRInfo::nonArgGPR2, Structure::classInfoOffse\
t()),
            CCallHelpers::TrustedImmPtr(JSFunction::info())));
</snip>

It is expecting the callee to be stored in nonArgGPR1 and nonArgGPR0.

However, the JIT path is storing the callee is in regT1 and regT0 if you look at JIT::compileOpCall.

This used to work because the check used to be generated by oldStyleLinkForGenerator which checked regT0 and regT1.

-static MacroAssemblerCodeRef oldStyleVirtualForGenerator(VM* vm, FunctionPtr compile, FunctionPtr notJSFunction, const char* name, CodeSpecializationKind kind)
-{
-    JSInterfaceJIT jit(vm);
-    
-    JSInterfaceJIT::JumpList slowCase;
-
-#if USE(JSVALUE64)    
-    slowCase.append(jit.emitJumpIfNotJSCell(JSInterfaceJIT::regT0));
-#else // USE(JSVALUE64)
-    slowCase.append(jit.branch32(JSInterfaceJIT::NotEqual, JSInterfaceJIT::regT1, JSInterfaceJIT::TrustedImm32(JSValue::CellTag)));
-#endif // USE(JSVALUE64)
-    slowCase.append(jit.emitJumpIfNotType(JSInterfaceJIT::regT0, JSInterfaceJIT::regT1, JSFunctionType));
-

I think the correct fix is to modify virtualForThunkGenerator to use regT0 and regT1.
Comment 1 Mandeep Singh Baines 2013-11-06 11:08:06 PST
Created attachment 216194 [details]
the patch

This crash affects all architectures. Due to random luck, I was only see a crash on ARM.

Attached is a fix which changes the thunk to use the regT* convention for storing the callee that was previously used by the baseline JIT thunk.
Comment 2 Build Bot 2013-11-07 08:18:04 PST
Comment on attachment 216194 [details]
the patch

Attachment 216194 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/22718211

New failing tests:
js/regress/emscripten-cube2hash.html
cssom/cssvalue-comparison.html
js/dom/JSON-parse.html
fast/canvas/canvas-blending-pattern-over-pattern.html
inspector-protocol/debugger/call-frame-this-strict.html
js/mozilla/strict/8.12.5.html
fast/canvas/canvas-blending-image-over-image.html
cssom/cssstyledeclaration-csstext-final-delimiter.html
fast/canvas/webgl/array-unit-tests.html
cssom/cssimportrule-media.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
js/mozilla/strict/13.1.html
http/tests/canvas/webgl/origin-clean-conformance.html
fast/regions/webkit-region-syntax-space.html
fast/media/mq-color-index-02.html
inspector-protocol/dom/remove-multiple-nodes.html
fast/media/w3c/test_media_queries.html
fast/dom/Document/CaretRangeFromPoint/hittest-relative-to-viewport.html
jquery/attributes.html
fast/xpath/xpath-iterator-result-should-mark-its-nodeset.html
fast/media/mq-js-update-media.html
inspector-protocol/debugger/setBreakpoint-condition.html
js/mozilla/strict/11.1.5.html
inspector-protocol/debugger/call-frame-this-nonstrict.html
fast/canvas/webgl/arraybuffer-transfer-of-control.html
fast/regions/webkit-flow-into-parsing.html
inspector-protocol/debugger/setBreakpoint-actions.html
js/dom/JSON-stringify.html
jquery/core.html
inspector-protocol/model/content-flow-content-nodes.html
Comment 3 Build Bot 2013-11-07 08:18:06 PST
Created attachment 216303 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Julien Brianceau 2013-11-08 04:47:44 PST
What ARM architecture do you use? If it's ARM_TRADITIONAL, the last stable revision I know is r158882 + changeset r158915
Comment 5 Build Bot 2013-11-08 07:13:43 PST
Comment on attachment 216194 [details]
the patch

Attachment 216194 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/22598522

New failing tests:
cssom/cssvalue-comparison.html
js/dom/JSON-parse.html
fast/canvas/canvas-blending-pattern-over-pattern.html
inspector-protocol/debugger/call-frame-this-strict.html
js/mozilla/strict/8.12.5.html
inspector-protocol/debugger/setBreakpoint-options-exception.html
fast/canvas/canvas-blending-image-over-image.html
cssom/cssstyledeclaration-csstext-final-delimiter.html
fast/canvas/webgl/array-unit-tests.html
cssom/cssimportrule-media.html
fast/dom/SelectorAPI/resig-SelectorsAPI-test.xhtml
http/tests/canvas/webgl/origin-clean-conformance.html
fast/regions/webkit-region-syntax-space.html
fast/media/mq-color-index-02.html
inspector-protocol/dom/remove-multiple-nodes.html
fast/media/w3c/test_media_queries.html
fast/dom/Document/CaretRangeFromPoint/hittest-relative-to-viewport.html
jquery/attributes.html
jquery/data.html
fast/media/mq-js-update-media.html
inspector-protocol/debugger/setBreakpoint-condition.html
js/mozilla/strict/11.1.5.html
inspector-protocol/debugger/call-frame-this-nonstrict.html
fast/canvas/webgl/arraybuffer-transfer-of-control.html
fast/regions/webkit-flow-into-parsing.html
inspector-protocol/debugger/setBreakpoint-actions.html
js/dom/JSON-stringify.html
jquery/core.html
inspector-protocol/model/content-flow-content-nodes.html
jquery/css.html
Comment 6 Build Bot 2013-11-08 07:13:45 PST
Created attachment 216395 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 7 Mandeep Singh Baines 2013-11-08 09:39:42 PST
Hi Julien,

Thanks for the reply. I'm building for ARMv7.

I'm very new to WebKit. This is actually the first time I've sent a patch.

Does the patch look reasonable (minus the tests that seem to be failing)? Still getting my head around JSC.

So there are stable versions? Where is the list of stable versions posted?

Regards,
Mandeep
Comment 8 Julien Brianceau 2013-11-10 08:58:58 PST
(In reply to comment #7)
> I'm very new to WebKit. This is actually the first time I've sent a patch.
That's nice to read !

> Does the patch look reasonable (minus the tests that seem to be failing)? Still getting my head around JSC.
Honestly, I don't think so, see https://bugs.webkit.org/show_bug.cgi?id=123277 for further information about this. Moreover, Mark's already fixed your issue with changeset 158315 (http://trac.webkit.org/changeset/158315).

> So there are stable versions?
Yes, fortunately ;)

> Where is the list of stable versions posted?
As far as I know, there's no such list. If you want to get this information, you might want to set up a buildbot running jsc tests for ARMv7 architecture to track build status and regressions. You also might want to add your bot in the webkit waterfall (http://build.webkit.org/waterfall).
Comment 9 Csaba Osztrogonác 2015-02-05 04:53:05 PST
close, based on Comment8