ScratchRegisterAllocator::preserveReusedRegistersByPushing() should allow room for...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Aug 2015 17:52:35 +0000 (17:52 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 28 Aug 2015 17:52:35 +0000 (17:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=148564

Reviewed by Saam Barati.

ScratchRegisterAllocator::preserveReusedRegistersByPushing() pushes registers on
the stack in order to preserve them.  But emitPutTransitionStub() (which uses
preserveReusedRegistersByPushing()) may also emit a call to a C helper function
to flush the heap write barrier buffer.  The code for emitting a C helper call
expects the stack pointer (sp) to already be pointing to a location on the stack
where there's adequate space reserved for storing the arguments to the C helper,
and that space is expected to be at the top of the stack.  Hence, there is a
conflict of expectations.  As a result, the arguments for the C helper will
overwrite and corrupt the values that are pushed on the stack by
preserveReusedRegistersByPushing().

In addition, JIT compiled functions always position the sp such that it will be
aligned (according to platform ABI dictates) after a C call is made (i.e. after
the frame pointer and return address is pushed on to the stack).
preserveReusedRegistersByPushing()'s arbitrary pushing of a number of saved
register values may mess up this alignment.

The fix is to have preserveReusedRegistersByPushing(), after it has pushed the
saved register values, adjust the sp to reserve an additional amount of stack
space needed for C call helpers plus any padding needed to restore proper sp
alignment.  The stack's ReservedZone will ensure that we have enough stack space
for this.  ScratchRegisterAllocator::restoreReusedRegistersByPopping() also
needs to be updated to perform the complement of this behavior.

* jit/Repatch.cpp:
(JSC::emitPutReplaceStub):
(JSC::emitPutTransitionStub):
* jit/ScratchRegisterAllocator.cpp:
(JSC::ScratchRegisterAllocator::allocateScratchGPR):
(JSC::ScratchRegisterAllocator::allocateScratchFPR):
(JSC::ScratchRegisterAllocator::preserveReusedRegistersByPushing):
(JSC::ScratchRegisterAllocator::restoreReusedRegistersByPopping):
* jit/ScratchRegisterAllocator.h:
(JSC::ScratchRegisterAllocator::numberOfReusedRegisters):
* tests/stress/regress-148564.js: Added.
(test):
(runTest):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/jit/Repatch.cpp
Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp
Source/JavaScriptCore/jit/ScratchRegisterAllocator.h
Source/JavaScriptCore/tests/stress/regress-148564.js [new file with mode: 0644]

index 5f0aa5e..67f7980 100644 (file)
@@ -1,3 +1,48 @@
+2015-08-28  Mark Lam  <mark.lam@apple.com>
+
+        ScratchRegisterAllocator::preserveReusedRegistersByPushing() should allow room for C helper calls and keep sp properly aligned.
+        https://bugs.webkit.org/show_bug.cgi?id=148564
+
+        Reviewed by Saam Barati.
+
+        ScratchRegisterAllocator::preserveReusedRegistersByPushing() pushes registers on
+        the stack in order to preserve them.  But emitPutTransitionStub() (which uses
+        preserveReusedRegistersByPushing()) may also emit a call to a C helper function
+        to flush the heap write barrier buffer.  The code for emitting a C helper call
+        expects the stack pointer (sp) to already be pointing to a location on the stack
+        where there's adequate space reserved for storing the arguments to the C helper,
+        and that space is expected to be at the top of the stack.  Hence, there is a
+        conflict of expectations.  As a result, the arguments for the C helper will
+        overwrite and corrupt the values that are pushed on the stack by
+        preserveReusedRegistersByPushing().
+
+        In addition, JIT compiled functions always position the sp such that it will be
+        aligned (according to platform ABI dictates) after a C call is made (i.e. after
+        the frame pointer and return address is pushed on to the stack).
+        preserveReusedRegistersByPushing()'s arbitrary pushing of a number of saved
+        register values may mess up this alignment.
+
+        The fix is to have preserveReusedRegistersByPushing(), after it has pushed the
+        saved register values, adjust the sp to reserve an additional amount of stack
+        space needed for C call helpers plus any padding needed to restore proper sp
+        alignment.  The stack's ReservedZone will ensure that we have enough stack space
+        for this.  ScratchRegisterAllocator::restoreReusedRegistersByPopping() also
+        needs to be updated to perform the complement of this behavior.
+
+        * jit/Repatch.cpp:
+        (JSC::emitPutReplaceStub):
+        (JSC::emitPutTransitionStub):
+        * jit/ScratchRegisterAllocator.cpp:
+        (JSC::ScratchRegisterAllocator::allocateScratchGPR):
+        (JSC::ScratchRegisterAllocator::allocateScratchFPR):
+        (JSC::ScratchRegisterAllocator::preserveReusedRegistersByPushing):
+        (JSC::ScratchRegisterAllocator::restoreReusedRegistersByPopping):
+        * jit/ScratchRegisterAllocator.h:
+        (JSC::ScratchRegisterAllocator::numberOfReusedRegisters):
+        * tests/stress/regress-148564.js: Added.
+        (test):
+        (runTest):
+
 2015-08-28  Csaba Osztrogon√°c  <ossy@webkit.org>
 
         Unreviewed Windows buildfix.
index 7cf6708..3d7cbb3 100644 (file)
@@ -916,7 +916,7 @@ static bool emitPutReplaceStub(
 
     CCallHelpers stubJit(vm, exec->codeBlock());
 
-    allocator.preserveReusedRegistersByPushing(stubJit);
+    size_t numberOfPaddingBytes = allocator.preserveReusedRegistersByPushing(stubJit);
 
     MacroAssembler::Jump badStructure = branchStructure(stubJit,
         MacroAssembler::NotEqual,
@@ -945,11 +945,11 @@ static bool emitPutReplaceStub(
     MacroAssembler::Jump failure;
     
     if (allocator.didReuseRegisters()) {
-        allocator.restoreReusedRegistersByPopping(stubJit);
+        allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
         success = stubJit.jump();
         
         badStructure.link(&stubJit);
-        allocator.restoreReusedRegistersByPopping(stubJit);
+        allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
         failure = stubJit.jump();
     } else {
         success = stubJit.jump();
@@ -1053,7 +1053,7 @@ static bool emitPutTransitionStub(
     } else
         scratchGPR3 = InvalidGPRReg;
     
-    allocator.preserveReusedRegistersByPushing(stubJit);
+    size_t numberOfPaddingBytes = allocator.preserveReusedRegistersByPushing(stubJit);
 
     MacroAssembler::JumpList failureCases;
             
@@ -1169,11 +1169,11 @@ static bool emitPutTransitionStub(
     MacroAssembler::Jump failure;
             
     if (allocator.didReuseRegisters()) {
-        allocator.restoreReusedRegistersByPopping(stubJit);
+        allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
         success = stubJit.jump();
 
         failureCases.link(&stubJit);
-        allocator.restoreReusedRegistersByPopping(stubJit);
+        allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
         failure = stubJit.jump();
     } else
         success = stubJit.jump();
@@ -1184,7 +1184,7 @@ static bool emitPutTransitionStub(
     if (structure->outOfLineCapacity() != oldStructure->outOfLineCapacity()) {
         slowPath.link(&stubJit);
         
-        allocator.restoreReusedRegistersByPopping(stubJit);
+        allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
         if (!scratchBuffer)
             scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSizeForCall());
         allocator.preserveUsedRegistersToScratchBufferForCall(stubJit, scratchBuffer, scratchGPR1);
index b99f940..a644152 100644 (file)
@@ -29,6 +29,7 @@
 #if ENABLE(JIT)
 
 #include "JSCInlines.h"
+#include "MaxFrameExtentForSlowPathCall.h"
 #include "VM.h"
 
 namespace JSC {
@@ -91,28 +92,44 @@ typename BankInfo::RegisterType ScratchRegisterAllocator::allocateScratch()
 GPRReg ScratchRegisterAllocator::allocateScratchGPR() { return allocateScratch<GPRInfo>(); }
 FPRReg ScratchRegisterAllocator::allocateScratchFPR() { return allocateScratch<FPRInfo>(); }
 
-void ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler& jit)
+size_t ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler& jit)
 {
     if (!didReuseRegisters())
-        return;
-        
+        return 0;
+
+    size_t numberOfBytesPushed = 0;
+
     for (unsigned i = 0; i < FPRInfo::numberOfRegisters; ++i) {
         FPRReg reg = FPRInfo::toRegister(i);
-        if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.get(reg))
+        if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.get(reg)) {
             jit.pushToSave(reg);
+            numberOfBytesPushed += sizeof(double);
+        }
     }
     for (unsigned i = 0; i < GPRInfo::numberOfRegisters; ++i) {
         GPRReg reg = GPRInfo::toRegister(i);
-        if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg))
+        if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg)) {
             jit.pushToSave(reg);
+            numberOfBytesPushed += sizeof(uintptr_t);
+        }
     }
+
+    size_t totalStackAdjustmentBytes = numberOfBytesPushed + maxFrameExtentForSlowPathCall;
+    totalStackAdjustmentBytes = WTF::roundUpToMultipleOf(stackAlignmentBytes(), totalStackAdjustmentBytes);
+
+    size_t numberOfPaddingBytes = totalStackAdjustmentBytes - numberOfBytesPushed;
+    jit.subPtr(MacroAssembler::TrustedImm32(numberOfPaddingBytes), MacroAssembler::stackPointerRegister);
+
+    return numberOfPaddingBytes;
 }
 
-void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& jit)
+void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& jit, size_t numberOfPaddingBytes)
 {
     if (!didReuseRegisters())
         return;
-        
+
+    jit.addPtr(MacroAssembler::TrustedImm32(numberOfPaddingBytes), MacroAssembler::stackPointerRegister);
+
     for (unsigned i = GPRInfo::numberOfRegisters; i--;) {
         GPRReg reg = GPRInfo::toRegister(i);
         if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg))
index f6b94f6..cd5ec9f 100644 (file)
@@ -62,8 +62,12 @@ public:
         return m_numberOfReusedRegisters;
     }
     
-    void preserveReusedRegistersByPushing(MacroAssembler& jit);
-    void restoreReusedRegistersByPopping(MacroAssembler& jit);
+    // 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().
+    size_t preserveReusedRegistersByPushing(MacroAssembler& jit);
+    void restoreReusedRegistersByPopping(MacroAssembler& jit, size_t numberOfPaddingBytes);
     
     RegisterSet usedRegistersForCall() const;
     
diff --git a/Source/JavaScriptCore/tests/stress/regress-148564.js b/Source/JavaScriptCore/tests/stress/regress-148564.js
new file mode 100644 (file)
index 0000000..89edb65
--- /dev/null
@@ -0,0 +1,72 @@
+//@ run("regress", "--enableAccessInlining=false")
+
+// Regression test for https://bugs.webkit.org/show_bug.cgi?id=148542
+//
+// In order to manifest, the bug being tested requires all these conditions to be true:
+// 1. A put operation must not being optimized by the DFG into a PutByOffset.
+//    It needs to be a PutById node instead so that it will use the inline cache.
+//    This is satisfied by using the --enableAccessInlining=false option above.
+//
+// 2. The PutById's execution must go through its transition stub.
+//
+// 3. In the transition stub, the object being put into must require a reallocation of its
+//    storage butterfly. This causes the stub to generate code to save some registers.
+//
+// 4. The transition stub needs to call the slow path for flushing the heap write barrier
+//    buffer.
+//
+// 5. The caller of the test must not be DFG compiled. This was not a strictly needed
+//    condition of the bug, but allowing the caller to compile seems to interfere with
+//    our method below of achieving condition 3.
+//
+// With the bug fixed, this test should not crash.
+
+var val = { a: 5, b: 10 }
+
+function test(obj, val, j, x, y, z) {
+    obj.a = val.a; // PutById after GetById
+    if (val.b)     // GetById to access val in a register again.
+        val.b++;
+}
+
+noInline(test);
+
+function runTest() {
+    for (var j = 0; j < 50; j++) {
+        var objs = [];
+
+        let numberOfObjects = 200;
+        for (var k = 0; k < numberOfObjects; k++) { 
+            var obj = { };
+
+            // Condition 3.
+            // Fuzzing the amount of property storage used so that we can get the
+            // repatch stub generator to resize the object out of line storage, and
+            // create more register pressure to do that work. This in turn causes it to
+            // need to preserve registers on the stack.
+            var numInitialProps = j % 20;
+            for (var i = 0; i < numInitialProps; i++)
+                obj["i" + i] = i;
+
+            objs[k] = obj;
+        }
+
+        // Condition 4.
+        // Put all the objects in the GC's oldGen so that we can exercise the write
+        // barrier when we exercise the PutById.
+        gc();
+
+        for (var k = 0; k < numberOfObjects; k++) {
+            // Condition 2.
+            // Eventually, the IC will converge on the slow path. Need to gc()
+            // periodically to repatch anew.
+            if (k % 97 == 1 && j % 5 == 1)
+                gc();
+
+            test(objs[k], val, j);
+        }
+    }
+}
+
+noDFG(runTest); // Condition 5.
+runTest();