Bug 136165

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

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.

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.

Created attachment 237016 [details] Patch

Comment on attachment 237016 [details] Patch Is it safe to start allocating registers after we've emitted all that code?

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

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.

Created attachment 237132 [details] Updated patch that always takes the slow path for X86

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

Comment on attachment 237132 [details] Updated patch that always takes the slow path for X86 r=me

*** Bug 136124 has been marked as a duplicate of this bug. ***

Committed r172959: <http://trac.webkit.org/changeset/172959>

(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

(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"