Bug 159720

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.

Created attachment 283634 [details] Patch

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.

(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?

auto jump = jit.jump(); This line uses constant pool before linkBuffer ctor call.

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

(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 .

(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.

(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.

(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.

(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.

(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.

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.

(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.

Created attachment 284690 [details] Patch It seems it fixes the assertion for me.

ping?

Created attachment 284778 [details] Patch updated after r203786, unfortunately now everything crashes, see bug160291 for details

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?

(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.