Bug 123277

Summary: REGRESSION(r157164): v8-v6/v8-raytrace.js crashes on arm and sh4
Product: WebKit Reporter: Julien Brianceau <jbriance>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, kkristof, mark.lam, msaboff, oliver, ossy, rgabor, szledan, yannick.poirier, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 108645    
Attachments:
Description Flags
run-layout-jsc results for arm r157163
none
run-layout-jsc results for arm r157164
none
Use regTx instead of nonArgGPRx in virtualForThunkGenerator function fpizlo: review-, fpizlo: commit-queue-

Julien Brianceau
Reported 2013-10-24 10:07:37 PDT
Since r157164 (http://trac.webkit.org/changeset/157164), the v8-v6/v8-raytrace.js test crashes in JIT code on CPU(ARM_TRADITIONAL) and CPU(SH4) architectures. The crashes occurs in code generated by virtualThunkGenerator function in jit/ThunkGenerators.cpp: CCallHelpers::TrustedImm32(JSValue::CellTag))); #endif jit.loadPtr(CCallHelpers::Address(GPRInfo::nonArgGPR0, JSCell::structureOffset()), GPRInfo::nonArgGPR2); slowCase.append( jit.branchPtr( CCallHelpers::NotEqual, CCallHelpers::Address(GPRInfo::nonArgGPR2, Structure::classInfoOffset()), CCallHelpers::TrustedImmPtr(JSFunction::info()))); The jit.loadPtr(CCallHelpers::Address(GPRInfo::nonArgGPR0, JSCell::structureOffset()), GPRInfo::nonArgGPR2) is fine, but loaded value in GPRInfo::nonArgGPR2 is null. So the next instruction "CCallHelpers::Address(GPRInfo::nonArgGPR2, Structure::classInfoOffset())" dereferences this null pointer, causing the crash. ARM generated code: Program received signal SIGSEGV, Segmentation fault. 0x3444c6cc in ?? () (gdb) disassemble $pc-16, $pc+16 Dump of assembler code from 0x3444c6bc to 0x3444c6dc: 0x3444c6bc: andeq r0, r0, r0 0x3444c6c0: cmn r8, #5 0x3444c6c4: bne 0x3444c708 0x3444c6c8: ldr r9, [r4] // this line is jit.loadPtr(CCallHelpers::Address(GPRInfo::nonArgGPR0, JSCell::structureOffset()), GPRInfo::nonArgGPR2) => 0x3444c6cc: ldr r12, [r9, #32] 0x3444c6d0: movw r3, #53092 ; 0xcf64 0x3444c6d4: movt r3, #69 ; 0x45 0x3444c6d8: cmp r12, r3 End of assembler dump. (gdb) info registers r0 0x344fef98 877653912 r1 0xfffffffb 4294967291 r2 0x0 0 r3 0x349e3ac0 882784960 r4 0x349e3ab8 882784952 r5 0x349e3ab8 882784952 r6 0x200 512 r7 0x3526296c 891693420 r8 0xfffffffb 4294967291 r9 0x0 0 r10 0x352ff1d4 892334548 r11 0x3524e100 891609344 r12 0x3444c6c0 876922560 sp 0x3efff3d8 0x3efff3d8 lr 0x3444cd24 0x3444cd24 pc 0x3444c6cc 0x3444c6cc cpsr 0x60000010 1610612752 SH4: (gdb) disassemble $pc-8, $pc+8 Dump of assembler code from 0x2c20d7c6 to 0x2c20d7d6: 0x2c20d7c6: mov.l 0x2c20d818,r13 ! 0x2e 0x2c20d7c8: braf r13 0x2c20d7ca: nop 0x2c20d7cc: mov.l @r10,r9 // this line is jit.loadPtr(CCallHelpers::Address(GPRInfo::nonArgGPR0, JSCell::structureOffset()), GPRInfo::nonArgGPR2) => 0x2c20d7ce: mov.l @(32,r9),r3 0x2c20d7d0: mov.l 0x2c20d81c,r11 ! 0x839594 <_ZN3JSC10JSFunction6s_infoE> 0x2c20d7d2: cmp/eq r11,r3 0x2c20d7d4: bt 0x2c20d7dc End of assembler dump. (gdb) regs PC 2c20d7ce SR 00008001 PR 2c20de3c MACH 00000000 GBR 2aafc4a0 VBR 00000000 DBR 00000000 MACL 00000000 SSR 00000000 SPC 00000000 SGR 00000000 FPUL 00000000 FPSCR 00080004 R0-R7 2c15e918 fffffffb 00000000 00000000 2b31500c 2b3aae00 2c15e918 00000007 R8-R15 fffffffb e6e03aee 2c072b90 2c08f6f0 00840b28 000002da 2c072b90 7bc5bf88 R0b-R7b 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 DR0-DR6 0000000000000000 c0329ffcf21e3574 c0329ffcf21e3574 4035000000000000 DR8-DR14 ebfb6332890233b0 0d23dd77e34c0009 41f0000000000000 0000000000000000 XD0-XD6 0000000000000000 0000000000000000 0000000000000000 0000000000000000 XD8-XD14 0000000000000000 0000000000000000 0000000000000000 0000000000000000
Attachments
run-layout-jsc results for arm r157163 (830.67 KB, application/x-compressed-tar)
2013-10-27 03:26 PDT, Julien Brianceau 		no flags
Details
run-layout-jsc results for arm r157164 (331.01 KB, application/x-compressed-tar)
2013-10-27 03:29 PDT, Julien Brianceau 		no flags
Details
Use regTx instead of nonArgGPRx in virtualForThunkGenerator function (3.90 KB, patch)
2013-10-29 16:44 PDT, Julien Brianceau 		fpizlo: review-
fpizlo: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Julien Brianceau
Comment 1 2013-10-24 10:18:24 PDT
Another information about this: - when DFG is disabled using JSC_useDFGJIT=false, the crash is still there (for both architectures) - when JIT is disabled using JSC_useJIT=false, the test runs fine without crash (for both architectures)
Julien Brianceau
Comment 2 2013-10-25 02:15:19 PDT
Here is the backtrace leading to this code generation on my ARM platform: #0 0x000969f4 in JSC::virtualForThunkGenerator (vm=0xa18710, kind=JSC::CodeForCall) at /webkit/Source/JavaScriptCore/jit/ThunkGenerators.cpp:198 #1 0x00096dd8 in JSC::virtualCallThunkGenerator (vm=0xa18710) at /webkit/Source/JavaScriptCore/jit/ThunkGenerators.cpp:267 #2 0x000902dc in JSC::JITThunks::ctiStub (this=0xa23be0, vm=0xa18710, generator=0x96db0 <JSC::virtualCallThunkGenerator(JSC::VM*)>) at /webkit/Source/JavaScriptCore/jit/JITThunks.cpp:71 #3 0x0007dbc4 in JSC::VM::getCTIStub (this=0xa18710, generator=0x96db0 <JSC::virtualCallThunkGenerator(JSC::VM*)>) at /webkit/Source/JavaScriptCore/runtime/VM.h:326 #4 0x0035a734 in JSC::linkSlowFor (repatchBuffer=..., vm=0xa18710, callLinkInfo=..., kind=JSC::CodeForCall) at /webkit/Source/JavaScriptCore/jit/Repatch.cpp:1219 #5 0x0035ab34 in JSC::linkSlowFor (exec=0x3531bd48, callLinkInfo=..., kind=JSC::CodeForCall) at /webkit/Source/JavaScriptCore/jit/Repatch.cpp:1263 #6 0x00345f00 in JSC::operationLinkClosureCall (execCallee=0x3531bd48) at /webkit/Source/JavaScriptCore/jit/JITOperations.cpp:658 #7 0x34d655d4 in ?? () #8 0x34d655d4 in ?? ()
Julien Brianceau
Comment 3 2013-10-25 03:28:45 PDT
First analyzis: it seems that there's a mixup betweeen JSInterfaceJIT::regT0 and GPRInfo::nonArgGPR0. The issue is not seen on X86 and X86_64, because on these architectures we have regT0 == nonArgGPR0 == X86Registers::eax
Julien Brianceau
Comment 4 2013-10-25 09:40:24 PDT
(In reply to comment #3) > The issue is not seen on X86 and X86_64, because on these architectures we have regT0 == nonArgGPR0 == X86Registers::eax I mean *this would explain why* the issue is not seen on X86 and X86_64.
Julien Brianceau
Comment 5 2013-10-27 03:26:52 PDT
Created attachment 215267 [details] run-layout-jsc results for arm r157163
Julien Brianceau
Comment 6 2013-10-27 03:29:26 PDT
Created attachment 215268 [details] run-layout-jsc results for arm r157164 Here are the run-layout-jsc results for r157163 and r157164. The delta is 20 new crashes between r157163 and r157164 for ARM_TRADITIONAL: js/array-proto-func-property-getter-except js/comparison-operators-greater js/comparison-operators js/comparison-operators-less js/date-set-to-nan js/dfg-float32array js/dfg-float64array js/dfg-inline-unused-this js/dfg-inline-unused-this-method-check js/dfg-int16array js/dfg-int32array js/dfg-int32array-overflow-values js/dfg-int8array js/dfg-intrinsic-unused-this js/dfg-intrinsic-unused-this-method-check js/dfg-uint16array js/dfg-uint32array js/dfg-uint32array-overflow-values js/dfg-uint8array js/dfg-uint8clampedarray
Julien Brianceau
Comment 7 2013-10-29 16:44:30 PDT
Created attachment 215444 [details] Use regTx instead of nonArgGPRx in virtualForThunkGenerator function This patch fixes many crashes on arm and sh4 architectures.
Filip Pizlo
Comment 8 2013-10-29 16:47:53 PDT
Comment on attachment 215444 [details] Use regTx instead of nonArgGPRx in virtualForThunkGenerator function I think this is wrong. The point of using nonArgGPR is that we want to use a register that isn't an argument. regT0 may be an argument register on some platforms. I think that the correct change is to make sure that when the JITs make calls, they use nonArgGPR for the callee.
Filip Pizlo
Comment 9 2013-10-29 16:49:39 PDT
(In reply to comment #6) > Created an attachment (id=215268) [details] > run-layout-jsc results for arm r157164 > > Here are the run-layout-jsc results for r157163 and r157164. > > The delta is 20 new crashes between r157163 and r157164 for ARM_TRADITIONAL: > js/array-proto-func-property-getter-except > js/comparison-operators-greater > js/comparison-operators > js/comparison-operators-less > js/date-set-to-nan > js/dfg-float32array > js/dfg-float64array > js/dfg-inline-unused-this > js/dfg-inline-unused-this-method-check > js/dfg-int16array > js/dfg-int32array > js/dfg-int32array-overflow-values > js/dfg-int8array > js/dfg-intrinsic-unused-this > js/dfg-intrinsic-unused-this-method-check > js/dfg-uint16array > js/dfg-uint32array > js/dfg-uint32array-overflow-values > js/dfg-uint8array > js/dfg-uint8clampedarray I'm curious, is your methodology for making these changes seriously just that you keep trying stuff until tests pass?
Filip Pizlo
Comment 10 2013-10-29 16:53:04 PDT
(In reply to comment #9) > (In reply to comment #6) > > Created an attachment (id=215268) [details] [details] > > run-layout-jsc results for arm r157164 > > > > Here are the run-layout-jsc results for r157163 and r157164. > > > > The delta is 20 new crashes between r157163 and r157164 for ARM_TRADITIONAL: > > js/array-proto-func-property-getter-except > > js/comparison-operators-greater > > js/comparison-operators > > js/comparison-operators-less > > js/date-set-to-nan > > js/dfg-float32array > > js/dfg-float64array > > js/dfg-inline-unused-this > > js/dfg-inline-unused-this-method-check > > js/dfg-int16array > > js/dfg-int32array > > js/dfg-int32array-overflow-values > > js/dfg-int8array > > js/dfg-intrinsic-unused-this > > js/dfg-intrinsic-unused-this-method-check > > js/dfg-uint16array > > js/dfg-uint32array > > js/dfg-uint32array-overflow-values > > js/dfg-uint8array > > js/dfg-uint8clampedarray > > I'm curious, is your methodology for making these changes seriously just that you keep trying stuff until tests pass? Reason why I ask is that quite clearly, the DFG is using nonArgGPR0 for the callee. I just found that out by looking for "Call" in the DFGSpeculativeJIT64.cpp and DFGSpeculativeJIT32_64.cpp files. That led me to emitCall(), where it's clear that we're moving the callee into nonArgGPR0 and not regT0. Hence this code will break the DFG.
Julien Brianceau
Comment 11 2013-10-29 17:15:32 PDT
(In reply to comment #10) > (In reply to comment #9) > > I'm curious, is your methodology for making these changes seriously just that you keep trying stuff until tests pass? No, I thought that my previous comments showed that I did a little analyzis before. > Reason why I ask is that quite clearly, the DFG is using nonArgGPR0 for the callee. I just found that out by looking for "Call" in the DFGSpeculativeJIT64.cpp and DFGSpeculativeJIT32_64.cpp files. That led me to emitCall(), where it's clear that we're moving the callee into nonArgGPR0 and not regT0. Digging into the JavaScriptCore engine is just a part of my job, and I have still to learn on how it works. I don't pretend to be an expert of this engine, I'm just trying to help. > Hence this code will break the DFG. I didn't realize this, but fortunately reviews are made for this, right?
Julien Brianceau
Comment 12 2013-10-30 15:38:05 PDT
This regression has been fixed with Filip's changeset r158315 (http://trac.webkit.org/changeset/158315) Thanks a lot!
Note You need to log in before you can comment on or make changes to this bug.