Bug 159711 - 64-bit alignment check isn't necessary in ARMAssembler::prepareExecutableCopy after r202214
Summary: 64-bit alignment check isn't necessary in ARMAssembler::prepareExecutableCopy...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Csaba Osztrogonác
URL:
Keywords:
Depends on:
Blocks: 159408
  Show dependency treegraph
 
Reported: 2016-07-13 04:01 PDT by Csaba Osztrogonác
Modified: 2016-07-28 02:29 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.44 KB, patch)
2016-07-13 04:03 PDT, Csaba Osztrogonác
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 2016-07-13 04:01:49 PDT
After r202214 this check is incorrect and causes crashes,
because the generated IC code isn't necessarily 64-bit sized.
Comment 1 Csaba Osztrogonác 2016-07-13 04:03:04 PDT
Created attachment 283510 [details]
Patch
Comment 2 Csaba Osztrogonác 2016-07-13 04:03:34 PDT
I'm going to attach debug backtrace soon to show what is the problem here.
Comment 3 Csaba Osztrogonác 2016-07-14 04:43:21 PDT
Program received signal SIGBUS, Bus error.
0xb27c42e8 in ?? ()
(gdb) bt
#0  0xb27c42e8 in ?? ()
#1  0xb6157554 in JSC::slow_path_enter (Cannot access memory at address 0xfffffffb
exec=0x0, pc=0xfffffffc) at ../../Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:596
#2  0xbeffe8e8 in ?? ()


(gdb) disas 0xb27c42b8,+100
Dump of assembler code from 0xb27c42b8 to 0xb27c431c:
   0xb27c42b8:  movw    r6, #31776      ; 0x7c20
   0xb27c42bc:  movt    r6, #45594      ; 0xb21a
   0xb27c42c0:  cmp     r12, r6
   0xb27c42c4:  ldrne   r12, [pc, #16]  ; 0xb27c42dc <====== load from constant pool
   0xb27c42c8:  bxne    r12
   0xb27c42cc:  ldr     r1, [r0, #20]
   0xb27c42d0:  ldr     r0, [r0, #16]
   0xb27c42d4:  b       0xb27c42e0 <===== jump after the constant pool
   0xb27c42d8:  bkpt    0xffff
   0xb27c42dc:  rsbslt  r5, r12, #84, 24        ; 0x5400
   0xb27c42e0:  nop                     ; (mov r0, r0)
   0xb27c42e4:  nop                     ; (mov r0, r0)
=> 0xb27c42e8:  bkpt    0x0000 <======= CRASH! We don't need this barrier.
   0xb27c42ec:  str     r0, [r6]
   0xb27c42f0:  ldr     r6, [pc, #1660] ; 0xb27c4974
   0xb27c42f4:  str     r1, [r6]
   0xb27c42f8:  str     r0, [r11, #-168]        ; 0xa8
   0xb27c42fc:  str     r1, [r11, #-164]        ; 0xa4
   0xb27c4300:  mov     r2, #0
   0xb27c4304:  mvn     r3, #0
   0xb27c4308:  ldr     r0, [r11, #-168]        ; 0xa8
   0xb27c430c:  ldr     r1, [r11, #-164]        ; 0xa4
   0xb27c4310:  cmn     r1, #5
   0xb27c4314:  bne     0xb27c5cd0
   0xb27c4318:  cmn     r3, #1
End of assembler dump.
Comment 4 Saam Barati 2016-07-14 18:52:59 PDT
Comment on attachment 283510 [details]
Patch

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

> Source/JavaScriptCore/assembler/ARMAssembler.cpp:-399
> -    if (!m_buffer.isAligned(8))
> -        bkpt(0);

Will we need this when we're not emitting code over old code?
Comment 5 Csaba Osztrogonác 2016-07-15 07:07:47 PDT
(In reply to comment #4)
> Comment on attachment 283510 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283510&action=review
> 
> > Source/JavaScriptCore/assembler/ARMAssembler.cpp:-399
> > -    if (!m_buffer.isAligned(8))
> > -        bkpt(0);
> 
> Will we need this when we're not emitting code over old code?

Practically we don't need this at all, it is placed to crash 
in case of implementation bug instead of undefined or bad behaviour.
Comment 6 Csaba Osztrogonác 2016-07-27 07:00:59 PDT
ping?
Comment 7 Mark Lam 2016-07-27 08:10:45 PDT
Comment on attachment 283510 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2016-07-28 02:29:24 PDT
Comment on attachment 283510 [details]
Patch

Clearing flags on attachment: 283510

Committed r203816: <http://trac.webkit.org/changeset/203816>
Comment 9 WebKit Commit Bot 2016-07-28 02:29:28 PDT
All reviewed patches have been landed.  Closing bug.