Bug 112676 – REGRESSION(r146089): It broke 20 sputnik tests on ARM traditional and Thumb2

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

A change between r146057-r146163 broke 20 sputnik tests on ARM traditional,
maybe on ARM Thumb2 too, who knows. See this page for details:
http://build.webkit.sed.hu/results/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/r146163%20%288084%29/results.html

I'm going to bisect the culprit revision.

[Csaba Osztrogonác]

The bug is already valid on r146100.

[Csaba Osztrogonác]

I got it, http://trac.webkit.org/changeset/146089 is the 
culprit or it simple unhid an ARM traditional DFG JIT bug.

[Csaba Osztrogonác]

I checked, this bug is valid on Thumb2 too (with an additional jsc test fail):
- r146089: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/8089
- r146088: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/8090

[Csaba Osztrogonác]

Hmmm, maybe it was already fixed by http://trac.webkit.org/changeset/146179. Let me check it.

[Csaba Osztrogonác]

(In reply to comment #4)
> Hmmm, maybe it was already fixed by http://trac.webkit.org/changeset/146179. Let me check it.

No, it wasn't fixed, the bug is still valid on r146200.

[Geoffrey Garen]

<rdar://problem/13452502>

[Filip Pizlo]

(In reply to comment #5)
> (In reply to comment #4)
> > Hmmm, maybe it was already fixed by http://trac.webkit.org/changeset/146179. Let me check it.
> 
> No, it wasn't fixed, the bug is still valid on r146200.

I cannot reproduce this on command-line in r146239.  Ossy, can you check if the attached test case passes on jsc command line for you in ARMv7?

Also, is ARMv7 on Qt using the ARMv7Assembler?

[Filip Pizlo]

Created attachment 193906 [details]
command-line test

[Csaba Osztrogonác]

(In reply to comment #8)
> Created an attachment (id=193906) [details]
> command-line test

I have ARM traditional build now and this test fails with DFG JIT, 
but passes with disabled DFG JIT:

$ ./jsc -f 1.js --useDFGJIT=false
<span><span class="pass">PASS</span> </span>
<br /><span class="pass">TEST COMPLETE</span>

$ ./jsc -f 1.js
<span><span class="fail">FAIL</span> SputnikError: #002.05833591723e-3126.3659873724e-314-002.397855243786e-312F </span>
<br /><span class="pass">TEST COMPLETE</span>


Let me check it on Thumb2 build too.

[Csaba Osztrogonác]

I got similar results with Thumb2 build: (on r146178)

$ ./jsc -f 1.js --useDFGJIT=false
<span><span class="pass">PASS</span> </span>
<br /><span class="pass">TEST COMPLETE</span>
buildbot@panda1:~/cute1/slaves/armReleaseTest/buildslave/arm-qt-linux-release-

$ ./jsc -f 1.js
<span><span class="fail">FAIL</span> SputnikError: #002.1219957905e-3142.1219957905e-314-002.1219957905e-314F </span>
<br /><span class="pass">TEST COMPLETE</span>

[Csaba Osztrogonác]

(In reply to comment #7)
> Also, is ARMv7 on Qt using the ARMv7Assembler?

Our GCC's default is ARM, which uses ARMAssembler, but we can 
override it with -mthumb cflag and then ARMv7Assembler is used.
I checked manually and this bug is valid with both of them.

[Filip Pizlo]

(In reply to comment #11)
> (In reply to comment #7)
> > Also, is ARMv7 on Qt using the ARMv7Assembler?
> 
> Our GCC's default is ARM, which uses ARMAssembler, but we can 
> override it with -mthumb cflag and then ARMv7Assembler is used.
> I checked manually and this bug is valid with both of them.

OK - this is super weird!

This test passes fine for me.

I think I found the issue though.  Look at this code in DFGSpeculativeJIT.h:

    JITCompiler::Call callOperation(C_DFGOperation_EJ operation, GPRReg result, GPRReg arg1Tag, GPRReg arg1Payload)
    {
        m_jit.setupArgumentsWithExecState(arg1Payload, arg1Tag);
        return appendCallWithExceptionCheckSetResult(operation, result);
    }

Can you try passing EABI_32BIT_DUMMY_ARG like so:

        m_jit.setupArgumentsWithExecState(EABI_32BIT_DUMMY_ARG arg1Payload, arg1Tag);

And seeing if it passes?  The DUMMY_ARG thing isn't necessary on our ABI but I think it is on yours.

Sorry for this breakage!  ABIs are hard! ;-)

[Csaba Osztrogonác]

(In reply to comment #12)
> Can you try passing EABI_32BIT_DUMMY_ARG like so:
> 
>         m_jit.setupArgumentsWithExecState(EABI_32BIT_DUMMY_ARG arg1Payload, arg1Tag);
> 
> And seeing if it passes?  The DUMMY_ARG thing isn't necessary on our ABI but I think it is on yours.
> 
> Sorry for this breakage!  ABIs are hard! ;-)

Yay, this command line test passes with this change. Many thanks for the fix.

[Filip Pizlo]

(In reply to comment #13)
> (In reply to comment #12)
> > Can you try passing EABI_32BIT_DUMMY_ARG like so:
> > 
> >         m_jit.setupArgumentsWithExecState(EABI_32BIT_DUMMY_ARG arg1Payload, arg1Tag);
> > 
> > And seeing if it passes?  The DUMMY_ARG thing isn't necessary on our ABI but I think it is on yours.
> > 
> > Sorry for this breakage!  ABIs are hard! ;-)
> 
> Yay, this command line test passes with this change. Many thanks for the fix.

Can you land?

[Csaba Osztrogonác]

Fix landed in http://trac.webkit.org/changeset/146309.

[Filip Pizlo]

(In reply to comment #15)
> Fix landed in http://trac.webkit.org/changeset/146309.

Thanks! :-)