Bug 123277 – REGRESSION(r157164): v8-v6/v8-raytrace.js crashes on arm and sh4

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Julien Brianceau]

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

[Julien Brianceau]

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]

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]

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]

(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]

Created attachment 215267 [details]
run-layout-jsc results for arm r157163

[Julien Brianceau]

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]

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 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]

(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]

(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]

(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]

This regression has been fixed with Filip's changeset r158315 (http://trac.webkit.org/changeset/158315)

Thanks a lot!