Skip to content

Commit

Permalink
[ARMv7] Fix concurrent BBQ repatching
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=273545

Reviewed by Keith Miller and Yusuke Suzuki.

Armv7 places a few interesting constraints on repatching in thumb mode:

- As far as I can tell, you cannot concurrently repatch a mov without an isb
- bl and family can be repatched without an isb, but bl is a 32-bit instruction.
So to repatch it concurrently, we need it to be 4-byte aligned.

This patch fixes this. I also added some debug assertions that verified that
this was the only place in our test case that tried to concurrently repatch an unaligned
bl, but the assertions were too involved to upstream.

This should fix export-arity.js crashes on armv7 on ToT.

* Source/JavaScriptCore/assembler/ARMv7Assembler.h:
(JSC::ARMv7Assembler::alignWithNop):
* Source/JavaScriptCore/assembler/AssemblerCommon.h:
(JSC::machineCodeCopy):
* Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:
(JSC::MacroAssemblerARMv7::threadSafePatchableNearCall):
(JSC::MacroAssemblerARMv7::threadSafePatchableNearTailCall):

Canonical link: https://commits.webkit.org/278305@main
  • Loading branch information
justinmichaud committed May 3, 2024
1 parent 78038e7 commit e9b5568
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 3 deletions.
8 changes: 8 additions & 0 deletions Source/JavaScriptCore/assembler/ARMv7Assembler.h
Original file line number Diff line number Diff line change
Expand Up @@ -2504,6 +2504,14 @@ class ARMv7Assembler {

return label();
}

AssemblerLabel alignWithNop(int alignment)
{
while (!m_formatter.isAligned(alignment))
nop();

return label();
}

static void* getRelocatedAddress(void* code, AssemblerLabel label)
{
Expand Down
13 changes: 13 additions & 0 deletions Source/JavaScriptCore/assembler/AssemblerCommon.h
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,19 @@ static void* performJITMemcpy(void *dst, const void *src, size_t n);
template<MachineCodeCopyMode copy>
ALWAYS_INLINE void* machineCodeCopy(void *dst, const void *src, size_t n)
{
#if CPU(ARM_THUMB2)
// For thumb instructions, we want to avoid the case where we have
// to repatch a 32-bit instruction that crosses 2 words.
bool isAligned = (dst == WTF::roundUpToMultipleOf<4>(dst));
if (n == 2 * sizeof(int16_t) && isAligned) {
*static_cast<uint32_t*>(dst) = *static_cast<const uint32_t*>(src);
return dst;
}
if (n == 1 * sizeof(int16_t)) {
*static_cast<uint16_t*>(dst) = *static_cast<const uint16_t*>(src);
return dst;
}
#endif
if constexpr (copy == MachineCodeCopyMode::Memcpy)
return memcpy(dst, src, n);
else
Expand Down
10 changes: 7 additions & 3 deletions Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h
Original file line number Diff line number Diff line change
Expand Up @@ -2804,16 +2804,20 @@ class MacroAssemblerARMv7 : public AbstractMacroAssembler<Assembler> {
return Call(m_assembler.b(), Call::LinkableNearTail);
}

// FIXME: why is this the same than nearCall() in ARM64? is it right?
ALWAYS_INLINE Call threadSafePatchableNearCall()
{
invalidateAllTempRegisters();
moveFixedWidthEncoding(TrustedImm32(0), dataTempRegister);
return Call(m_assembler.blx(dataTempRegister), Call::LinkableNear);
padBeforePatch();
m_assembler.alignWithNop(sizeof(int));
m_assembler.bl();
return Call(m_assembler.labelIgnoringWatchpoints(), Call::LinkableNear);
}

ALWAYS_INLINE Call threadSafePatchableNearTailCall()
{
invalidateAllTempRegisters();
padBeforePatch();
m_assembler.alignWithNop(sizeof(int));
return Call(m_assembler.b(), Call::LinkableNearTail);
}

Expand Down

0 comments on commit e9b5568

Please sign in to comment.