Bug 160295 - [ARM] REGRESSION: generateSelfPropertyAccess shouldn't overwrite the constant pool
Summary: [ARM] REGRESSION: generateSelfPropertyAccess shouldn't overwrite the constant...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Blocks: 159408
  Show dependency treegraph
Reported: 2016-07-28 08:25 PDT by Csaba Osztrogonác
Modified: 2017-08-01 17:06 PDT (History)
7 users (show)

See Also:

Noisy Patch (11.67 KB, patch)
2017-08-01 17:06 PDT, Caio Lima
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-28 08:25:25 PDT
ARMv7 (ARM instruction set) backend is completely broken
due to the recent IC related refactoring/development.

I got one more regression related to this IC development.

generateSelfPropertyAccess() overwrites the constant pool
with nops which doesn't belong to IC code, but previous

It is easy to reproduce this bug on sunspider-1.0/3d-raytrace.js with default settings.
(There are only ~100 crashed on r203785 with other fixes applied:
 - https://trac.webkit.org/changeset/203816
 - https://bugs.webkit.org/attachment.cgi?id=284690 from bug159720
 - revert https://trac.webkit.org/changeset/203272 )

Let's see the generated JIT code

Generated Baseline JIT code for intersect#DSuR25:[0xafba61c0->0xafbdd3c0, BaselineFunctionCall, 368], instruction count = 368
   Source: function (orig, dir, near, far) { var u = (this.axis + 1) % 3; var v = (this.axis + 2) % 3; var d = dir[this.axis] + this.nu * dir[u] + this.nv * dir[v]; var t = (this.nd - orig[this.axis] - this.nu * orig[u] - this.nv * orig[v]) / d; if (t < near || t > far) return null; var Pu = orig[u] + t * dir[u] - this.eu; var Pv = orig[v] + t * dir[v] - this.ev; var a2 = Pv * this.nu1 + Pu * this.nv1; if (a2 < 0) return null; var a3 = Pu * this.nu2 + Pv * this.nv2; if (a3 < 0) return null; if ((a2 + a3) > 1) return null; return t; }
   Code at [0xb1506000, 0xb150a794):
    [ 129] get_by_val        loc12, arg1, loc13    ArrayWithDouble, Original; predicting Nonintasdouble
              0xb1506b60: [0xe5860000]  ldr     r6, [pc, #2152]           <============ read from constant pool adress: 0xb15073d0
              0xb1506b64: [0xe59f6864]  str     r0, [r6]                  <============ CRASH, because the address was overwritten
              0xb1506b68: [0xe5861000]  ldr     r6, [pc, #2148]
              0xb1506b6c: [0xe50b0068]  str     r1, [r6]
              0xb1506b70: [0xe50b1064]  str     r0, [r11, #-104]
              0xb1506b74: [0xe51b0060]  str     r1, [r11, #-100]
    [ 135] sub               loc11, loc11, loc12    results: Result:<Int32> LHS ObservedType:<Number> RHS ObservedType:<Number> LHS ResultType:<0x3e> RHS ResultType:<0x3e>
    [ 227] get_by_id         loc12, this, eu(@id4) llint(struct = 0xafba3a40 (offset = 5))    predicting Nonboolint32
              0xb1507388: [0xe59b1024]  ldr     r0, [r11, #32]
              0xb150738c: [0xea00090c]  ldr     r1, [r11, #36]
              0xb1507390: [0xe1a00000]  b       #9264
              0xb1507394: [0xe1a00000]  mov     r0, r0
              0xb1507398: [0xe1a00000]  mov     r0, r0
              0xb150739c: [0xe1a00000]  mov     r0, r0
              0xb15073a0: [0xe1a00000]  mov     r0, r0
              0xb15073a4: [0xe1a00000]  mov     r0, r0
              0xb15073a8: [0xe1a00000]  mov     r0, r0
              0xb15073ac: [0xe1a00000]  mov     r0, r0
              0xb15073b0: [0xe1a00000]  mov     r0, r0
              0xb15073b4: [0xe1a00000]  mov     r0, r0
              0xb15073b8: [0xe1a00000]  mov     r0, r0
              0xb15073bc: [0xe1a00000]  mov     r0, r0
              0xb15073c0: [0xea000077]  mov     r0, r0
              0xb15073c4: [0x00002c8c]  b       #476                 <================== constant pool starts with barrier
              0xb15073c8: [0x00000b60]  andeq   r2, r0, r12, lsl #25
              0xb15073cc: [0xaffebb58]  andeq   r0, r0, r0, ror #22
              0xb15073d0: [0xaffebb5c]  svcge   #16694104            <================== read from here before the CRASH
              0xb15073d4: [0x00000bb0]  svcge   #16694108
              0xb15073d8: [0x00000be0] unknown instruction

Let's see what's going wrong

generating 492 nops from this backtrace which overwrite the constant pool

1   0xb58087a4 JSC::LinkBuffer::allocate(JSC::MacroAssembler&, void*, JSC::JITCompilationEffort)
2   0xb58085f8 JSC::LinkBuffer::linkCode(JSC::MacroAssembler&, void*, JSC::JITCompilationEffort)
3   0xb58bef1c JSC::LinkBuffer::LinkBuffer(JSC::MacroAssembler&, void*, unsigned int, JSC::JITCompilationEffort, bool)
4   0xb58bd654
5   0xb58bca80 JSC::InlineAccess::generateSelfPropertyAccess(JSC::VM&, JSC::StructureStubInfo&, JSC::Structure*, int)
6   0xb5f81550
7   0xb5f82024 JSC::repatchGetByID(JSC::ExecState*, JSC::JSValue, JSC::Identifier const&, JSC::PropertySlot const&, JSC::StructureStubInfo&, JSC::GetByIDKind)
8   0xb5f48338
9   0xb5f54308
10  0xb5f53670
11  0xb5f48528

Generated JIT code for InlineAccessType: 'property access':
    Code at [0xb1507390, 0xb15075a8):
          0xb1507390: [0xe3036a40]      ldr     r12, [r0]
          0xb1507394: [0xe34a6fba]      movw    r6, #14912
          0xb1507398: [0xe15c0006]      movt    r6, #44986
          0xb150739c: [0x159fc010]      cmp     r12, r6
          0xb15073a0: [0x112fff1c]      ldrne   r12, [pc, #16]
          0xb15073a4: [0xe590103c]      bxne    r12
          0xb15073a8: [0xe5900038]      ldr     r1, [r0, #60]
          0xb15073ac: [0xea000001]      ldr     r0, [r0, #56]
          0xb15073b0: [0xe12fff7f]      b       #4
          0xb15073b4: [0xb15097c8]      bkpt    #65535
          0xb15073b8: [0xe1a00000] unknown instruction
          0xb15073bc: [0xe1a00000]      mov     r0, r0
          0xb15073c0: [0xe1a00000]      mov     r0, r0
          0xb15073c4: [0xe1a00000]      mov     r0, r0      < ============= constant pool barrier should be here, but was overwritten
          0xb15073c8: [0xe1a00000]      mov     r0, r0
          0xb15075a0: [0xe1a00000]      mov     r0, r0
          0xb15075a4: [0xe59f68e0]      mov     r0, r0

Have you got any idea how can we fix this serious regression? It seems the new IC
mechanism doesn't respect constant pools at all. :( My first idea is that we should
force flush constant pool before generating any code for get_by_id. Do you think
if it would help? And could you help where should I put this flush instruction?
Comment 1 Csaba Osztrogonác 2016-07-29 00:57:42 PDT
Could you give me some hints how can we fix this regression?
Comment 2 Csaba Osztrogonác 2016-07-29 11:08:34 PDT
Comment 3 Saam Barati 2016-08-07 14:21:59 PDT
So this happens when we're regenerating the IC?
We don't want to take into account the constant pool into the size of the IC.
Is there a predictable way to predict the size of a constant pool for code
with X number of branches when X is well known?
Comment 4 Caio Lima 2017-08-01 17:02:48 PDT
(In reply to Saam Barati from comment #3)
> So this happens when we're regenerating the IC?

No. The problem is happening when the getById fast path is being generated in "JITByIdGenerator::generateFastCommon" and the constant poll is flushed in the middle of IC code. As the logic in "JSC::LinkBuffer::allocate" is to fill al remaining IC code with nops, constant pool is then overwritten in such case. However, it also could be overwritten by IC repatch version as well.

> We don't want to take into account the constant pool into the size of the IC.

I've found a solution that I'm not happy with, but at least enables me run code with IC enabled.
Comment 5 Caio Lima 2017-08-01 17:06:37 PDT
Created attachment 316916 [details]
Noisy Patch

This Patch doesn't apply to master. Also, it's is just the hacking I've made to identify the problem. Maybe it can be useful to share. However, I'm evaluating if enable JIT into ARMv6 will provide performance gain as we expect and if we succeed on that I can revisit this bug afterwards.