Repatch should save and restore all used registers - not just temp ones - when making...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Mar 2014 23:31:18 +0000 (23:31 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 10 Mar 2014 23:31:18 +0000 (23:31 +0000)
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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@165414 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/RegisterSet.cpp
Source/JavaScriptCore/jit/RegisterSet.h
Source/JavaScriptCore/jit/Repatch.cpp
Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp
Source/JavaScriptCore/jit/ScratchRegisterAllocator.h

index f221501..0072131 100644 (file)
@@ -1,3 +1,33 @@
+2014-03-10  Filip Pizlo  <fpizlo@apple.com>
+
+        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  <mhahnenberg@apple.com>
 
         Remove ConditionalStore barrier
index 3f84ae5..8bc7f29 100644 (file)
@@ -73,13 +73,26 @@ RegisterSet RegisterSet::specialRegisters()
 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 @@ RegisterSet RegisterSet::allRegisters()
     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);
index 9e559af..22f403f 100644 (file)
@@ -79,6 +79,8 @@ public:
     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;
index 6c38a0d..580174a 100644 (file)
@@ -1012,15 +1012,15 @@ static void emitPutTransitionStub(
         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();
     }
     
index a6903a9..6fdbbea 100644 (file)
@@ -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 @@ void ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler&
         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 @@ void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& j
         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 @@ void ScratchRegisterAllocator::restoreUsedRegistersFromScratchBuffer(MacroAssemb
         }
     }
     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);
     }
 }
 
index 57e4815..f6b94f6 100644 (file)
@@ -29,6 +29,7 @@
 #if ENABLE(JIT)
 
 #include "MacroAssembler.h"
+#include "RegisterSet.h"
 #include "TempRegisterSet.h"
 
 namespace JSC {
@@ -39,7 +40,7 @@ struct ScratchBuffer;
 
 class ScratchRegisterAllocator {
 public:
-    ScratchRegisterAllocator(const TempRegisterSet& usedRegisters);
+    ScratchRegisterAllocator(const RegisterSet& usedRegisters);
     ~ScratchRegisterAllocator();
 
     void lock(GPRReg reg);
@@ -64,14 +65,16 @@ public:
     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;