RESOLVED FIXED 136165
REGRESSION(r172794) + 32Bit build: ASSERT failures in for-in-tests.js tests.
https://bugs.webkit.org/show_bug.cgi?id=136165
Summary REGRESSION(r172794) + 32Bit build: ASSERT failures in for-in-tests.js tests.
Michael Saboff
Reported 2014-08-22 11:48:33 PDT
The following JavaScriptCore regression tests are failing after r172794 <http://trac.webkit.org/changeset/172794>: stress/for-in-base-reassigned-later-and-change-structure.js.always-trigger-copy-phase stress/for-in-base-reassigned-later-and-change-structure.js.default stress/for-in-base-reassigned-later-and-change-structure.js.default-ftl stress/for-in-base-reassigned-later-and-change-structure.js.dfg-eager stress/for-in-base-reassigned-later-and-change-structure.js.dfg-eager-no-cjit-validate stress/for-in-base-reassigned-later-and-change-structure.js.ftl-eager stress/for-in-base-reassigned-later-and-change-structure.js.ftl-eager-no-cjit stress/for-in-base-reassigned-later-and-change-structure.js.ftl-no-cjit-no-inline-validate stress/for-in-base-reassigned-later-and-change-structure.js.ftl-no-cjit-validate stress/for-in-base-reassigned-later-and-change-structure.js.no-cjit-validate-phases stress/for-in-base-reassigned-later-and-change-structure.js.no-llint stress/for-in-tests.js.always-trigger-copy-phase stress/for-in-tests.js.default stress/for-in-tests.js.default-ftl stress/for-in-tests.js.dfg-eager stress/for-in-tests.js.dfg-eager-no-cjit-validate stress/for-in-tests.js.ftl-eager stress/for-in-tests.js.ftl-eager-no-cjit stress/for-in-tests.js.ftl-no-cjit-no-inline-validate stress/for-in-tests.js.ftl-no-cjit-validate stress/for-in-tests.js.no-cjit-validate-phases stress/for-in-tests.js.no-llint The "for-in-base-reassigned-later-and-change-structure.js" fail with Exception: Error: bad result: NaN The "for-in-tests" fail with ASSERTION FAILED: currentLowest != NUM_REGS && currentSpillOrder != SpillHintInvalid
Attachments
Patch (7.54 KB, patch)
2014-08-22 17:37 PDT, Michael Saboff
no flags
Updated patch that always takes the slow path for X86 (3.32 KB, patch)
2014-08-25 21:05 PDT, Michael Saboff
mhahnenb: review+
Michael Saboff
Comment 1 2014-08-22 11:51:17 PDT
Here is a complete stack trace for a for-in-tests crash: stress/for-in-tests.js.default-ftl: ASSERTION FAILED: currentLowest != NUM_REGS && currentSpillOrder != SpillHintInvalid stress/for-in-tests.js.default-ftl: /Volumes/Data/src/webkit/Source/JavaScriptCore/dfg/DFGRegisterBank.h(138) : RegID JSC::DFG::RegisterBank<JSC::GPRInfo>::allocate(JSC::VirtualRegister &) [BankInfo = JSC::GPRInfo] stress/for-in-tests.js.default-ftl: 1 0x98486d WTFCrash stress/for-in-tests.js.default-ftl: 2 0x46ab48 JSC::DFG::RegisterBank<JSC::GPRInfo>::allocate(JSC::VirtualRegister&) stress/for-in-tests.js.default-ftl: 3 0x446b95 JSC::DFG::SpeculativeJIT::allocate() stress/for-in-tests.js.default-ftl: 4 0x47479e JSC::DFG::SpeculativeJIT::fillSpeculateCell(JSC::DFG::Edge) stress/for-in-tests.js.default-ftl: 5 0x4466c1 JSC::DFG::SpeculateCellOperand::gpr() stress/for-in-tests.js.default-ftl: 6 0x49504e JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*) stress/for-in-tests.js.default-ftl: 7 0x428b75 JSC::DFG::SpeculativeJIT::compileCurrentBlock() stress/for-in-tests.js.default-ftl: 8 0x4294b2 JSC::DFG::SpeculativeJIT::compile() stress/for-in-tests.js.default-ftl: 9 0x3a3ad0 JSC::DFG::JITCompiler::compileBody() stress/for-in-tests.js.default-ftl: 10 0x3a5ccd JSC::DFG::JITCompiler::compileFunction() stress/for-in-tests.js.default-ftl: 11 0x4144c7 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&) stress/for-in-tests.js.default-ftl: 12 0x4139c4 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*) stress/for-in-tests.js.default-ftl: 13 0x35e94d JSC::DFG::compileImpl(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue, JSC::OperandValueTraits<JSC::JSValue> > const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>) stress/for-in-tests.js.default-ftl: 14 0x35e1a2 JSC::DFG::compile(JSC::VM&, JSC::CodeBlock*, JSC::CodeBlock*, JSC::DFG::CompilationMode, unsigned int, JSC::Operands<JSC::JSValue, JSC::OperandValueTraits<JSC::JSValue> > const&, WTF::PassRefPtr<JSC::DeferredCompilationCallback>) stress/for-in-tests.js.default-ftl: 15 0x5dba49 operationOptimize stress/for-in-tests.js.default-ftl: 16 0x1f862c4 stress/for-in-tests.js.default-ftl: 17 0x1f884c0 stress/for-in-tests.js.default-ftl: 18 0x737c3c llint_entry stress/for-in-tests.js.default-ftl: 19 0x732d85 vmEntryToJavaScript stress/for-in-tests.js.default-ftl: 20 0x5c28f0 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) stress/for-in-tests.js.default-ftl: 21 0x59f39f JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) stress/for-in-tests.js.default-ftl: 22 0x2118bf JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, JSC::JSValue*) stress/for-in-tests.js.default-ftl: 23 0x86c86 runWithScripts(GlobalObject*, WTF::Vector<Script, 0ul, WTF::CrashOnOverflow> const&, bool) stress/for-in-tests.js.default-ftl: 24 0x86130 jscmain(int, char**) stress/for-in-tests.js.default-ftl: 25 0x85ea9 main stress/for-in-tests.js.default-ftl: 26 0x92907701 start stress/for-in-tests.js.default-ftl: 27 0x5 stress/for-in-tests.js.default-ftl: test_script_8184: line 2: 87667 Segmentation fault: 11 "$@" ../../.vm/JavaScriptCore.framework/Resources/jsc --useFTLJIT\=false --useFTLJIT\=true --enableExperimentalFTLCoverage\=true for-in-tests.js stress/for-in-tests.js.default-ftl: ERROR: Unexpected exit code: 139
Michael Saboff
Comment 2 2014-08-22 13:28:12 PDT
For the ASSERT failure in for-in-tests.js, looks like we are running out of registers in DFG::SpeculativeJIT::compile as part of case GetDirectPname. The code wants 7 registers and X86 32 bit only has 6 to use. Restructuring to use one less register.
Michael Saboff
Comment 3 2014-08-22 17:29:33 PDT
Using this bug to track the ASSERT failures in for-in-tests.js Created clone <https://bugs.webkit.org/show_bug.cgi?id=136187> REGRESSION(r172794) + 32Bit build: for-in-base-reassigned-later-and-change-structure.js fail with NaN result to track the other failure.
Michael Saboff
Comment 4 2014-08-22 17:37:54 PDT
Mark Hahnenberg
Comment 5 2014-08-23 14:08:08 PDT
Comment on attachment 237016 [details] Patch Is it safe to start allocating registers after we've emitted all that code?
Michael Saboff
Comment 6 2014-08-23 21:42:33 PDT
(In reply to comment #5) > (From update of attachment 237016 [details]) > Is it safe to start allocating registers after we've emitted all that code? Why not? The two registers in the block, indexGRP and enumeratorGPR, will be available for use when the block goes out of scope. At that point we'll have two free registers, but we only need one, propertyGRP. The other four registers stay allocated the whole time. When propertyGPR is allocated, it most likely will be filled from its spill location due to a total of 6 live registers while in the block. BTW, do you know why the line: m_jit.move(MacroAssembler::TrustedImm32(JSValue::CellTag), scratchGPR); at the end of the modified sections is there? Seems like scratch reg is never used after we set it to the CellTag.
Filip Pizlo
Comment 7 2014-08-23 22:31:51 PDT
Comment on attachment 237016 [details] Patch I don't like this approach. It adds a lot of complexity just for x86-32, which is a rarely used architecture for us. Arm doesn't have this problem. It would be better, I think, to have an x86 special case where we just always call slow, and then do what ToT does on other architectures. That's how we have fixed such problems in the past.
Michael Saboff
Comment 8 2014-08-25 21:05:57 PDT
Created attachment 237132 [details] Updated patch that always takes the slow path for X86
Mark Hahnenberg
Comment 9 2014-08-26 07:41:54 PDT
(In reply to comment #8) > Created an attachment (id=237132) [details] > Updated patch that always takes the slow path for X86 Looks like you forgot to mark it as a patch.
Mark Hahnenberg
Comment 10 2014-08-26 07:42:42 PDT
Comment on attachment 237132 [details] Updated patch that always takes the slow path for X86 r=me
Mark Lam
Comment 11 2014-08-26 08:29:22 PDT
*** Bug 136124 has been marked as a duplicate of this bug. ***
Michael Saboff
Comment 12 2014-08-26 08:56:18 PDT
Csaba Osztrogonác
Comment 13 2014-08-26 10:57:47 PDT
(In reply to comment #7) > (From update of attachment 237016 [details]) > I don't like this approach. It adds a lot of complexity just for x86-32, which is a rarely used architecture for us. Arm doesn't have this problem. It would be better, I think, to have an x86 special case where we just always call slow, and then do what ToT does on other architectures. That's how we have fixed such problems in the past. ARM does have this issues as I mentioned in https://bugs.webkit.org/show_bug.cgi?id=136124#c0
Michael Saboff
Comment 14 2014-08-26 11:05:20 PDT
(In reply to comment #13) > (In reply to comment #7) > > (From update of attachment 237016 [details] [details]) > > I don't like this approach. It adds a lot of complexity just for x86-32, which is a rarely used architecture for us. Arm doesn't have this problem. It would be better, I think, to have an x86 special case where we just always call slow, and then do what ToT does on other architectures. That's how we have fixed such problems in the past. > > ARM does have this issues as I mentioned in https://bugs.webkit.org/show_bug.cgi?id=136124#c0 The issues with for-in-base-reassigned-later-and-change-structure.js are addressed in <https://bugs.webkit.org/show_bug.cgi?id=136187> - "REGRESSION(r172794) + 32Bit build: for-in-base-reassigned-later-and-change-structure.js fail with NaN result"
Note You need to log in before you can comment on or make changes to this bug.