Bug 136165 – REGRESSION(r172794) + 32Bit build: ASSERT failures in for-in-tests.js tests.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

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

[Michael Saboff]

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]

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]

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]

Created attachment 237016 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 237016 [details]
Patch

Is it safe to start allocating registers after we've emitted all that code?

[Michael Saboff]

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

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

[Mark Hahnenberg]

(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 on attachment 237132 [details]
Updated patch that always takes the slow path for X86

r=me

[Mark Lam]

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

[Michael Saboff]

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

[Csaba Osztrogonác]

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

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