C calls in PolymorphicAccess shouldn't assume that the top of the stack looks like...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Oct 2015 21:49:08 +0000 (21:49 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 21 Oct 2015 21:49:08 +0000 (21:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=125711

Reviewed by Filip Pizlo.

This patch ensures that anytime we need to make a C call inside
PolymorphicAccess, we ensure there is enough space on the stack to do so.

This patch also enables GetByIdFlush/PutByIdFlush inside the FTL.
Because PolymorphicAccess now spills the necessary registers
before making a JS/C call, any registers that LLVM report as
being in use for the patchpoint will be spilled before making
a call by PolymorphicAccess.

* bytecode/PolymorphicAccess.cpp:
(JSC::AccessGenerationState::restoreScratch):
(JSC::AccessGenerationState::succeed):
(JSC::AccessGenerationState::calculateLiveRegistersForCallAndExceptionHandling):
(JSC::AccessCase::generate):
(JSC::PolymorphicAccess::regenerate):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileNode):
(JSC::FTL::DFG::LowerDFGToLLVM::compileGetById):
(JSC::FTL::DFG::LowerDFGToLLVM::emitStoreBarrier):
* jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::emitTypeOf):
(JSC::AssemblyHelpers::makeSpaceOnStackForCCall):
(JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall):
* jit/RegisterSet.cpp:
(JSC::RegisterSet::webAssemblyCalleeSaveRegisters):
(JSC::RegisterSet::registersToNotSaveForJSCall):
(JSC::RegisterSet::registersToNotSaveForCCall):
(JSC::RegisterSet::allGPRs):
(JSC::RegisterSet::registersToNotSaveForCall): Deleted.
* jit/RegisterSet.h:
(JSC::RegisterSet::set):
* jit/ScratchRegisterAllocator.cpp:
(JSC::ScratchRegisterAllocator::allocateScratchGPR):
(JSC::ScratchRegisterAllocator::allocateScratchFPR):
(JSC::ScratchRegisterAllocator::preserveReusedRegistersByPushing):
(JSC::ScratchRegisterAllocator::restoreReusedRegistersByPopping):
These methods now take an extra parameter indicating if they
should create space for a C call at the top of the stack if
there are any reused registers to spill.

(JSC::ScratchRegisterAllocator::usedRegistersForCall):
* jit/ScratchRegisterAllocator.h:
(JSC::ScratchRegisterAllocator::usedRegisters):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/jit/AssemblyHelpers.h
Source/JavaScriptCore/jit/RegisterSet.cpp
Source/JavaScriptCore/jit/RegisterSet.h
Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp
Source/JavaScriptCore/jit/ScratchRegisterAllocator.h

index df6c927..5388461 100644 (file)
@@ -1,3 +1,56 @@
+2015-10-21  Saam barati  <sbarati@apple.com>
+
+        C calls in PolymorphicAccess shouldn't assume that the top of the stack looks like a JSC JIT frame and enable *ByIdFlush in FTL
+        https://bugs.webkit.org/show_bug.cgi?id=125711
+
+        Reviewed by Filip Pizlo.
+
+        This patch ensures that anytime we need to make a C call inside
+        PolymorphicAccess, we ensure there is enough space on the stack to do so.
+
+        This patch also enables GetByIdFlush/PutByIdFlush inside the FTL.
+        Because PolymorphicAccess now spills the necessary registers
+        before making a JS/C call, any registers that LLVM report as
+        being in use for the patchpoint will be spilled before making
+        a call by PolymorphicAccess.
+
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::AccessGenerationState::restoreScratch):
+        (JSC::AccessGenerationState::succeed):
+        (JSC::AccessGenerationState::calculateLiveRegistersForCallAndExceptionHandling):
+        (JSC::AccessCase::generate):
+        (JSC::PolymorphicAccess::regenerate):
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileNode):
+        (JSC::FTL::DFG::LowerDFGToLLVM::compileGetById):
+        (JSC::FTL::DFG::LowerDFGToLLVM::emitStoreBarrier):
+        * jit/AssemblyHelpers.h:
+        (JSC::AssemblyHelpers::emitTypeOf):
+        (JSC::AssemblyHelpers::makeSpaceOnStackForCCall):
+        (JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall):
+        * jit/RegisterSet.cpp:
+        (JSC::RegisterSet::webAssemblyCalleeSaveRegisters):
+        (JSC::RegisterSet::registersToNotSaveForJSCall):
+        (JSC::RegisterSet::registersToNotSaveForCCall):
+        (JSC::RegisterSet::allGPRs):
+        (JSC::RegisterSet::registersToNotSaveForCall): Deleted.
+        * jit/RegisterSet.h:
+        (JSC::RegisterSet::set):
+        * jit/ScratchRegisterAllocator.cpp:
+        (JSC::ScratchRegisterAllocator::allocateScratchGPR):
+        (JSC::ScratchRegisterAllocator::allocateScratchFPR):
+        (JSC::ScratchRegisterAllocator::preserveReusedRegistersByPushing):
+        (JSC::ScratchRegisterAllocator::restoreReusedRegistersByPopping):
+        These methods now take an extra parameter indicating if they
+        should create space for a C call at the top of the stack if
+        there are any reused registers to spill.
+
+        (JSC::ScratchRegisterAllocator::usedRegistersForCall):
+        * jit/ScratchRegisterAllocator.h:
+        (JSC::ScratchRegisterAllocator::usedRegisters):
+
 2015-10-21  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Array previews with Symbol objects have too few preview values
index 40e12ab..4c77ee6 100644 (file)
@@ -36,7 +36,6 @@
 #include "JITOperations.h"
 #include "JSCInlines.h"
 #include "LinkBuffer.h"
-#include "MaxFrameExtentForSlowPathCall.h"
 #include "ScratchRegisterAllocator.h"
 #include "StructureStubClearingWatchpoint.h"
 #include "StructureStubInfo.h"
@@ -55,7 +54,7 @@ Watchpoint* AccessGenerationState::addWatchpoint(const ObjectPropertyCondition&
 
 void AccessGenerationState::restoreScratch()
 {
-    allocator->restoreReusedRegistersByPopping(*jit, numberOfBytesUsedToPreserveReusedRegisters);
+    allocator->restoreReusedRegistersByPopping(*jit, numberOfBytesUsedToPreserveReusedRegisters, ScratchRegisterAllocator::ExtraStackSpace::NoExtraSpace);
 }
 
 void AccessGenerationState::succeed()
@@ -75,7 +74,7 @@ void AccessGenerationState::calculateLiveRegistersForCallAndExceptionHandling()
             RELEASE_ASSERT(JITCode::isOptimizingJIT(jit->codeBlock()->jitType()));
 
         m_liveRegistersForCall = RegisterSet(m_liveRegistersToPreserveAtExceptionHandlingCallSite, allocator->usedRegisters());
-        m_liveRegistersForCall.exclude(RegisterSet::registersToNotSaveForCall());
+        m_liveRegistersForCall.exclude(RegisterSet::registersToNotSaveForJSCall());
     }
 }
 
@@ -744,17 +743,12 @@ void AccessCase::generate(AccessGenerationState& state)
                         CodeLocationLabel(vm.getCTIStub(linkCallThunkGenerator).code()));
                 });
         } else {
-            unsigned stackOffset = 0;
-            // Need to make room for the C call so our spillage isn't overwritten.
-            if (state.numberOfStackBytesUsedForRegisterPreservation()) {
-                if (maxFrameExtentForSlowPathCall)
-                    stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
-            }
-            if (stackOffset) {
-                jit.subPtr(
-                    CCallHelpers::TrustedImm32(stackOffset),
-                    CCallHelpers::stackPointerRegister);
-            }
+            // Need to make room for the C call so any of our stack spillage isn't overwritten.
+            // We also need to make room because we may be an inline cache in the FTL and not
+            // have a JIT call frame.
+            bool needsToMakeRoomOnStackForCCall = state.numberOfStackBytesUsedForRegisterPreservation() || codeBlock->jitType() == JITCode::FTLJIT;
+            if (needsToMakeRoomOnStackForCCall)
+                jit.makeSpaceOnStackForCCall();
 
             // getter: EncodedJSValue (*GetValueFunc)(ExecState*, JSObject* slotBase, EncodedJSValue thisValue, PropertyName);
             // setter: void (*PutValueFunc)(ExecState*, JSObject* base, EncodedJSValue thisObject, EncodedJSValue value);
@@ -783,12 +777,8 @@ void AccessCase::generate(AccessGenerationState& state)
             operationCall = jit.call();
             if (m_type == CustomGetter)
                 jit.setupResults(valueRegs);
-
-            if (stackOffset) {
-                jit.addPtr(
-                    CCallHelpers::TrustedImm32(stackOffset),
-                    CCallHelpers::stackPointerRegister);
-            }
+            if (needsToMakeRoomOnStackForCCall)
+                jit.reclaimSpaceOnStackForCCall();
 
             CCallHelpers::Jump noException =
                 jit.emitExceptionCheck(CCallHelpers::InvertedExceptionCheck);
@@ -893,11 +883,12 @@ void AccessCase::generate(AccessGenerationState& state)
         else
             scratchGPR3 = InvalidGPRReg;
 
-        size_t numberOfBytesUsedToPreserveReusedRegisters = allocator.preserveReusedRegistersByPushing(jit);
+        size_t numberOfBytesUsedToPreserveReusedRegisters = allocator.preserveReusedRegistersByPushing(jit, ScratchRegisterAllocator::ExtraStackSpace::SpaceForCCall);
 
         ASSERT(structure()->transitionWatchpointSetHasBeenInvalidated());
 
         bool scratchGPRHasStorage = false;
+        bool needsToMakeRoomOnStackForCCall = !numberOfBytesUsedToPreserveReusedRegisters && codeBlock->jitType() == JITCode::FTLJIT;
 
         if (newStructure()->outOfLineCapacity() != structure()->outOfLineCapacity()) {
             size_t newSize = newStructure()->outOfLineCapacity() * sizeof(JSValue);
@@ -1004,8 +995,12 @@ void AccessCase::generate(AccessGenerationState& state)
             // barrier slow path was just the normal slow path, below.
             // https://bugs.webkit.org/show_bug.cgi?id=149030
             allocator.preserveUsedRegistersToScratchBufferForCall(jit, scratchBuffer, scratchGPR2);
+            if (needsToMakeRoomOnStackForCCall)
+                jit.makeSpaceOnStackForCCall();
             jit.setupArgumentsWithExecState(baseGPR);
             callFlushWriteBarrierBuffer = jit.call();
+            if (needsToMakeRoomOnStackForCCall)
+                jit.reclaimSpaceOnStackForCCall();
             allocator.restoreUsedRegistersFromScratchBufferForCall(
                 jit, scratchBuffer, scratchGPR2);
 
@@ -1018,13 +1013,15 @@ void AccessCase::generate(AccessGenerationState& state)
                 });
         }
         
-        allocator.restoreReusedRegistersByPopping(jit, numberOfBytesUsedToPreserveReusedRegisters);
+        allocator.restoreReusedRegistersByPopping(jit, numberOfBytesUsedToPreserveReusedRegisters, ScratchRegisterAllocator::ExtraStackSpace::SpaceForCCall);
         state.succeed();
 
         if (newStructure()->outOfLineCapacity() != structure()->outOfLineCapacity()) {
             slowPath.link(&jit);
-            allocator.restoreReusedRegistersByPopping(jit, numberOfBytesUsedToPreserveReusedRegisters);
+            allocator.restoreReusedRegistersByPopping(jit, numberOfBytesUsedToPreserveReusedRegisters, ScratchRegisterAllocator::ExtraStackSpace::SpaceForCCall);
             allocator.preserveUsedRegistersToScratchBufferForCall(jit, scratchBuffer, scratchGPR);
+            if (needsToMakeRoomOnStackForCCall)
+                jit.makeSpaceOnStackForCCall();
 #if USE(JSVALUE64)
             jit.setupArgumentsWithExecState(
                 baseGPR,
@@ -1039,6 +1036,8 @@ void AccessCase::generate(AccessGenerationState& state)
                 valueRegs.payloadGPR(), valueRegs.tagGPR());
 #endif
             CCallHelpers::Call operationCall = jit.call();
+            if (needsToMakeRoomOnStackForCCall)
+                jit.reclaimSpaceOnStackForCCall();
             allocator.restoreUsedRegistersFromScratchBufferForCall(jit, scratchBuffer, scratchGPR);
             state.succeed();
 
@@ -1240,7 +1239,7 @@ MacroAssemblerCodePtr PolymorphicAccess::regenerate(
     CCallHelpers jit(&vm, codeBlock);
     state.jit = &jit;
 
-    state.numberOfBytesUsedToPreserveReusedRegisters = allocator.preserveReusedRegistersByPushing(jit);
+    state.numberOfBytesUsedToPreserveReusedRegisters = allocator.preserveReusedRegistersByPushing(jit, ScratchRegisterAllocator::ExtraStackSpace::NoExtraSpace);
 
     bool allGuardedByStructureCheck = true;
     bool hasJSGetterSetterCall = false;
index 537f8f6..2a04218 100644 (file)
@@ -159,6 +159,7 @@ inline CapabilityLevel canCompile(Node* node)
     case MakeRope:
     case NewArrayWithSize:
     case GetById:
+    case GetByIdFlush:
     case ToThis:
     case MultiGetByOffset:
     case MultiPutByOffset:
@@ -222,6 +223,7 @@ inline CapabilityLevel canCompile(Node* node)
         return CannotCompile;
     case PutByIdDirect:
     case PutById:
+    case PutByIdFlush:
         if (node->child1().useKind() == CellUse)
             break;
         return CannotCompile;
index 5cd8c3a..826fc07 100644 (file)
@@ -554,6 +554,7 @@ private:
             compilePutStructure();
             break;
         case GetById:
+        case GetByIdFlush:
             compileGetById();
             break;
         case In:
@@ -561,6 +562,7 @@ private:
             break;
         case PutById:
         case PutByIdDirect:
+        case PutByIdFlush:
             compilePutById();
             break;
         case GetButterfly:
@@ -2212,9 +2214,6 @@ private:
     
     void compileGetById()
     {
-        // Pretty much the only reason why we don't also support GetByIdFlush is because:
-        // https://bugs.webkit.org/show_bug.cgi?id=125711
-        
         switch (m_node->child1().useKind()) {
         case CellUse: {
             setJSValue(getById(lowCell(m_node->child1())));
@@ -8596,7 +8595,7 @@ private:
                         GPRReg scratch2 = scratchRegisterAllocator.allocateScratchGPR();
 
                         unsigned bytesPushed =
-                            scratchRegisterAllocator.preserveReusedRegistersByPushing(jit);
+                            scratchRegisterAllocator.preserveReusedRegistersByPushing(jit, ScratchRegisterAllocator::ExtraStackSpace::SpaceForCCall);
 
                         // We've already saved these, so when we make a slow path call, we don't have
                         // to save them again.
@@ -8618,7 +8617,7 @@ private:
                                 scratch1, scratch2, CCallHelpers::ScalePtr,
                                 static_cast<int32_t>(-sizeof(void*))));
 
-                        scratchRegisterAllocator.restoreReusedRegistersByPopping(jit, bytesPushed);
+                        scratchRegisterAllocator.restoreReusedRegistersByPopping(jit, bytesPushed, ScratchRegisterAllocator::ExtraStackSpace::SpaceForCCall);
 
                         params.doneJumps.append(jit.jump());
 
@@ -8627,7 +8626,7 @@ private:
                             usedRegisters, jit, params.lazySlowPath->callSiteIndex(),
                             params.exceptionJumps, operationFlushWriteBarrierBuffer, InvalidGPRReg,
                             baseGPR);
-                        scratchRegisterAllocator.restoreReusedRegistersByPopping(jit, bytesPushed);
+                        scratchRegisterAllocator.restoreReusedRegistersByPopping(jit, bytesPushed, ScratchRegisterAllocator::ExtraStackSpace::SpaceForCCall);
                         params.doneJumps.append(jit.jump());
                     });
             },
index f772de9..147820c 100644 (file)
@@ -35,6 +35,7 @@
 #include "InlineCallFrame.h"
 #include "JITCode.h"
 #include "MacroAssembler.h"
+#include "MaxFrameExtentForSlowPathCall.h"
 #include "RegisterAtOffsetList.h"
 #include "RegisterSet.h"
 #include "TypeofType.h"
@@ -1322,6 +1323,20 @@ public:
     }
 
     Vector<BytecodeAndMachineOffset>& decodedCodeMapFor(CodeBlock*);
+
+    void makeSpaceOnStackForCCall()
+    {
+        unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
+        if (stackOffset)
+            subPtr(TrustedImm32(stackOffset), stackPointerRegister);
+    }
+
+    void reclaimSpaceOnStackForCCall()
+    {
+        unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
+        if (stackOffset)
+            addPtr(TrustedImm32(stackOffset), stackPointerRegister);
+    }
     
 protected:
     VM* m_vm;
index 86b5bf6..9ef0c11 100644 (file)
@@ -307,11 +307,16 @@ RegisterSet RegisterSet::webAssemblyCalleeSaveRegisters()
 }
 #endif
 
-RegisterSet RegisterSet::registersToNotSaveForCall()
+RegisterSet RegisterSet::registersToNotSaveForJSCall()
 {
     return RegisterSet(RegisterSet::vmCalleeSaveRegisters(), RegisterSet::stackRegisters(), RegisterSet::reservedHardwareRegisters());
 }
 
+RegisterSet RegisterSet::registersToNotSaveForCCall()
+{
+    return RegisterSet(RegisterSet::calleeSaveRegisters(), RegisterSet::stackRegisters(), RegisterSet::reservedHardwareRegisters());
+}
+
 RegisterSet RegisterSet::allGPRs()
 {
     RegisterSet result;
index 6400fd7..deb7207 100644 (file)
@@ -62,7 +62,8 @@ public:
     static RegisterSet allFPRs();
     static RegisterSet allRegisters();
 
-    static RegisterSet registersToNotSaveForCall();
+    static RegisterSet registersToNotSaveForJSCall();
+    static RegisterSet registersToNotSaveForCCall();
     
     void set(Reg reg, bool value = true)
     {
index 66f6dc7..70c4d4c 100644 (file)
@@ -102,7 +102,7 @@ typename BankInfo::RegisterType ScratchRegisterAllocator::allocateScratch()
 GPRReg ScratchRegisterAllocator::allocateScratchGPR() { return allocateScratch<GPRInfo>(); }
 FPRReg ScratchRegisterAllocator::allocateScratchFPR() { return allocateScratch<FPRInfo>(); }
 
-unsigned ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler& jit)
+unsigned ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler& jit, ExtraStackSpace extraStackSpace)
 {
     if (!didReuseRegisters())
         return 0;
@@ -119,13 +119,13 @@ unsigned ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembl
             registersToSpill.set(reg);
     }
 
-    unsigned extraStackBytesAtTopOfStack = maxFrameExtentForSlowPathCall;
+    unsigned extraStackBytesAtTopOfStack = extraStackSpace == ExtraStackSpace::SpaceForCCall ? maxFrameExtentForSlowPathCall : 0;
     unsigned stackAdjustmentSize = ScratchRegisterAllocator::preserveRegistersToStackForCall(jit, registersToSpill, extraStackBytesAtTopOfStack);
 
     return stackAdjustmentSize;
 }
 
-void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& jit, unsigned numberOfBytesUsedToPreserveReusedRegisters)
+void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& jit, unsigned numberOfBytesUsedToPreserveReusedRegisters, ExtraStackSpace extraStackSpace)
 {
     if (!didReuseRegisters())
         return;
@@ -142,7 +142,7 @@ void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& j
             registersToFill.set(reg);
     }
 
-    unsigned extraStackBytesAtTopOfStack = maxFrameExtentForSlowPathCall;
+    unsigned extraStackBytesAtTopOfStack = extraStackSpace == ExtraStackSpace::SpaceForCCall ? maxFrameExtentForSlowPathCall : 0;
     RegisterSet dontRestore; // Empty set. We want to restore everything.
     ScratchRegisterAllocator::restoreRegistersFromStackForCall(jit, registersToFill, dontRestore, numberOfBytesUsedToPreserveReusedRegisters, extraStackBytesAtTopOfStack);
 }
@@ -150,7 +150,7 @@ void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& j
 RegisterSet ScratchRegisterAllocator::usedRegistersForCall() const
 {
     RegisterSet result = m_usedRegisters;
-    result.exclude(RegisterSet::registersToNotSaveForCall());
+    result.exclude(RegisterSet::registersToNotSaveForJSCall());
     return result;
 }
 
index 37869d8..889cea5 100644 (file)
@@ -66,12 +66,9 @@ public:
 
     RegisterSet usedRegisters() const { return m_usedRegisters; }
     
-    // preserveReusedRegistersByPushing() returns the number of padding bytes used to keep the stack
-    // pointer properly aligned and to reserve room for calling a C helper. This number of padding
-    // bytes must be provided to restoreReusedRegistersByPopping() in order to reverse the work done
-    // by preserveReusedRegistersByPushing().
-    unsigned preserveReusedRegistersByPushing(MacroAssembler& jit);
-    void restoreReusedRegistersByPopping(MacroAssembler& jit, unsigned numberOfBytesUsedToPreserveReusedRegisters);
+    enum class ExtraStackSpace { SpaceForCCall, NoExtraSpace };
+    unsigned preserveReusedRegistersByPushing(MacroAssembler& jit, ExtraStackSpace);
+    void restoreReusedRegistersByPopping(MacroAssembler& jit, unsigned numberOfBytesUsedToPreserveReusedRegisters, ExtraStackSpace);
     
     RegisterSet usedRegistersForCall() const;