Bug 98857 – [Qt][ARM] REGRESSION(r130826): It made 33 JSC test and 466 layout tests crash

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

https://trac.webkit.org/changeset/130826 made 33 JSC test 
and 466 layout tests crash on the Qt ARM bot.

(last good revision: r130824, ... build fail ..., first bad revision: r130828)
results: http://build.webkit.sed.hu/builders/ARMv7%20Linux%20Qt5%20Release%20%28Test%29/builds/7185

[Csaba Osztrogonác]

ping?

[Filip Pizlo]

(In reply to comment #1)
> ping?

You guys are probably doing something strange with branch patching.

[Csaba Osztrogonác]

Now there are 800 crashing tests on the ARM bot.

Zoltán, Gábor, are you going to fix this bug in the near future? 
Don't you plan maintaining the ARM traditional assembler?

[Csaba Osztrogonác]

ping?

[Gabor Ballabas]

(In reply to comment #4)
> ping?

I'm working on it. It seems that something goes wrong with the get_by_val operation and the ARMAssembler::getLdrImmAddress function waits a pc-relative ldr instruction but gets something else instead.

[Csaba Osztrogonác]

Adding it as Qt 5.0.0 release blocker bug. It wouldn't 
be good to release Qt 5 with this serious bug. 

Or we should disable DFG JIT on ARM as a workaround. Gábor,
so you think if the bug would disappear with disabling DFG JIT?

[Gabor Ballabas]

(In reply to comment #6)

> Or we should disable DFG JIT on ARM as a workaround. Gábor,
> so you think if the bug would disappear with disabling DFG JIT?

Unfortunately disabling the DFG JIT wouldn't solve this problem.

I have some debugging information about the crash maybe Filip or someone else with more expertise could figure out something from it:


(gdb) info breakpoints
Num     Type           Disp Enb Address    What
1       breakpoint     keep y   0x002cffd4 in JSC::JIT::privateCompileGetByVal(JSC::ByValInfo*, JSC::ReturnAddressPtr, JSC::JITArrayMode) 
                                           at /home/bgabor/WebKit/Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1468
2       breakpoint     keep n   0x0008f0c0 in JSC::ARMAssembler::getLdrImmAddress(unsigned int*) at /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h:780
(gdb) r
Starting program: /home/bgabor/jsc/test-crash/jsc -s -f ecma_3/shell.js -f ecma_3/Object/shell.js -f ecma_3/Object/class-001.js
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/libthread_db.so.1".
[New Thread 0x42850450 (LWP 4914)]

Breakpoint 1, JSC::JIT::privateCompileGetByVal (this=0xbeffdae0, byValInfo=0x82b6c8, returnAddress=..., arrayMode=JSC::JITArrayStorage)
    at /home/bgabor/WebKit/Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1468
1468        repatchBuffer.relink(byValInfo->badTypeJump, CodeLocationLabel(byValInfo->stubRoutine->code().code()));
(gdb) p byValInfo->badTypeJump
$1 = {<JSC::CodeLocationCommon> = {<JSC::MacroAssemblerCodePtr> = {m_value = 0x40022c10}, <No data fields>}, <No data fields>}
(gdb) x/i 0x40022c10
   0x40022c10:  ldr     r4, [r0, #3071384]
(gdb) enable 2
(gdb) c
Continuing.

Breakpoint 2, JSC::ARMAssembler::getLdrImmAddress (insn=0x40022c0c) at /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h:783
783                 if ((*insn & LdrPcImmediateInstructionMask) != LdrPcImmediateInstruction) {
(gdb) x/i 0x40022c0c
   0x40022c0c:  bne     0x40022e60
(gdb) x/i (0x40022c0c + 0x4)
   0x40022c10:  ldr     r4, [r0, #3071384]
(gdb) c
Continuing.
ASSERTION FAILED: (*insn & BlxInstructionMask) == BlxInstruction
/home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h(785) : static JSC::ARMWord* JSC::ARMAssembler::getLdrImmAddress(JSC::ARMWord*)
1   0x8f134 /home/bgabor/jsc/test-crash/jsc() [0x8f134]
2   0x93b08 /home/bgabor/jsc/test-crash/jsc() [0x93b08]
3   0x20c6bc /home/bgabor/jsc/test-crash/jsc() [0x20c6bc]
4   0x20ce7c /home/bgabor/jsc/test-crash/jsc() [0x20ce7c]
5   0x20cd84 /home/bgabor/jsc/test-crash/jsc() [0x20cd84]
6   0x2d0028 /home/bgabor/jsc/test-crash/jsc() [0x2d0028]
7   0x2edb88 /home/bgabor/jsc/test-crash/jsc() [0x2edb88]
8   0x2e3da0 /home/bgabor/jsc/test-crash/jsc() [0x2e3da0]
9   0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
10  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
11  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
12  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
13  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
14  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
15  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
16  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
17  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
18  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
19  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
20  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
21  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
22  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
23  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
24  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
25  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
26  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
27  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
28  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
29  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
30  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
31  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]

Program received signal SIGSEGV, Segmentation fault.
0x0008f144 in JSC::ARMAssembler::getLdrImmAddress (insn=0x40022c0c) at /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h:785
785                     ASSERT((*insn & BlxInstructionMask) == BlxInstruction);
(gdb)

[Gabor Ballabas]

Created attachment 170587 [details]
Hack for disabling one of the array-related optimizations.

And if I disable one of the array-related optimizations as in the attached patch a lot of crashes go away.
It is also worth mentioning that the codeblock within the if 0 - endif seems to be completely ignored
in Linux-x86.

[Filip Pizlo]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > Or we should disable DFG JIT on ARM as a workaround. Gábor,
> > so you think if the bug would disappear with disabling DFG JIT?
> 
> Unfortunately disabling the DFG JIT wouldn't solve this problem.
> 
> I have some debugging information about the crash maybe Filip or someone else with more expertise could figure out something from it:
> 
> 
> (gdb) info breakpoints
> Num     Type           Disp Enb Address    What
> 1       breakpoint     keep y   0x002cffd4 in JSC::JIT::privateCompileGetByVal(JSC::ByValInfo*, JSC::ReturnAddressPtr, JSC::JITArrayMode) 
>                                            at /home/bgabor/WebKit/Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1468
> 2       breakpoint     keep n   0x0008f0c0 in JSC::ARMAssembler::getLdrImmAddress(unsigned int*) at /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h:780
> (gdb) r
> Starting program: /home/bgabor/jsc/test-crash/jsc -s -f ecma_3/shell.js -f ecma_3/Object/shell.js -f ecma_3/Object/class-001.js
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/libthread_db.so.1".
> [New Thread 0x42850450 (LWP 4914)]
> 
> Breakpoint 1, JSC::JIT::privateCompileGetByVal (this=0xbeffdae0, byValInfo=0x82b6c8, returnAddress=..., arrayMode=JSC::JITArrayStorage)
>     at /home/bgabor/WebKit/Source/JavaScriptCore/jit/JITPropertyAccess.cpp:1468
> 1468        repatchBuffer.relink(byValInfo->badTypeJump, CodeLocationLabel(byValInfo->stubRoutine->code().code()));
> (gdb) p byValInfo->badTypeJump
> $1 = {<JSC::CodeLocationCommon> = {<JSC::MacroAssemblerCodePtr> = {m_value = 0x40022c10}, <No data fields>}, <No data fields>}
> (gdb) x/i 0x40022c10
>    0x40022c10:  ldr     r4, [r0, #3071384]

This looks suspicious.  Why are we loading from r0 at such a *HUGE* offset?

> (gdb) enable 2
> (gdb) c
> Continuing.
> 
> Breakpoint 2, JSC::ARMAssembler::getLdrImmAddress (insn=0x40022c0c) at /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h:783
> 783                 if ((*insn & LdrPcImmediateInstructionMask) != LdrPcImmediateInstruction) {
> (gdb) x/i 0x40022c0c
>    0x40022c0c:  bne     0x40022e60

So, you're emitting a bne for patchableBranch32, and then you're trying to patch a blx when relink() is called.

That's your bug.

Either make relink() work with bne, or make patchableBranch32 emit a blx.

> (gdb) x/i (0x40022c0c + 0x4)
>    0x40022c10:  ldr     r4, [r0, #3071384]
> (gdb) c
> Continuing.
> ASSERTION FAILED: (*insn & BlxInstructionMask) == BlxInstruction
> /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h(785) : static JSC::ARMWord* JSC::ARMAssembler::getLdrImmAddress(JSC::ARMWord*)
> 1   0x8f134 /home/bgabor/jsc/test-crash/jsc() [0x8f134]
> 2   0x93b08 /home/bgabor/jsc/test-crash/jsc() [0x93b08]
> 3   0x20c6bc /home/bgabor/jsc/test-crash/jsc() [0x20c6bc]
> 4   0x20ce7c /home/bgabor/jsc/test-crash/jsc() [0x20ce7c]
> 5   0x20cd84 /home/bgabor/jsc/test-crash/jsc() [0x20cd84]
> 6   0x2d0028 /home/bgabor/jsc/test-crash/jsc() [0x2d0028]
> 7   0x2edb88 /home/bgabor/jsc/test-crash/jsc() [0x2edb88]
> 8   0x2e3da0 /home/bgabor/jsc/test-crash/jsc() [0x2e3da0]
> 9   0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 10  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 11  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 12  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 13  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 14  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 15  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 16  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 17  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 18  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 19  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 20  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 21  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 22  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 23  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 24  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 25  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 26  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 27  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 28  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 29  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 30  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 31  0x2e3b48 /home/bgabor/jsc/test-crash/jsc() [0x2e3b48]
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x0008f144 in JSC::ARMAssembler::getLdrImmAddress (insn=0x40022c0c) at /home/bgabor/WebKit/Source/JavaScriptCore/assembler/ARMAssembler.h:785
> 785                     ASSERT((*insn & BlxInstructionMask) == BlxInstruction);
> (gdb)

[Gabor Ballabas]

(In reply to comment #9)

> So, you're emitting a bne for patchableBranch32, and then you're trying to patch a blx when relink() is called.
> 
> That's your bug.
> 
> Either make relink() work with bne, or make patchableBranch32 emit a blx.
> 

Thank you for your time and your advice. I will follow your suggestion and take a closer look at those areas you have mentioned.

[Csaba Osztrogonác]

any progress?

[Csaba Osztrogonác]

ping?

[Gabor Ballabas]

Created attachment 173021 [details]
Workaround patch

This is just a workaround to make the ARM tester buildbot happy.
Meanwhile I'm going to work on the proper fix.

[Csaba Osztrogonác]

Comment on attachment 173021 [details]
Workaround patch

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

I think we should push this workaround as soon as possible to make ARM_TRADITIONAL tests happy.
And I hope Zoltán and Gábor will manage to fix the bug properly on the following weeks.

Zoltán? Filip?

> Source/JavaScriptCore/jit/JITInlineMethods.h:182
> +/* FIXME: Check if we can do better in ARM. For now we just want to avoid the

s/in ARM/on ARM

> Source/JavaScriptCore/jit/JITInlineMethods.h:183
> + * following javascriptcore test crashes:

s/crashes/crash

[Zoltan Herczeg]

I will check this next week.

[Gabor Ballabas]

Created attachment 173263 [details]
Rebased fix

[Csaba Osztrogonác]

(In reply to comment #16)
> Created an attachment (id=173263) [details]
> Rebased fix

Is this workaround still valid? What about the proper fix?
The ARM traditional assembler became useless ~1.5 months before.
Is there any chance to be fixed in the near future?

[Filip Pizlo]

Comment on attachment 173263 [details]
Rebased fix

I don't particularly like this fix.  First, it's masking the problem that relink() is assuming that something is a blx when it's actually a bne.  Second, if you really want to disable patching of get_by_val/put_by_val then you should do it without preprocessor macros in the JIT's code.  That's just ugly and error-prone.

I strongly recommend that you do the *really simple* fix of making relink() work with bne, or making patchableBranch32() emit a blx.

But I will defer to Zoltan's opinion on this.  Zoltan, do you think that what I'm suggesting is sensible?  Or do you feel strongly that we should land this workaround as-is?

[Gabor Ballabas]

Created attachment 175969 [details]
Proper fix

[Zoltan Herczeg]

Comment on attachment 175969 [details]
Proper fix

Thanks for the fix.

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM.h:816
> +        ARMWord tmp = (static_cast<unsigned>(imm.m_value) == 0x80000000) ? ARMAssembler::InvalidImmediate : m_assembler.getOp2(-imm.m_value);
> +        if (tmp != ARMAssembler::InvalidImmediate)
> +            m_assembler.cmn(reg, tmp);
> +        else
> +            m_assembler.cmp(reg, m_assembler.getImm(imm.m_value, ARMRegisters::S0));

Please move this code duplication to a common subfunction.

[Gabor Ballabas]

Created attachment 175985 [details]
Refactoring code duplication.

[WebKit Review Bot]

Attachment 175985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/assembler/MacroAssemblerARM.h:840:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gabor Ballabas]

Created attachment 175987 [details]
Style fix

[Gabor Ballabas]

Created attachment 175993 [details]
Refactoring compare32 to private.

[Gabor Ballabas]

Created attachment 175994 [details]
Style fix

[Zoltan Herczeg]

Comment on attachment 175994 [details]
Style fix

Thanks, r=me

[Csaba Osztrogonác]

Comment on attachment 175994 [details]
Style fix

Clearing flags on attachment: 175994

Committed r135717: <http://trac.webkit.org/changeset/135717>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

*** Bug 101832 has been marked as a duplicate of this bug. ***