Bug 98857 - [Qt][ARM] REGRESSION(r130826): It made 33 JSC test and 466 layout tests crash
Summary: [Qt][ARM] REGRESSION(r130826): It made 33 JSC test and 466 layout tests crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 420+
Hardware: All All
: P1 Critical
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
: 101832 (view as bug list)
Depends on:
Blocks: 76773 79668 97288
  Show dependency treegraph
 
Reported: 2012-10-09 22:15 PDT by Csaba Osztrogonác
Modified: 2012-11-27 01:57 PST (History)
10 users (show)

See Also:


Attachments
Hack for disabling one of the array-related optimizations. (892 bytes, patch)
2012-10-25 01:39 PDT, Gabor Ballabas
no flags Details | Formatted Diff | Diff
Workaround patch (2.74 KB, patch)
2012-11-08 05:13 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff
Rebased fix (3.37 KB, patch)
2012-11-09 04:26 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff
Proper fix (2.53 KB, patch)
2012-11-26 04:28 PST, Gabor Ballabas
zherczeg: review-
Details | Formatted Diff | Diff
Refactoring code duplication. (3.44 KB, patch)
2012-11-26 06:05 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff
Style fix (3.44 KB, patch)
2012-11-26 06:10 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff
Refactoring compare32 to private. (3.71 KB, patch)
2012-11-26 06:28 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff
Style fix (3.49 KB, patch)
2012-11-26 06:34 PST, Gabor Ballabas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2012-10-09 22:15:27 PDT
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
Comment 1 Csaba Osztrogonác 2012-10-11 10:24:52 PDT
ping?
Comment 2 Filip Pizlo 2012-10-11 12:02:41 PDT
(In reply to comment #1)
> ping?

You guys are probably doing something strange with branch patching.
Comment 3 Csaba Osztrogonác 2012-10-12 22:52:12 PDT
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?
Comment 4 Csaba Osztrogonác 2012-10-19 00:00:11 PDT
ping?
Comment 5 Gabor Ballabas 2012-10-19 04:31:46 PDT
(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.
Comment 6 Csaba Osztrogonác 2012-10-25 00:00:23 PDT
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?
Comment 7 Gabor Ballabas 2012-10-25 01:17:23 PDT
(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)
Comment 8 Gabor Ballabas 2012-10-25 01:39:57 PDT
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.
Comment 9 Filip Pizlo 2012-10-25 09:55:26 PDT
(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)
Comment 10 Gabor Ballabas 2012-10-26 04:21:53 PDT
(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.
Comment 11 Csaba Osztrogonác 2012-11-05 09:56:36 PST
any progress?
Comment 12 Csaba Osztrogonác 2012-11-06 04:47:11 PST
ping?
Comment 13 Gabor Ballabas 2012-11-08 05:13:54 PST
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.
Comment 14 Csaba Osztrogonác 2012-11-08 05:41:40 PST
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
Comment 15 Zoltan Herczeg 2012-11-08 07:13:06 PST
I will check this next week.
Comment 16 Gabor Ballabas 2012-11-09 04:26:25 PST
Created attachment 173263 [details]
Rebased fix
Comment 17 Csaba Osztrogonác 2012-11-21 02:27:23 PST
(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?
Comment 18 Filip Pizlo 2012-11-21 02:37:02 PST
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?
Comment 19 Gabor Ballabas 2012-11-26 04:28:35 PST
Created attachment 175969 [details]
Proper fix
Comment 20 Zoltan Herczeg 2012-11-26 05:35:16 PST
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.
Comment 21 Gabor Ballabas 2012-11-26 06:05:23 PST
Created attachment 175985 [details]
Refactoring code duplication.
Comment 22 WebKit Review Bot 2012-11-26 06:07:53 PST
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.
Comment 23 Gabor Ballabas 2012-11-26 06:10:31 PST
Created attachment 175987 [details]
Style fix
Comment 24 Gabor Ballabas 2012-11-26 06:28:53 PST
Created attachment 175993 [details]
Refactoring compare32 to private.
Comment 25 Gabor Ballabas 2012-11-26 06:34:28 PST
Created attachment 175994 [details]
Style fix
Comment 26 Zoltan Herczeg 2012-11-26 06:36:27 PST
Comment on attachment 175994 [details]
Style fix

Thanks, r=me
Comment 27 Csaba Osztrogonác 2012-11-26 06:48:31 PST
Comment on attachment 175994 [details]
Style fix

Clearing flags on attachment: 175994

Committed r135717: <http://trac.webkit.org/changeset/135717>
Comment 28 Csaba Osztrogonác 2012-11-26 06:48:39 PST
All reviewed patches have been landed.  Closing bug.
Comment 29 Csaba Osztrogonác 2012-11-27 01:57:39 PST
*** Bug 101832 has been marked as a duplicate of this bug. ***