Bug 181124 – [DFG] Cleaning up and unifying 32bit code more

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[DFG] Cleaning up and unifying 32bit code more

[Yusuke Suzuki]

Created attachment 330116 [details]
Patch

[Mark Lam]

Please fix the 32-bit build:

/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/i386/UnifiedSource44.o
In file included from /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource44.cpp:5:
In file included from ./dfg/DFGJITCompiler.cpp:33:
In file included from ./dfg/DFGInlineCacheWrapperInlines.h:31:
In file included from ./dfg/DFGSlowPathGenerator.h:31:
./dfg/DFGSpeculativeJIT.h:2301:23: error: class member cannot be redeclared
    JITCompiler::Call callOperation(J_JITOperation_EGReoJ operation, JSValueRegs result, GPRReg arg1, GPRReg arg2, JSValueRegs arg3)
                      ^
./dfg/DFGSpeculativeJIT.h:1350:23: note: previous declaration is here
    JITCompiler::Call callOperation(J_JITOperation_EGReoJ operation, JSValueRegs result, GPRReg arg1, GPRReg arg2, JSValueRegs arg3)
                      ^
./dfg/DFGSpeculativeJIT.h:2306:23: error: class member cannot be redeclared
    JITCompiler::Call callOperation(J_JITOperation_EGReoJss operation, JSValueRegs result, GPRReg arg1, GPRReg arg2, GPRReg arg3)
                      ^
./dfg/DFGSpeculativeJIT.h:1356:23: note: previous declaration is here
    JITCompiler::Call callOperation(J_JITOperation_EGReoJss operation, JSValueRegs result, GPRReg arg1, GPRReg arg2, GPRReg arg3)
                      ^
2 errors generated.

[Mark Lam]

Comment on attachment 330116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330116&action=review

r=me with 32-bit build fix.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1359
> -    JITCompiler::Call callOperation(J_JITOperation_EGReoJ operation, GPRReg result, GPRReg arg1, GPRReg arg2, GPRReg arg3)
> +    JITCompiler::Call callOperation(J_JITOperation_EGReoJ operation, JSValueRegs result, GPRReg arg1, GPRReg arg2, JSValueRegs arg3)
>      {
> -        m_jit.setupArgumentsWithExecState(arg1, arg2, arg3);
> -        return appendCallSetResult(operation, result);
> +        m_jit.setupArgumentsWithExecState(arg1, arg2, arg3.payloadGPR());
> +        return appendCallSetResult(operation, result.payloadGPR());
>      }
>  
> -    JITCompiler::Call callOperation(J_JITOperation_EGReoJss operation, GPRReg result, GPRReg arg1, GPRReg arg2, GPRReg arg3)
> +    JITCompiler::Call callOperation(J_JITOperation_EGReoJss operation, JSValueRegs result, GPRReg arg1, GPRReg arg2, GPRReg arg3)
>      {
>          m_jit.setupArgumentsWithExecState(arg1, arg2, arg3);
> -        return appendCallSetResult(operation, result);
> +        return appendCallSetResult(operation, result.payloadGPR());

Looks like these should be USE(JSVALUE64) only.

[Yusuke Suzuki]

Created attachment 330133 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 330116 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=330116&action=review

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:1359
>> +        return appendCallSetResult(operation, result.payloadGPR());
> 
> Looks like these should be USE(JSVALUE64) only.

Yeah, thanks. Fixed.

[Yusuke Suzuki]

Committed r226269: <https://trac.webkit.org/changeset/226269>

[Radar WebKit Bug Importer]

<rdar://problem/36197076>

[Guillaume Emont]

The MIPS buildbot now has 4k+ test failures after this commit, I suspect it might be related.

[Yusuke Suzuki]

(In reply to Guillaume Emont from comment #8)
> The MIPS buildbot now has 4k+ test failures after this commit, I suspect it
> might be related.

It seems that x86 32bit does not show any regressions.
And I think the regression is largely removed. Could you perform clean build?

[Guillaume Emont]

The latest run shows 61 failures, but these still include segfaults. I just triggered a clean build on the buildbot.

[Guillaume Emont]

Also, it looks like we have the same ~60 new segfaults/failures on the linux 32-bit arm bots.

[Mark Lam]

(In reply to Guillaume Emont from comment #11)
> Also, it looks like we have the same ~60 new segfaults/failures on the linux
> 32-bit arm bots.

Look at the definitions of the various forms of callOperation.  Yusuke changed the use of some of these.  If I remember correctly, the MIPs ABI is one that does not pack its arguments.  Instead, it requires some argument registers to be left unused as spacers depending on the types of the arguments being passed.  You should check if Yusuke's change broke this somehow.

[Yusuke Suzuki]

(In reply to Guillaume Emont from comment #11)
> Also, it looks like we have the same ~60 new segfaults/failures on the linux
> 32-bit arm bots.

Guillaume, could you take a look this? I don't have ARM machines, and this is not reproducible in x86 32bit.

[Guillaume Emont]

(In reply to Yusuke Suzuki from comment #13)
> (In reply to Guillaume Emont from comment #11)
> > Also, it looks like we have the same ~60 new segfaults/failures on the linux
> > 32-bit arm bots.
> 
> Guillaume, could you take a look this? I don't have ARM machines, and this
> is not reproducible in x86 32bit.

I will look at it next week. Sorry I won't have time before then.

[Guillaume Emont]

Here's what I know so far about the segfault on mips, when testing with sunspider-1.0/string-fasta.js. 

On the device console:
  [14978.011855] do_page_fault() #2: sending SIGSEGV to jsc for invalid read access from
  [14978.011869] 312eeec0 (epc == 3040f124, ra == 3040b714)

In gdb:
Program received signal SIGSEGV, Segmentation fault.
0x3040f124 in ?? ()
(gdb) x/10i $pc-24
   0x3040f10c:  move    v1,t3
   0x3040f110:  sll     t7,v1,0x3
   0x3040f114:  addu    t7,t7,t2
   0x3040f118:  lw      v1,20(t7)
   0x3040f11c:  sll     t7,v1,0x3
   0x3040f120:  addu    t7,t7,t2
=> 0x3040f124:  lw      v0,16(t7)
   0x3040f128:  nop
   0x3040f12c:  nop
   0x3040f130:  b       0x3040f174
   0x3040f134:  nop

On this occurrence, I ran with --dumpDisassembly=true, and I will attach the output. It looks like we are in this part:

     317:<!3:loc14>	GetDirectPname(Cell:@314, KnownCell:@311, KnownInt32:@315, KnownCell:@316, JS|MustGen|VarArgs|UseAsOther, NonIntAsdouble, R:World, W:Heap, Exits, ClobbersExit, bc#264, ExitValid)  predicting NonIntAsdouble
        disassembly not available for range 0x3040f080...0x3040f174

[Guillaume Emont]

Created attachment 330423 [details]
Output of jsc --useDisassembly=true string-fasta.js on the run mentioned in comment https://bugs.webkit.org/show_bug.cgi?id=181124#c15

Here is the output of --dumpDisassembly=true, does not include actual disassembly as we don't have that on mips so far.

[Guillaume Emont]

Created attachment 330424 [details]
Disassembly of GetDirectPname from gdb.

Attaching the disassembly of GetDirectPname as provided by gdb (x/61i 0x3040f080).

[Yusuke Suzuki]

(In reply to Guillaume Emont from comment #17)
> Created attachment 330424 [details]
> Disassembly of GetDirectPname from gdb.
> 
> Attaching the disassembly of GetDirectPname as provided by gdb (x/61i
> 0x3040f080).

Is it still a thing after Zan’s patches are landed?

[Guillaume Emont]

Oops. I looked too fast and thought Žan had reversed the commit, not fixed it... sorry about the noise...

[Yusuke Suzuki]

(In reply to Guillaume Emont from comment #19)
> Oops. I looked too fast and thought Žan had reversed the commit, not fixed
> it... sorry about the noise...

No problem! It's great to hear that our 32bit regressions are fixed :D