- Revision
- 165414
- Author
- [email protected]
- Date
- 2014-03-10 16:31:18 -0700 (Mon, 10 Mar 2014)
Log Message
Repatch should save and restore all used registers - not just temp ones - when making a call
https://bugs.webkit.org/show_bug.cgi?id=130041
Reviewed by Geoffrey Garen and Mark Hahnenberg.
The save/restore code was written back when the only client was the DFG, which only uses a
subset of hardware registers: the "temp" registers in our lingo. But the FTL may use many
other registers, especially on ARM64. The fact that Repatch doesn't know to save those can
lead to data corruption on ARM64.
* jit/RegisterSet.cpp:
(JSC::RegisterSet::calleeSaveRegisters):
(JSC::RegisterSet::numberOfSetGPRs):
(JSC::RegisterSet::numberOfSetFPRs):
* jit/RegisterSet.h:
* jit/Repatch.cpp:
(JSC::storeToWriteBarrierBuffer):
(JSC::emitPutTransitionStub):
* jit/ScratchRegisterAllocator.cpp:
(JSC::ScratchRegisterAllocator::ScratchRegisterAllocator):
(JSC::ScratchRegisterAllocator::preserveReusedRegistersByPushing):
(JSC::ScratchRegisterAllocator::restoreReusedRegistersByPopping):
(JSC::ScratchRegisterAllocator::usedRegistersForCall):
(JSC::ScratchRegisterAllocator::desiredScratchBufferSizeForCall):
(JSC::ScratchRegisterAllocator::preserveUsedRegistersToScratchBufferForCall):
(JSC::ScratchRegisterAllocator::restoreUsedRegistersFromScratchBufferForCall):
* jit/ScratchRegisterAllocator.h:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (165413 => 165414)
--- trunk/Source/_javascript_Core/ChangeLog 2014-03-10 23:22:05 UTC (rev 165413)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-03-10 23:31:18 UTC (rev 165414)
@@ -1,3 +1,33 @@
+2014-03-10 Filip Pizlo <[email protected]>
+
+ Repatch should save and restore all used registers - not just temp ones - when making a call
+ https://bugs.webkit.org/show_bug.cgi?id=130041
+
+ Reviewed by Geoffrey Garen and Mark Hahnenberg.
+
+ The save/restore code was written back when the only client was the DFG, which only uses a
+ subset of hardware registers: the "temp" registers in our lingo. But the FTL may use many
+ other registers, especially on ARM64. The fact that Repatch doesn't know to save those can
+ lead to data corruption on ARM64.
+
+ * jit/RegisterSet.cpp:
+ (JSC::RegisterSet::calleeSaveRegisters):
+ (JSC::RegisterSet::numberOfSetGPRs):
+ (JSC::RegisterSet::numberOfSetFPRs):
+ * jit/RegisterSet.h:
+ * jit/Repatch.cpp:
+ (JSC::storeToWriteBarrierBuffer):
+ (JSC::emitPutTransitionStub):
+ * jit/ScratchRegisterAllocator.cpp:
+ (JSC::ScratchRegisterAllocator::ScratchRegisterAllocator):
+ (JSC::ScratchRegisterAllocator::preserveReusedRegistersByPushing):
+ (JSC::ScratchRegisterAllocator::restoreReusedRegistersByPopping):
+ (JSC::ScratchRegisterAllocator::usedRegistersForCall):
+ (JSC::ScratchRegisterAllocator::desiredScratchBufferSizeForCall):
+ (JSC::ScratchRegisterAllocator::preserveUsedRegistersToScratchBufferForCall):
+ (JSC::ScratchRegisterAllocator::restoreUsedRegistersFromScratchBufferForCall):
+ * jit/ScratchRegisterAllocator.h:
+
2014-03-10 Mark Hahnenberg <[email protected]>
Remove ConditionalStore barrier
Modified: trunk/Source/_javascript_Core/jit/RegisterSet.cpp (165413 => 165414)
--- trunk/Source/_javascript_Core/jit/RegisterSet.cpp 2014-03-10 23:22:05 UTC (rev 165413)
+++ trunk/Source/_javascript_Core/jit/RegisterSet.cpp 2014-03-10 23:31:18 UTC (rev 165414)
@@ -73,13 +73,26 @@
RegisterSet RegisterSet::calleeSaveRegisters()
{
RegisterSet result;
-#if CPU(X86_64)
+#if CPU(X86)
result.set(X86Registers::ebx);
result.set(X86Registers::ebp);
+ result.set(X86Registers::edi);
+ result.set(X86Registers::esi);
+#elif CPU(X86_64)
+ result.set(X86Registers::ebx);
+ result.set(X86Registers::ebp);
result.set(X86Registers::r12);
result.set(X86Registers::r13);
result.set(X86Registers::r14);
result.set(X86Registers::r15);
+#elif CPU(ARM_THUMB2)
+ result.set(ARMRegisters::r4);
+ result.set(ARMRegisters::r5);
+ result.set(ARMRegisters::r6);
+ result.set(ARMRegisters::r8);
+ result.set(ARMRegisters::r9);
+ result.set(ARMRegisters::r10);
+ result.set(ARMRegisters::r11);
#elif CPU(ARM64)
// We don't include LR in the set of callee-save registers even though it technically belongs
// there. This is because we use this set to describe the set of registers that need to be saved
@@ -126,6 +139,20 @@
return result;
}
+size_t RegisterSet::numberOfSetGPRs() const
+{
+ RegisterSet temp = *this;
+ temp.filter(allGPRs());
+ return temp.numberOfSetRegisters();
+}
+
+size_t RegisterSet::numberOfSetFPRs() const
+{
+ RegisterSet temp = *this;
+ temp.filter(allFPRs());
+ return temp.numberOfSetRegisters();
+}
+
void RegisterSet::dump(PrintStream& out) const
{
m_vector.dump(out);
Modified: trunk/Source/_javascript_Core/jit/RegisterSet.h (165413 => 165414)
--- trunk/Source/_javascript_Core/jit/RegisterSet.h 2014-03-10 23:22:05 UTC (rev 165413)
+++ trunk/Source/_javascript_Core/jit/RegisterSet.h 2014-03-10 23:31:18 UTC (rev 165414)
@@ -79,6 +79,8 @@
void filter(const RegisterSet& other) { m_vector.filter(other.m_vector); }
void exclude(const RegisterSet& other) { m_vector.exclude(other.m_vector); }
+ size_t numberOfSetGPRs() const;
+ size_t numberOfSetFPRs() const;
size_t numberOfSetRegisters() const { return m_vector.bitCount(); }
void dump(PrintStream&) const;
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (165413 => 165414)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2014-03-10 23:22:05 UTC (rev 165413)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2014-03-10 23:31:18 UTC (rev 165414)
@@ -1012,15 +1012,15 @@
slowPath.link(&stubJit);
allocator.restoreReusedRegistersByPopping(stubJit);
- ScratchBuffer* scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSize());
- allocator.preserveUsedRegistersToScratchBuffer(stubJit, scratchBuffer, scratchGPR1);
+ ScratchBuffer* scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSizeForCall());
+ allocator.preserveUsedRegistersToScratchBufferForCall(stubJit, scratchBuffer, scratchGPR1);
#if USE(JSVALUE64)
stubJit.setupArgumentsWithExecState(baseGPR, MacroAssembler::TrustedImmPtr(structure), MacroAssembler::TrustedImm32(slot.cachedOffset()), valueGPR);
#else
stubJit.setupArgumentsWithExecState(baseGPR, MacroAssembler::TrustedImmPtr(structure), MacroAssembler::TrustedImm32(slot.cachedOffset()), valueGPR, valueTagGPR);
#endif
operationCall = stubJit.call();
- allocator.restoreUsedRegistersFromScratchBuffer(stubJit, scratchBuffer, scratchGPR1);
+ allocator.restoreUsedRegistersFromScratchBufferForCall(stubJit, scratchBuffer, scratchGPR1);
successInSlowPath = stubJit.jump();
}
Modified: trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.cpp (165413 => 165414)
--- trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.cpp 2014-03-10 23:22:05 UTC (rev 165413)
+++ trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.cpp 2014-03-10 23:31:18 UTC (rev 165414)
@@ -33,7 +33,7 @@
namespace JSC {
-ScratchRegisterAllocator::ScratchRegisterAllocator(const TempRegisterSet& usedRegisters)
+ScratchRegisterAllocator::ScratchRegisterAllocator(const RegisterSet& usedRegisters)
: m_usedRegisters(usedRegisters)
, m_numberOfReusedRegisters(0)
{
@@ -97,12 +97,14 @@
return;
for (unsigned i = 0; i < FPRInfo::numberOfRegisters; ++i) {
- if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.getFPRByIndex(i))
- jit.pushToSave(FPRInfo::toRegister(i));
+ FPRReg reg = FPRInfo::toRegister(i);
+ if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.get(reg))
+ jit.pushToSave(reg);
}
for (unsigned i = 0; i < GPRInfo::numberOfRegisters; ++i) {
- if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.getGPRByIndex(i))
- jit.pushToSave(GPRInfo::toRegister(i));
+ GPRReg reg = GPRInfo::toRegister(i);
+ if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg))
+ jit.pushToSave(reg);
}
}
@@ -112,49 +114,61 @@
return;
for (unsigned i = GPRInfo::numberOfRegisters; i--;) {
- if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.getGPRByIndex(i))
- jit.popToRestore(GPRInfo::toRegister(i));
+ GPRReg reg = GPRInfo::toRegister(i);
+ if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg))
+ jit.popToRestore(reg);
}
for (unsigned i = FPRInfo::numberOfRegisters; i--;) {
- if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.getFPRByIndex(i))
- jit.popToRestore(FPRInfo::toRegister(i));
+ FPRReg reg = FPRInfo::toRegister(i);
+ if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.get(reg))
+ jit.popToRestore(reg);
}
}
-unsigned ScratchRegisterAllocator::desiredScratchBufferSize() const
+RegisterSet ScratchRegisterAllocator::usedRegistersForCall() const
{
- return m_usedRegisters.numberOfSetRegisters() * sizeof(JSValue);
+ RegisterSet result = m_usedRegisters;
+ result.exclude(RegisterSet::calleeSaveRegisters());
+ result.exclude(RegisterSet::stackRegisters());
+ result.exclude(RegisterSet::reservedHardwareRegisters());
+ return result;
}
-void ScratchRegisterAllocator::preserveUsedRegistersToScratchBuffer(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR)
+unsigned ScratchRegisterAllocator::desiredScratchBufferSizeForCall() const
{
+ return usedRegistersForCall().numberOfSetRegisters() * sizeof(JSValue);
+}
+
+void ScratchRegisterAllocator::preserveUsedRegistersToScratchBufferForCall(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR)
+{
+ RegisterSet usedRegisters = usedRegistersForCall();
+
unsigned count = 0;
- for (unsigned i = GPRInfo::numberOfRegisters; i--;) {
- if (m_usedRegisters.getGPRByIndex(i)) {
-#if USE(JSVALUE64)
- jit.store64(GPRInfo::toRegister(i), static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++));
-#else
- jit.store32(GPRInfo::toRegister(i), static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++));
-#endif
- }
- if (scratchGPR == InvalidGPRReg && !m_lockedRegisters.getGPRByIndex(i) && !m_scratchRegisters.getGPRByIndex(i))
- scratchGPR = GPRInfo::toRegister(i);
+ for (GPRReg reg = MacroAssembler::firstRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
+ if (usedRegisters.get(reg))
+ jit.storePtr(reg, static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++));
+ if (GPRInfo::toIndex(reg) != GPRInfo::InvalidIndex
+ && scratchGPR == InvalidGPRReg
+ && !m_lockedRegisters.get(reg) && !m_scratchRegisters.get(reg))
+ scratchGPR = reg;
}
RELEASE_ASSERT(scratchGPR != InvalidGPRReg);
- for (unsigned i = FPRInfo::numberOfRegisters; i--;) {
- if (m_usedRegisters.getFPRByIndex(i)) {
+ for (FPRReg reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
+ if (usedRegisters.get(reg)) {
jit.move(MacroAssembler::TrustedImmPtr(static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++)), scratchGPR);
- jit.storeDouble(FPRInfo::toRegister(i), scratchGPR);
+ jit.storeDouble(reg, scratchGPR);
}
}
- RELEASE_ASSERT(count * sizeof(JSValue) == desiredScratchBufferSize());
+ RELEASE_ASSERT(count * sizeof(JSValue) == desiredScratchBufferSizeForCall());
jit.move(MacroAssembler::TrustedImmPtr(scratchBuffer->activeLengthPtr()), scratchGPR);
jit.storePtr(MacroAssembler::TrustedImmPtr(static_cast<size_t>(count * sizeof(JSValue))), scratchGPR);
}
-void ScratchRegisterAllocator::restoreUsedRegistersFromScratchBuffer(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR)
+void ScratchRegisterAllocator::restoreUsedRegistersFromScratchBufferForCall(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR)
{
+ RegisterSet usedRegisters = usedRegistersForCall();
+
if (scratchGPR == InvalidGPRReg) {
// Find a scratch register.
for (unsigned i = GPRInfo::numberOfRegisters; i--;) {
@@ -165,28 +179,23 @@
}
}
RELEASE_ASSERT(scratchGPR != InvalidGPRReg);
-
+
jit.move(MacroAssembler::TrustedImmPtr(scratchBuffer->activeLengthPtr()), scratchGPR);
jit.storePtr(MacroAssembler::TrustedImmPtr(0), scratchGPR);
// Restore double registers first.
- unsigned count = m_usedRegisters.numberOfSetGPRs();
- for (unsigned i = FPRInfo::numberOfRegisters; i--;) {
- if (m_usedRegisters.getFPRByIndex(i)) {
+ unsigned count = usedRegisters.numberOfSetGPRs();
+ for (FPRReg reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
+ if (usedRegisters.get(reg)) {
jit.move(MacroAssembler::TrustedImmPtr(static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++)), scratchGPR);
- jit.loadDouble(scratchGPR, FPRInfo::toRegister(i));
+ jit.loadDouble(scratchGPR, reg);
}
}
count = 0;
- for (unsigned i = GPRInfo::numberOfRegisters; i--;) {
- if (m_usedRegisters.getGPRByIndex(i)) {
-#if USE(JSVALUE64)
- jit.load64(static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++), GPRInfo::toRegister(i));
-#else
- jit.load32(static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++), GPRInfo::toRegister(i));
-#endif
- }
+ for (GPRReg reg = MacroAssembler::firstRegister(); reg <= MacroAssembler::lastRegister(); reg = MacroAssembler::nextRegister(reg)) {
+ if (usedRegisters.get(reg))
+ jit.loadPtr(static_cast<EncodedJSValue*>(scratchBuffer->dataBuffer()) + (count++), reg);
}
}
Modified: trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.h (165413 => 165414)
--- trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.h 2014-03-10 23:22:05 UTC (rev 165413)
+++ trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.h 2014-03-10 23:31:18 UTC (rev 165414)
@@ -29,6 +29,7 @@
#if ENABLE(JIT)
#include "MacroAssembler.h"
+#include "RegisterSet.h"
#include "TempRegisterSet.h"
namespace JSC {
@@ -39,7 +40,7 @@
class ScratchRegisterAllocator {
public:
- ScratchRegisterAllocator(const TempRegisterSet& usedRegisters);
+ ScratchRegisterAllocator(const RegisterSet& usedRegisters);
~ScratchRegisterAllocator();
void lock(GPRReg reg);
@@ -64,14 +65,16 @@
void preserveReusedRegistersByPushing(MacroAssembler& jit);
void restoreReusedRegistersByPopping(MacroAssembler& jit);
- unsigned desiredScratchBufferSize() const;
+ RegisterSet usedRegistersForCall() const;
- void preserveUsedRegistersToScratchBuffer(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR = InvalidGPRReg);
+ unsigned desiredScratchBufferSizeForCall() const;
- void restoreUsedRegistersFromScratchBuffer(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR = InvalidGPRReg);
+ void preserveUsedRegistersToScratchBufferForCall(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR = InvalidGPRReg);
+ void restoreUsedRegistersFromScratchBufferForCall(MacroAssembler& jit, ScratchBuffer* scratchBuffer, GPRReg scratchGPR = InvalidGPRReg);
+
private:
- TempRegisterSet m_usedRegisters;
+ RegisterSet m_usedRegisters;
TempRegisterSet m_lockedRegisters;
TempRegisterSet m_scratchRegisters;
unsigned m_numberOfReusedRegisters;
