Bug 159720 - [ARM] ASSERTION FAILED: linkBuffer.isValid() in InlineAccess.cpp:291 after r202214 and r203537
Summary: [ARM] ASSERTION FAILED: linkBuffer.isValid() in InlineAccess.cpp:291 after r2...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 160157 (view as bug list)
Depends on:
Blocks: 159408 159649
  Show dependency treegraph
 
Reported: 2016-07-13 08:53 PDT by Csaba Osztrogonác
Modified: 2017-06-20 02:15 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2016-07-14 05:39 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.19 KB, patch)
2016-07-27 06:33 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2016-07-28 05:12 PDT, Csaba Osztrogonác
sbarati: review-
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 08:53:55 PDT
jsc-stress-results/.tests/cdjs-tests.yaml/cdjs$ ./../../.vm/JavaScriptCore.framework/Resources/jsc main.js

Program received signal SIGSEGV, Segmentation fault.
0xb64880b4 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
323	    *(int *)(uintptr_t)0xbbadbeef = 0;
(gdb) bt
#0  0xb64880b4 in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:323
#1  0xb5909240 in JSC::InlineAccess::rewireStubAsJump (vm=..., stubInfo=..., target=...)
    at ../../Source/JavaScriptCore/bytecode/InlineAccess.cpp:291
#2  0xb5fc087c in JSC::tryCachePutByID (exec=0xbeffe968, baseValue=..., structure=0xb21a7220, ident=..., slot=..., stubInfo=..., 
    putKind=JSC::NotDirect) at ../../Source/JavaScriptCore/jit/Repatch.cpp:452
#3  0xb5fc0a80 in JSC::repatchPutByID (exec=0xbeffe968, baseValue=..., structure=0xb21a7220, propertyName=..., slot=..., stubInfo=..., 
    putKind=JSC::NotDirect) at ../../Source/JavaScriptCore/jit/Repatch.cpp:463
#4  0xb5f88ca8 in JSC::operationPutByIdNonStrictOptimize (exec=0xbeffe968, stubInfo=0xb258dd80, encodedValue=-18486637472, 
    encodedBase=-18486456960, uid=0xb25aedf8) at ../../Source/JavaScriptCore/jit/JITOperations.cpp:421
#5  0xb27ca8ec in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Comment 1 Csaba Osztrogonác 2016-07-14 04:12:00 PDT
InlineAccess.cpp
=================
template <typename Function>
ALWAYS_INLINE static bool linkCodeInline(const char* name, CCallHelpers& jit, StructureStubInfo& stubInfo, const Function& function)
{
    if (jit.m_assembler.buffer().codeSize() <= stubInfo.patch.inlineSize) {
        bool needsBranchCompaction = false;
        LinkBuffer linkBuffer(jit, stubInfo.patch.start.dataLocation(), stubInfo.patch.inlineSize, JITCompilationMustSucceed, needsBranc
hCompaction);
        ASSERT(linkBuffer.isValid()); <=================== BANG!
        function(linkBuffer);
        FINALIZE_CODE(linkBuffer, ("InlineAccessType: '%s'", name));
        return true;
    }
...

LinkBuffer.h
=============
bool didFailToAllocate() const
{
    return !m_didAllocate;
}

bool isValid() const
{
    return !didFailToAllocate();
}

====
linkBuffer.isValid() is to ensure that LinkBuffer() constructor 
call sets its m_didAllocate member to true, but it isn't set.

- LinkBuffer::LinkBuffer(...) calls LinkBuffer::linkCode(...)
- LinkBuffer::linkCode(...) calls LinkBuffer::allocate(...)
- LinkBuffer::allocate(...):  initialSize = 12 > m_size = 4
and that's why allocate returns at the beginning without
allocation and setting m_didAllocate to true.

m_code = 0xb27ca808

Dump of assembler code from 0xb27ca808 to 0xb27ca860:
   0xb27ca808:  b       0xb27ca8b0
   0xb27ca80c:  nop                     ; (mov r0, r0)
   0xb27ca810:  nop                     ; (mov r0, r0)
   0xb27ca814:  nop                     ; (mov r0, r0)
   0xb27ca818:  nop                     ; (mov r0, r0)
   0xb27ca81c:  nop                     ; (mov r0, r0)
   0xb27ca820:  nop                     ; (mov r0, r0)
   0xb27ca824:  nop                     ; (mov r0, r0)
   0xb27ca828:  nop                     ; (mov r0, r0)
   0xb27ca82c:  nop                     ; (mov r0, r0)
   0xb27ca830:  nop                     ; (mov r0, r0)
   0xb27ca834:  nop                     ; (mov r0, r0)
   0xb27ca838:  ldr     r0, [r11, #32]
   0xb27ca83c:  ldr     r1, [r11, #36]  ; 0x24
   0xb27ca840:  tst     sp, #15
   0xb27ca844:  beq     0xb27ca850
   0xb27ca848:  mov     r6, #100        ; 0x64
   0xb27ca84c:  bkpt    0x0000
   0xb27ca850:  mov     sp, r11
   0xb27ca854:  pop     {r11}           ; (ldr r11, [sp], #4)
   0xb27ca858:  pop     {lr}            ; (ldr lr, [sp], #4)
   0xb27ca85c:  bx      lr


I think the assertion is simply incorrect in this case and should be removed.
But I don't understand exactly the original change, please let me know
if I misunderstood this bug.
Comment 2 Csaba Osztrogonác 2016-07-14 05:39:04 PDT
Created attachment 283634 [details]
Patch
Comment 3 Saam Barati 2016-07-14 18:40:27 PDT
Comment on attachment 283634 [details]
Patch

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

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:-291
> -    RELEASE_ASSERT(linkBuffer.isValid());

It doesn't make sense why this is failing.
We call linkBuffer constructor with initial size equal to jit.m_assembler.buffer().codeSize().

LinkBuffer::allocate() is written like:
void LinkBuffer::allocate(MacroAssembler& macroAssembler, void* ownerUID, JITCompilationEffort effort)
{
    size_t initialSize = macroAssembler.m_assembler.codeSize();
    if (m_code) {
        if (initialSize > m_size)
            return;
        
        size_t nopsToFillInBytes = m_size - initialSize;
        macroAssembler.emitNops(nopsToFillInBytes);
        m_didAllocate = true;
        return;
    }
    ....
--------------------
So this assertion is catching a real bug.
I suspect that it's something else that's adding code to the assembler buffer before calling allocate.
Look at ::linkCode():

void LinkBuffer::linkCode(MacroAssembler& macroAssembler, void* ownerUID, JITCompilationEffort effort)
{
#if !ENABLE(BRANCH_COMPACTION)
#if defined(ASSEMBLER_HAS_CONSTANT_POOL) && ASSEMBLER_HAS_CONSTANT_POOL
    macroAssembler.m_assembler.buffer().flushConstantPool(false);
#endif
    allocate(macroAssembler, ownerUID, effort);
    if (!m_didAllocate)
        return;
    ASSERT(m_code);
    AssemblerBuffer& buffer = macroAssembler.m_assembler.buffer();
#if CPU(ARM_TRADITIONAL)
    macroAssembler.m_assembler.prepareExecutableCopy(m_code);
#endif
    performJITMemcpy(m_code, buffer.data(), buffer.codeSize());
#if CPU(MIPS)
    macroAssembler.m_assembler.relocateJumps(buffer.data(), m_code);
#endif
#elif CPU(ARM_THUMB2)
    copyCompactAndLinkCode<uint16_t>(macroAssembler, ownerUID, effort);
#elif CPU(ARM64)
    copyCompactAndLinkCode<uint32_t>(macroAssembler, ownerUID, effort);
#endif // !ENABLE(BRANCH_COMPACTION)

    m_linkTasks = WTFMove(macroAssembler.m_linkTasks);
}

Maybe it's the call to prepareExecutableCopy which looks like it's trying to do some alignment. Or maybe it's the call to flushConstantPool.

I suspect it's the alignment. When we're generating code into an already allocated slot of memory, I suspect it's incorrect for the linkBuffer
to add more code to it unless it's certain it won't overflow the code size it's inserting into.
Comment 4 Csaba Osztrogonác 2016-07-25 04:51:03 PDT
(In reply to comment #3)
> So this assertion is catching a real bug.
> I suspect that it's something else that's adding code to the assembler
> buffer before calling allocate.
> Look at ::linkCode():
> 
> void LinkBuffer::linkCode(MacroAssembler& macroAssembler, void* ownerUID,
> JITCompilationEffort effort)
> {
> #if !ENABLE(BRANCH_COMPACTION)
> #if defined(ASSEMBLER_HAS_CONSTANT_POOL) && ASSEMBLER_HAS_CONSTANT_POOL
>     macroAssembler.m_assembler.buffer().flushConstantPool(false);
> #endif
>     allocate(macroAssembler, ownerUID, effort);
>     if (!m_didAllocate)
>         return;
>     ASSERT(m_code);
>     AssemblerBuffer& buffer = macroAssembler.m_assembler.buffer();
> #if CPU(ARM_TRADITIONAL)
>     macroAssembler.m_assembler.prepareExecutableCopy(m_code);
> #endif
>     performJITMemcpy(m_code, buffer.data(), buffer.codeSize());
> #if CPU(MIPS)
>     macroAssembler.m_assembler.relocateJumps(buffer.data(), m_code);
> #endif
> #elif CPU(ARM_THUMB2)
>     copyCompactAndLinkCode<uint16_t>(macroAssembler, ownerUID, effort);
> #elif CPU(ARM64)
>     copyCompactAndLinkCode<uint32_t>(macroAssembler, ownerUID, effort);
> #endif // !ENABLE(BRANCH_COMPACTION)
> 
>     m_linkTasks = WTFMove(macroAssembler.m_linkTasks);
> }
> 
> Maybe it's the call to prepareExecutableCopy which looks like it's trying to
> do some alignment. Or maybe it's the call to flushConstantPool.
> 
> I suspect it's the alignment. When we're generating code into an already
> allocated slot of memory, I suspect it's incorrect for the linkBuffer
> to add more code to it unless it's certain it won't overflow the code size
> it's inserting into.

I started debugging this issue and I can confirm that flushConstantPool
flushes one constant in this case. Is it incorrect behavior? If yes,
what should we do with this constant? What could be the proper fix?
Comment 5 Csaba Osztrogonác 2016-07-25 05:12:43 PDT
auto jump = jit.jump();

This line uses constant pool before linkBuffer ctor call.
Comment 6 Csaba Osztrogonác 2016-07-25 05:15:48 PDT
*** Bug 160157 has been marked as a duplicate of this bug. ***
Comment 7 Csaba Osztrogonác 2016-07-25 05:28:26 PDT
(In reply to comment #4)
> I started debugging this issue and I can confirm that flushConstantPool
> flushes one constant in this case. Is it incorrect behavior? If yes,
> what should we do with this constant? What could be the proper fix?

flushConstantPool call was added here ~ 3 years ago in
https://trac.webkit.org/changeset/157796 .
Comment 8 Julien Brianceau 2016-07-25 07:59:31 PDT
(In reply to comment #7)
> flushConstantPool call was added here ~ 3 years ago in
> https://trac.webkit.org/changeset/157796 .

Actually, flushConstantPool is there for a long time: https://trac.webkit.org/changeset/46057

In https://trac.webkit.org/changeset/157796, I moved its visibility from private to public.

About the assert issue, I have no idea right now.
Comment 9 Csaba Osztrogonác 2016-07-25 10:10:28 PDT
(In reply to comment #6)
> *** Bug 160157 has been marked as a duplicate of this bug. ***

This snippet fixes this assertion mentioned in bug160157
(https://trac.webkit.org/changeset/203537)

     size_t initialSize = macroAssembler.m_assembler.codeSize();
     if (m_code) {
-        if (initialSize > m_size)
+        if (initialSize > m_size) {
+            m_didAllocate = true;
             return;
+        }


Additionally it fixes the assertion caused by r202214 (with enabling ICs 
again and applying other patches from bug159408). But I noticed some
strange crashes (~only 100 tests) when generating IC overwrites unrelated
code with zillion nops.
Comment 10 Saam Barati 2016-07-25 10:39:07 PDT
(In reply to comment #9)
> (In reply to comment #6)
> > *** Bug 160157 has been marked as a duplicate of this bug. ***
> 
> This snippet fixes this assertion mentioned in bug160157
> (https://trac.webkit.org/changeset/203537)
> 
>      size_t initialSize = macroAssembler.m_assembler.codeSize();
>      if (m_code) {
> -        if (initialSize > m_size)
> +        if (initialSize > m_size) {
> +            m_didAllocate = true;
>              return;
> +        }
Is this a diff you're proposing? If so, this is completely wrong.
You're saying that we succeeded even if the memory we're overwriting is
not large enough to hold the assembly we generated.

> 
> 
> Additionally it fixes the assertion caused by r202214 (with enabling ICs 
> again and applying other patches from bug159408). But I noticed some
> strange crashes (~only 100 tests) when generating IC overwrites unrelated
> code with zillion nops.
Comment 11 Csaba Osztrogonác 2016-07-25 10:52:02 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #6)
> > > *** Bug 160157 has been marked as a duplicate of this bug. ***
> > 
> > This snippet fixes this assertion mentioned in bug160157
> > (https://trac.webkit.org/changeset/203537)
> > 
> >      size_t initialSize = macroAssembler.m_assembler.codeSize();
> >      if (m_code) {
> > -        if (initialSize > m_size)
> > +        if (initialSize > m_size) {
> > +            m_didAllocate = true;
> >              return;
> > +        }
> Is this a diff you're proposing? If so, this is completely wrong.
I'm just trying to debug what's wrong here and found that 
this hack fixed the assertion mentioned in bug160157. 

Could you explain what is the problem with flushConstantPool call
before allocate? 

Your code generates a jump which is and ldr with a constant pool entry 
which will contain the address. The flushConstantPool call flushes it
which makes initialSize bigger than m_size, which makes the assertion hit.

I don't any idea what would be the proper fix here.
Could you give me some hints?

> You're saying that we succeeded even if the memory we're overwriting is
> not large enough to hold the assembly we generated.
I haven't investigate this overwrite issue yet. I just noticed zillion
generated nops which overwrote a constant pool entry and caused crash.
Comment 12 Saam Barati 2016-07-25 10:56:32 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #6)
> > > > *** Bug 160157 has been marked as a duplicate of this bug. ***
> > > 
> > > This snippet fixes this assertion mentioned in bug160157
> > > (https://trac.webkit.org/changeset/203537)
> > > 
> > >      size_t initialSize = macroAssembler.m_assembler.codeSize();
> > >      if (m_code) {
> > > -        if (initialSize > m_size)
> > > +        if (initialSize > m_size) {
> > > +            m_didAllocate = true;
> > >              return;
> > > +        }
> > Is this a diff you're proposing? If so, this is completely wrong.
> I'm just trying to debug what's wrong here and found that 
> this hack fixed the assertion mentioned in bug160157. 
> 
> Could you explain what is the problem with flushConstantPool call
> before allocate? 
> 
> Your code generates a jump which is and ldr with a constant pool entry 
> which will contain the address. The flushConstantPool call flushes it
> which makes initialSize bigger than m_size, which makes the assertion hit.
> 
> I don't any idea what would be the proper fix here.
> Could you give me some hints?
You need to generate enough code in the inline path so that this is always allowed.
The numbers defined in InlineAccess.h should accommodate this.
How does the constant pool work? What code do we generate for the jump?
Do we plant the jump target in executable memory and do a load relative
to PC or something?

> 
> > You're saying that we succeeded even if the memory we're overwriting is
> > not large enough to hold the assembly we generated.
> I haven't investigate this overwrite issue yet. I just noticed zillion
> generated nops which overwrote a constant pool entry and caused crash.
Interesting. Maybe it's the fillNops below, though I doubt it. Lets start by
fixing things so we always have enough room to plant a jump in the inline path.
Comment 13 Csaba Osztrogonác 2016-07-26 11:20:55 PDT
Let's start again, I try to describe what happens step-by-step.

Source/JavaScriptCore/bytecode/InlineAccess.cpp
================================================
void InlineAccess::rewireStubAsJump(VM& vm, StructureStubInfo& stubInfo, CodeLocationLabel target)
{
    CCallHelpers jit(&vm);

    auto jump = jit.jump();

    // We don't need a nop sled here because nobody should be jumping into the middle of an IC.
    bool needsBranchCompaction = false;
    LinkBuffer linkBuffer(jit, stubInfo.patch.start.dataLocation(), jit.m_assembler.buffer().codeSize(), JITCompilationMustSucceed, needsBranchCompaction);
    RELEASE_ASSERT(linkBuffer.isValid());     <============== BANG !!!
    linkBuffer.link(jump, target);

    FINALIZE_CODE(linkBuffer, ("InlineAccess: linking constant jump"));
}

Source/JavaScriptCore/assembler/LinkBuffer.cpp
===============================================

void LinkBuffer::linkCode(MacroAssembler& macroAssembler, void* ownerUID, JITCompilationEffort effort)
{
#if !ENABLE(BRANCH_COMPACTION)
#if defined(ASSEMBLER_HAS_CONSTANT_POOL) && ASSEMBLER_HAS_CONSTANT_POOL
    macroAssembler.m_assembler.buffer().flushConstantPool(false);
#endif
    allocate(macroAssembler, ownerUID, effort);
    if (!m_didAllocate)
        return;
    ASSERT(m_code);
    AssemblerBuffer& buffer = macroAssembler.m_assembler.buffer();
#if CPU(ARM_TRADITIONAL)
    macroAssembler.m_assembler.prepareExecutableCopy(m_code);
#endif
    performJITMemcpy(m_code, buffer.data(), buffer.codeSize());
#if CPU(MIPS)
    macroAssembler.m_assembler.relocateJumps(buffer.data(), m_code);
#endif
#elif CPU(ARM_THUMB2)
    copyCompactAndLinkCode<uint16_t>(macroAssembler, ownerUID, effort);
#elif CPU(ARM64)
    copyCompactAndLinkCode<uint32_t>(macroAssembler, ownerUID, effort);
#endif // !ENABLE(BRANCH_COMPACTION)

    m_linkTasks = WTFMove(macroAssembler.m_linkTasks);
}


void LinkBuffer::allocate(MacroAssembler& macroAssembler, void* ownerUID, JITCompilationEffort effort)
{
    size_t initialSize = macroAssembler.m_assembler.codeSize();
    if (m_code) {
        if (initialSize > m_size)
            return;
        
        size_t nopsToFillInBytes = m_size - initialSize;
        macroAssembler.emitNops(nopsToFillInBytes);
        m_didAllocate = true;
        return;
    }




step 1:
-------
InlineAccess.cpp - InlineAccess::rewireStubAsJump

auto jump = jit.jump();
LinkBuffer linkBuffer(jit, stubInfo.patch.start.dataLocation(), jit.m_assembler.buffer().codeSize(), JITCompilationMustSucceed, needsBranchCompaction);

- create a jump instruction (4 bytes) + an entry in the constant pool.
- call LinkBuffer ctor with codeSize() == 4

step 2:
--------
LinkBuffer.cpp - LinkBuffer::linkCode

- LinkBuffer ctor calls LinkBuffer::linkCode
- flushConstantPool(true) is called

  The jump looks like after flushConstantPool(true):
  --------------------------------------------------------------------------
  0xbeffe5c4:	ldr	pc, [pc]	; 0xbeffe5cc
  0xbeffe5c8:	b	0xbeffe5d0
  0xbeffe5cc:			; <UNDEFINED> instruction: 0xffffffff
  --------------------------------------------------------------------------
  0xbeffe5c4: load the address from the constant pool and then jump to it
  0xbeffe5c8: barrier to jump over the constant pool (true arg of flushConstantPool() )
  0xbeffe5cc: constant pool, the real address will be stored here

- allocate is called, where 
  initialSize = 12 (because flushConstantPool added two more instructions)
  m_size = 4 (because LinkBuffer ctor call get 4 as argument)


    if (m_code) {
        if (initialSize > m_size)
            return;                <======== returns, m_didAllocate isn't set!
        
        size_t nopsToFillInBytes = m_size - initialSize;
        macroAssembler.emitNops(nopsToFillInBytes);
        m_didAllocate = true;
        return;
    }


step 3:
--------
- linkcode returns next to allocate because m_didAllocate is false

My previous hack which added "m_didAllocate = true;" before the return
let the linkCode continue its work and copy the new IC to the proper place.

My question is still open, what do you think, how should we fix this bug properly? There is enough room for the 12 bytes long jump (ldr, b, constant pool), but rewireStubAsJump doesn't respect the fact that the jump will be
bigger at the beginning of linkCode() function. We have same problems in
all newly added IC generator function.
Comment 14 Saam Barati 2016-07-26 11:36:58 PDT
(In reply to comment #13)
> Let's start again, I try to describe what happens step-by-step.
> 
> Source/JavaScriptCore/bytecode/InlineAccess.cpp
> ================================================
> void InlineAccess::rewireStubAsJump(VM& vm, StructureStubInfo& stubInfo,
> CodeLocationLabel target)
> {
>     CCallHelpers jit(&vm);
> 
>     auto jump = jit.jump();
> 
>     // We don't need a nop sled here because nobody should be jumping into
> the middle of an IC.
>     bool needsBranchCompaction = false;
>     LinkBuffer linkBuffer(jit, stubInfo.patch.start.dataLocation(),
> jit.m_assembler.buffer().codeSize(), JITCompilationMustSucceed,
> needsBranchCompaction);
>     RELEASE_ASSERT(linkBuffer.isValid());     <============== BANG !!!
>     linkBuffer.link(jump, target);
> 
>     FINALIZE_CODE(linkBuffer, ("InlineAccess: linking constant jump"));
> }
> 
> Source/JavaScriptCore/assembler/LinkBuffer.cpp
> ===============================================
> 
> void LinkBuffer::linkCode(MacroAssembler& macroAssembler, void* ownerUID,
> JITCompilationEffort effort)
> {
> #if !ENABLE(BRANCH_COMPACTION)
> #if defined(ASSEMBLER_HAS_CONSTANT_POOL) && ASSEMBLER_HAS_CONSTANT_POOL
>     macroAssembler.m_assembler.buffer().flushConstantPool(false);
> #endif
>     allocate(macroAssembler, ownerUID, effort);
>     if (!m_didAllocate)
>         return;
>     ASSERT(m_code);
>     AssemblerBuffer& buffer = macroAssembler.m_assembler.buffer();
> #if CPU(ARM_TRADITIONAL)
>     macroAssembler.m_assembler.prepareExecutableCopy(m_code);
> #endif
>     performJITMemcpy(m_code, buffer.data(), buffer.codeSize());
> #if CPU(MIPS)
>     macroAssembler.m_assembler.relocateJumps(buffer.data(), m_code);
> #endif
> #elif CPU(ARM_THUMB2)
>     copyCompactAndLinkCode<uint16_t>(macroAssembler, ownerUID, effort);
> #elif CPU(ARM64)
>     copyCompactAndLinkCode<uint32_t>(macroAssembler, ownerUID, effort);
> #endif // !ENABLE(BRANCH_COMPACTION)
> 
>     m_linkTasks = WTFMove(macroAssembler.m_linkTasks);
> }
> 
> 
> void LinkBuffer::allocate(MacroAssembler& macroAssembler, void* ownerUID,
> JITCompilationEffort effort)
> {
>     size_t initialSize = macroAssembler.m_assembler.codeSize();
>     if (m_code) {
>         if (initialSize > m_size)
>             return;
>         
>         size_t nopsToFillInBytes = m_size - initialSize;
>         macroAssembler.emitNops(nopsToFillInBytes);
>         m_didAllocate = true;
>         return;
>     }
> 
> 
> 
> 
> step 1:
> -------
> InlineAccess.cpp - InlineAccess::rewireStubAsJump
> 
> auto jump = jit.jump();
> LinkBuffer linkBuffer(jit, stubInfo.patch.start.dataLocation(),
> jit.m_assembler.buffer().codeSize(), JITCompilationMustSucceed,
> needsBranchCompaction);
> 
> - create a jump instruction (4 bytes) + an entry in the constant pool.
> - call LinkBuffer ctor with codeSize() == 4
> 
> step 2:
> --------
> LinkBuffer.cpp - LinkBuffer::linkCode
> 
> - LinkBuffer ctor calls LinkBuffer::linkCode
> - flushConstantPool(true) is called
> 
>   The jump looks like after flushConstantPool(true):
>   --------------------------------------------------------------------------
>   0xbeffe5c4:	ldr	pc, [pc]	; 0xbeffe5cc
>   0xbeffe5c8:	b	0xbeffe5d0
>   0xbeffe5cc:			; <UNDEFINED> instruction: 0xffffffff
>   --------------------------------------------------------------------------
>   0xbeffe5c4: load the address from the constant pool and then jump to it
>   0xbeffe5c8: barrier to jump over the constant pool (true arg of
> flushConstantPool() )
>   0xbeffe5cc: constant pool, the real address will be stored here
> 
> - allocate is called, where 
>   initialSize = 12 (because flushConstantPool added two more instructions)
>   m_size = 4 (because LinkBuffer ctor call get 4 as argument)
> 
> 
>     if (m_code) {
>         if (initialSize > m_size)
>             return;                <======== returns, m_didAllocate isn't
> set!
>         
>         size_t nopsToFillInBytes = m_size - initialSize;
>         macroAssembler.emitNops(nopsToFillInBytes);
>         m_didAllocate = true;
>         return;
>     }
> 
> 
> step 3:
> --------
> - linkcode returns next to allocate because m_didAllocate is false
> 
> My previous hack which added "m_didAllocate = true;" before the return
> let the linkCode continue its work and copy the new IC to the proper place.
> 
> My question is still open, what do you think, how should we fix this bug
> properly? There is enough room for the 12 bytes long jump (ldr, b, constant
> pool), but rewireStubAsJump doesn't respect the fact that the jump will be
> bigger at the beginning of linkCode() function. We have same problems in
> all newly added IC generator function.

I see. I think the problem here is that LinkBuffer will actually grow the code size after
it's passed an assembler. What we want, logically, is to have flushConstantPool() to be
called before we pass in the assembler to LinkBuffer (the best way to accomplish this I'm not sure).
Ans then we want to assert that the assembler buffer size after flushConstantPool() was called
is <= the inline size reserved by StructureStubInfo.
Comment 15 Csaba Osztrogonác 2016-07-27 06:33:34 PDT
Created attachment 284690 [details]
Patch

It seems it fixes the assertion for me.
Comment 16 Csaba Osztrogonác 2016-07-28 02:08:14 PDT
ping?
Comment 17 Csaba Osztrogonác 2016-07-28 05:12:03 PDT
Created attachment 284778 [details]
Patch

updated after r203786, unfortunately now everything crashes, see bug160291 for details
Comment 18 Saam Barati 2016-07-28 08:46:25 PDT
Comment on attachment 284778 [details]
Patch

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

> Source/JavaScriptCore/jit/JITMathIC.h:127
> +            jit.m_assembler.buffer().flushConstantPool();

This is not all you want here. You want to make sure all the fast paths leave enough space for a constant pool flush.
If you look elsewhere in this file, you'll see we refer to maxJumpSize (I forget the exact name).
We'd want something similar for this. Can we reliably predict the size of the constant pool of a single jump?
Comment 19 Csaba Osztrogonác 2016-07-29 01:14:19 PDT
(In reply to comment #18)
> Comment on attachment 284778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284778&action=review
> 
> > Source/JavaScriptCore/jit/JITMathIC.h:127
> > +            jit.m_assembler.buffer().flushConstantPool();
> 
> This is not all you want here. You want to make sure all the fast paths
> leave enough space for a constant pool flush.
> If you look elsewhere in this file, you'll see we refer to maxJumpSize (I
> forget the exact name).
> We'd want something similar for this. 

I found that MacroAssembler::maxJumpReplacementSize() - inlineSize nops
are added in generateInline() :
https://trac.webkit.org/browser/trunk/Source/JavaScriptCore/jit/JITMathIC.h#L88

But I have no clue how many nops should be added and where.
I really don't understand what this generateOutOfLine does and why. :-/

> Can we reliably predict the size of the constant pool of a single jump?
I don't think so. Normally a jump could be an ldr + a branch + a constant
(3 machine word, 3x4 = 12 bytes) But we can't determine how many constants
are flushed. Constant pool are flushed if it is full or if it is triggered
explicitly. Sometimes a flush call flushes hundreds constants belong to
the previous couple opcodes. It is exactly what happens in bug160295.

It seems the existance of constant pool and this new IC generating mechanism
aren't compatible fundamentally. It would be great to find a way how to fix
these issues. 

Maybe we should make the ARM traditional assembler somehow not to use
constant pool for IC jumps, but movw+mowt+branch. But I don't know if
other instructions in IC codes need constant pool or not. Additionally
we should force flush the constant pool somehow before all IC generation:
bug160295.