Bug 159419 – [ARMv7] ASSERTION FAILED: (cond == Zero) || (cond == NonZero)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/197655 modified the code in compileArithMul()
to check negative zero:
-            speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branch32(MacroAssembler::LessThan, reg1, TrustedImm32(0)));
-            speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branch32(MacroAssembler::LessThan, reg2, TrustedImm32(0)));
+            speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Signed, reg1));
+            speculationCheck(NegativeZero, JSValueRegs(), 0, m_jit.branchTest32(MacroAssembler::Signed, reg2));

Using branchTest32 with signed conditional flag is incorrect, because the underlying 
tst ARM assembly instruction doesn't affect the V (signed/overflow) flag at all.

The MacroAssemblerARM.h asserts in this case properly.

ASSERTION FAILED: (cond == Zero) || (cond == NonZero)
../../Source/JavaScriptCore/assembler/MacroAssemblerARM.h(714) : JSC::AbstractMacroAssembler<JSC::ARMAssembler, JSC::MacroAssemblerARM>::Jump JSC::MacroAssemblerARM::branchTest32(JSC::MacroAssemblerARM::ResultCondition, JSC::AbstractMacroAssembler<JSC::ARMAssembler, JSC::MacroAssemblerARM>::RegisterID, JSC::AbstractMacroAssembler<JSC::ARMAssembler, JSC::MacroAssemblerARM>::TrustedImm32)
1   0x41af4060 WTFCrash
2   0x40fa0a20 JSC::MacroAssemblerARM::branchTest32(JSC::MacroAssemblerARM::ResultCondition, JSC::ARMRegisters::RegisterID, JSC::AbstractMacroAssembler<JSC::ARMAssembler, JSC::MacroAssemblerARM>::TrustedImm32)
3   0x413ab1e0 JSC::DFG::SpeculativeJIT::compileArithMul(JSC::DFG::Node*)
4   0x41410fec JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*)
5   0x4139f5ec JSC::DFG::SpeculativeJIT::compileCurrentBlock()
6   0x4139fcec JSC::DFG::SpeculativeJIT::compile()
7   0x4127220c JSC::DFG::JITCompiler::compileBody()
8   0x41273e04 JSC::DFG::JITCompiler::compileFunction()
9   0x4134ed18 JSC::DFG::Plan::compileInThreadImpl(JSC::DFG::LongLivedState&)
10  0x4134e078 JSC::DFG::Plan::compileInThread(JSC::DFG::LongLivedState&, JSC::DFG::ThreadData*)
11  0x411f2e88
12  0x411f2fec 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>)
13  0x41600004
Illegal instruction
ERROR: Unexpected exit code: 132

[Csaba Osztrogonác]

Created attachment 282794 [details]
Patch

WIP patch, it fixes only the traditional ARM instruction set, will investigate the Thumb2 too.

[Csaba Osztrogonác]

(In reply to comment #0)
> Using branchTest32 with signed conditional flag is incorrect, 
> because the underlying tst ARM assembly instruction doesn't 
> affect the V (signed/overflow) flag at all.

Ah, I was wrong. Not the V, but N flag is responsible for 
signed conditional which is really updated by the tst instruction.
So it's not a regression, but an incomplete assertion. Fix is coming.

[Csaba Osztrogonác]

Created attachment 282863 [details]
Patch

[Benjamin Poulain]

Comment on attachment 282863 [details]
Patch

LGTM but you don't need all those parenthesis, that's excessive.

[Csaba Osztrogonác]

(In reply to comment #4)
> Comment on attachment 282863 [details]
> Patch
> 
> LGTM but you don't need all those parenthesis, that's excessive.

Thanks, I'll remove the extra parenthesis before landing.

[Csaba Osztrogonác]

Created attachment 282996 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 282996 [details]
Patch for landing

Clearing flags on attachment: 282996

Committed r202899: <http://trac.webkit.org/changeset/202899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.