- Revision
- 189103
- Author
- mark....@apple.com
- Date
- 2015-08-28 10:52:35 -0700 (Fri, 28 Aug 2015)
Log Message
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):
Modified Paths
Added Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (189102 => 189103)
--- trunk/Source/_javascript_Core/ChangeLog 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-08-28 17:52:35 UTC (rev 189103)
@@ -1,3 +1,48 @@
+2015-08-28 Mark Lam <mark....@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 <o...@webkit.org>
Unreviewed Windows buildfix.
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (189102 => 189103)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2015-08-28 17:52:35 UTC (rev 189103)
@@ -916,7 +916,7 @@
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 @@
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 @@
} else
scratchGPR3 = InvalidGPRReg;
- allocator.preserveReusedRegistersByPushing(stubJit);
+ size_t numberOfPaddingBytes = allocator.preserveReusedRegistersByPushing(stubJit);
MacroAssembler::JumpList failureCases;
@@ -1169,11 +1169,11 @@
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 @@
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);
Modified: trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.cpp (189102 => 189103)
--- trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.cpp 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.cpp 2015-08-28 17:52:35 UTC (rev 189103)
@@ -29,6 +29,7 @@
#if ENABLE(JIT)
#include "JSCInlines.h"
+#include "MaxFrameExtentForSlowPathCall.h"
#include "VM.h"
namespace JSC {
@@ -91,28 +92,44 @@
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))
Modified: trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.h (189102 => 189103)
--- trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.h 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/_javascript_Core/jit/ScratchRegisterAllocator.h 2015-08-28 17:52:35 UTC (rev 189103)
@@ -62,8 +62,12 @@
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;
Added: trunk/Source/_javascript_Core/tests/stress/regress-148564.js (0 => 189103)
--- trunk/Source/_javascript_Core/tests/stress/regress-148564.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/regress-148564.js 2015-08-28 17:52:35 UTC (rev 189103)
@@ -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();
