[DFG] Cleaning up and unifying 32bit code more
Created attachment 330116 [details] Patch
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.
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.
Created attachment 330133 [details] Patch
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.
Committed r226269: <https://trac.webkit.org/changeset/226269>
<rdar://problem/36197076>
The MIPS buildbot now has 4k+ test failures after this commit, I suspect it might be related.
(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?
The latest run shows 61 failures, but these still include segfaults. I just triggered a clean build on the buildbot.
Also, it looks like we have the same ~60 new segfaults/failures on the linux 32-bit arm bots.
(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.
(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.
(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.
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
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.
Created attachment 330424 [details] Disassembly of GetDirectPname from gdb. Attaching the disassembly of GetDirectPname as provided by gdb (x/61i 0x3040f080).
(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?
Oops. I looked too fast and thought Žan had reversed the commit, not fixed it... sorry about the noise...
(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