Bug 112676

Summary: REGRESSION(r146089): It broke 20 sputnik tests on ARM traditional and Thumb2
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, ossy, rgabor, zherczeg
Priority: P1 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 108645, 112376    
Attachments:
Description Flags
command-line test none

Description Csaba Osztrogonác 2013-03-19 03:02:33 PDT
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.
Comment 1 Csaba Osztrogonác 2013-03-19 03:48:36 PDT
The bug is already valid on r146100.
Comment 2 Csaba Osztrogonác 2013-03-19 05:00:49 PDT
I got it, http://trac.webkit.org/changeset/146089 is the 
culprit or it simple unhid an ARM traditional DFG JIT bug.
Comment 4 Csaba Osztrogonác 2013-03-19 06:29:38 PDT
Hmmm, maybe it was already fixed by http://trac.webkit.org/changeset/146179. Let me check it.
Comment 5 Csaba Osztrogonác 2013-03-19 06:53:38 PDT
(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.
Comment 6 Geoffrey Garen 2013-03-19 09:50:47 PDT
<rdar://problem/13452502>
Comment 7 Filip Pizlo 2013-03-19 13:17:00 PDT
(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?
Comment 8 Filip Pizlo 2013-03-19 13:17:20 PDT
Created attachment 193906 [details]
command-line test
Comment 9 Csaba Osztrogonác 2013-03-19 15:54:45 PDT
(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.
Comment 10 Csaba Osztrogonác 2013-03-19 16:04:41 PDT
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>
Comment 11 Csaba Osztrogonác 2013-03-19 16:08:44 PDT
(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.
Comment 12 Filip Pizlo 2013-03-19 16:19:39 PDT
(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! ;-)
Comment 13 Csaba Osztrogonác 2013-03-19 16:46:39 PDT
(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.
Comment 14 Filip Pizlo 2013-03-19 17:49:08 PDT
(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?
Comment 15 Csaba Osztrogonác 2013-03-20 00:45:58 PDT
Fix landed in http://trac.webkit.org/changeset/146309.
Comment 16 Filip Pizlo 2013-03-20 00:57:32 PDT
(In reply to comment #15)
> Fix landed in http://trac.webkit.org/changeset/146309.

Thanks! :-)