Bug 96893

Summary: 32-bit LLInt get_by_val does vector length checks incorrectly
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch mhahnenberg: review+

Description Mark Lam 2012-09-16 22:52:43 PDT
The butterflies (r128400) are causing crashes in the 32-bit build.  I've confirmed this by running the JS part of the layout as follows:

$ ./Tools/Scripts/run-webkit-tests --debug --32-bit fast/js fast/regex ietestcenter/Javascript sputnik

The crashes started to manifest on r128400.  They do not on r128369.

Here's what an example call trace looks like:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00724ad7 llint_op_get_by_val + 150
1   com.apple.JavaScriptCore      	0x004fa434 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*) + 100 (JITCode.h:134)
2   com.apple.JavaScriptCore      	0x004f61d6 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 5814 (Interpreter.cpp:991)
3   com.apple.JavaScriptCore      	0x003a4e7f JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) + 559 (Completion.cpp:75)
4   com.apple.WebCore             	0x02271c98 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) + 120 (JSMainThreadExecState.h:77)
5   com.apple.WebCore             	0x02b0ce4e WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld*) + 350 (ScriptController.cpp:148)
6   com.apple.WebCore             	0x02b0cf91 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 65 (ScriptController.cpp:165)
7   com.apple.WebCore             	0x02b2659a WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 794 (ScriptElement.cpp:301)
8   com.apple.WebCore             	0x02b250fa WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1770 (ScriptElement.cpp:241)
9   com.apple.WebCore             	0x01d5c7ce WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 510 (HTMLScriptRunner.cpp:292)
...

From running the C++ llint, I found that the crash occurs here:
_llint_op_get_by_val:
    traceExecution()
    loadi 8[PC], t2
    loadi 12[PC], t3
    loadConstantOrVariablePayload(t2, CellTag, t0, .opGetByValSlow)
    loadConstantOrVariablePayload(t3, Int32Tag, t1, .opGetByValSlow)
    loadp JSCell::m_structure[t0], t3
    loadp 16[PC], t2
    if VALUE_PROFILER
        storep t3, ArrayProfile::m_lastSeenStructure[t2]
    end
    btpz Structure::m_indexingType[t3], HasArrayStorage, .opGetByValSlow
    loadp JSObject::m_butterfly[t0], t3
    biaeq t1, -sizeof IndexingHeader + IndexingHeader::m_vectorLength[t0], .opGetByValSlow
    loadi 4[PC], t0
    loadi ArrayStorage::m_vector + TagOffset[t3, t1, 8], t2      // <=== Crashed here.
    loadi ArrayStorage::m_vector + PayloadOffset[t3, t1, 8], t1
    bieq t2, EmptyValueTag, .opGetByValSlow

The crash is at the above line in LowLevelInterpreter32_64.asm.  The crash does not manifest consistently on any one test (except when I run it in the C++ llint on the Windows port).  But if I run those 4 tests together (fast/js fast/regex ietestcenter/Javascript sputnik), the crash will manifest in several tests, usually a few in ietestcenter/... and many more in sputnik/.…

To repro the crashes, you will need to set useJIT() and useDFGJIT() to false in Options.cpp and run interpreted only.
Comment 1 Geoffrey Garen 2012-09-17 09:39:51 PDT
<rdar://problem/12311678>
Comment 2 Filip Pizlo 2012-09-17 14:37:38 PDT
This appears fixed as of http://trac.webkit.org/changeset/128790
Comment 3 Mark Lam 2012-09-17 15:03:24 PDT
Filip managed to repro the issue.
Comment 4 Filip Pizlo 2012-09-17 16:34:22 PDT
Created attachment 164473 [details]
the patch
Comment 5 Mark Hahnenberg 2012-09-17 16:35:04 PDT
Comment on attachment 164473 [details]
the patch

r=me
Comment 6 Filip Pizlo 2012-09-17 16:36:57 PDT
Landed in http://trac.webkit.org/changeset/128822