<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[189103] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/189103">189103</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2015-08-28 10:52:35 -0700 (Fri, 28 Aug 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>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):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorejitRepatchcpp">trunk/Source/JavaScriptCore/jit/Repatch.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorejitScratchRegisterAllocatorcpp">trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorejitScratchRegisterAllocatorh">trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoretestsstressregress148564js">trunk/Source/JavaScriptCore/tests/stress/regress-148564.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (189102 => 189103)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-08-28 17:52:35 UTC (rev 189103)
</span><span class="lines">@@ -1,3 +1,48 @@
</span><ins>+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):
+
</ins><span class="cx"> 2015-08-28 Csaba Osztrogonác <ossy@webkit.org>
</span><span class="cx">
</span><span class="cx"> Unreviewed Windows buildfix.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitRepatchcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/Repatch.cpp (189102 => 189103)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/Repatch.cpp 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/JavaScriptCore/jit/Repatch.cpp 2015-08-28 17:52:35 UTC (rev 189103)
</span><span class="lines">@@ -916,7 +916,7 @@
</span><span class="cx">
</span><span class="cx"> CCallHelpers stubJit(vm, exec->codeBlock());
</span><span class="cx">
</span><del>- allocator.preserveReusedRegistersByPushing(stubJit);
</del><ins>+ size_t numberOfPaddingBytes = allocator.preserveReusedRegistersByPushing(stubJit);
</ins><span class="cx">
</span><span class="cx"> MacroAssembler::Jump badStructure = branchStructure(stubJit,
</span><span class="cx"> MacroAssembler::NotEqual,
</span><span class="lines">@@ -945,11 +945,11 @@
</span><span class="cx"> MacroAssembler::Jump failure;
</span><span class="cx">
</span><span class="cx"> if (allocator.didReuseRegisters()) {
</span><del>- allocator.restoreReusedRegistersByPopping(stubJit);
</del><ins>+ allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
</ins><span class="cx"> success = stubJit.jump();
</span><span class="cx">
</span><span class="cx"> badStructure.link(&stubJit);
</span><del>- allocator.restoreReusedRegistersByPopping(stubJit);
</del><ins>+ allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
</ins><span class="cx"> failure = stubJit.jump();
</span><span class="cx"> } else {
</span><span class="cx"> success = stubJit.jump();
</span><span class="lines">@@ -1053,7 +1053,7 @@
</span><span class="cx"> } else
</span><span class="cx"> scratchGPR3 = InvalidGPRReg;
</span><span class="cx">
</span><del>- allocator.preserveReusedRegistersByPushing(stubJit);
</del><ins>+ size_t numberOfPaddingBytes = allocator.preserveReusedRegistersByPushing(stubJit);
</ins><span class="cx">
</span><span class="cx"> MacroAssembler::JumpList failureCases;
</span><span class="cx">
</span><span class="lines">@@ -1169,11 +1169,11 @@
</span><span class="cx"> MacroAssembler::Jump failure;
</span><span class="cx">
</span><span class="cx"> if (allocator.didReuseRegisters()) {
</span><del>- allocator.restoreReusedRegistersByPopping(stubJit);
</del><ins>+ allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
</ins><span class="cx"> success = stubJit.jump();
</span><span class="cx">
</span><span class="cx"> failureCases.link(&stubJit);
</span><del>- allocator.restoreReusedRegistersByPopping(stubJit);
</del><ins>+ allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
</ins><span class="cx"> failure = stubJit.jump();
</span><span class="cx"> } else
</span><span class="cx"> success = stubJit.jump();
</span><span class="lines">@@ -1184,7 +1184,7 @@
</span><span class="cx"> if (structure->outOfLineCapacity() != oldStructure->outOfLineCapacity()) {
</span><span class="cx"> slowPath.link(&stubJit);
</span><span class="cx">
</span><del>- allocator.restoreReusedRegistersByPopping(stubJit);
</del><ins>+ allocator.restoreReusedRegistersByPopping(stubJit, numberOfPaddingBytes);
</ins><span class="cx"> if (!scratchBuffer)
</span><span class="cx"> scratchBuffer = vm->scratchBufferForSize(allocator.desiredScratchBufferSizeForCall());
</span><span class="cx"> allocator.preserveUsedRegistersToScratchBufferForCall(stubJit, scratchBuffer, scratchGPR1);
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitScratchRegisterAllocatorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp (189102 => 189103)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.cpp 2015-08-28 17:52:35 UTC (rev 189103)
</span><span class="lines">@@ -29,6 +29,7 @@
</span><span class="cx"> #if ENABLE(JIT)
</span><span class="cx">
</span><span class="cx"> #include "JSCInlines.h"
</span><ins>+#include "MaxFrameExtentForSlowPathCall.h"
</ins><span class="cx"> #include "VM.h"
</span><span class="cx">
</span><span class="cx"> namespace JSC {
</span><span class="lines">@@ -91,28 +92,44 @@
</span><span class="cx"> GPRReg ScratchRegisterAllocator::allocateScratchGPR() { return allocateScratch<GPRInfo>(); }
</span><span class="cx"> FPRReg ScratchRegisterAllocator::allocateScratchFPR() { return allocateScratch<FPRInfo>(); }
</span><span class="cx">
</span><del>-void ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler& jit)
</del><ins>+size_t ScratchRegisterAllocator::preserveReusedRegistersByPushing(MacroAssembler& jit)
</ins><span class="cx"> {
</span><span class="cx"> if (!didReuseRegisters())
</span><del>- return;
-
</del><ins>+ return 0;
+
+ size_t numberOfBytesPushed = 0;
+
</ins><span class="cx"> for (unsigned i = 0; i < FPRInfo::numberOfRegisters; ++i) {
</span><span class="cx"> FPRReg reg = FPRInfo::toRegister(i);
</span><del>- if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.get(reg))
</del><ins>+ if (m_scratchRegisters.getFPRByIndex(i) && m_usedRegisters.get(reg)) {
</ins><span class="cx"> jit.pushToSave(reg);
</span><ins>+ numberOfBytesPushed += sizeof(double);
+ }
</ins><span class="cx"> }
</span><span class="cx"> for (unsigned i = 0; i < GPRInfo::numberOfRegisters; ++i) {
</span><span class="cx"> GPRReg reg = GPRInfo::toRegister(i);
</span><del>- if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg))
</del><ins>+ if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg)) {
</ins><span class="cx"> jit.pushToSave(reg);
</span><ins>+ numberOfBytesPushed += sizeof(uintptr_t);
+ }
</ins><span class="cx"> }
</span><ins>+
+ size_t totalStackAdjustmentBytes = numberOfBytesPushed + maxFrameExtentForSlowPathCall;
+ totalStackAdjustmentBytes = WTF::roundUpToMultipleOf(stackAlignmentBytes(), totalStackAdjustmentBytes);
+
+ size_t numberOfPaddingBytes = totalStackAdjustmentBytes - numberOfBytesPushed;
+ jit.subPtr(MacroAssembler::TrustedImm32(numberOfPaddingBytes), MacroAssembler::stackPointerRegister);
+
+ return numberOfPaddingBytes;
</ins><span class="cx"> }
</span><span class="cx">
</span><del>-void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& jit)
</del><ins>+void ScratchRegisterAllocator::restoreReusedRegistersByPopping(MacroAssembler& jit, size_t numberOfPaddingBytes)
</ins><span class="cx"> {
</span><span class="cx"> if (!didReuseRegisters())
</span><span class="cx"> return;
</span><del>-
</del><ins>+
+ jit.addPtr(MacroAssembler::TrustedImm32(numberOfPaddingBytes), MacroAssembler::stackPointerRegister);
+
</ins><span class="cx"> for (unsigned i = GPRInfo::numberOfRegisters; i--;) {
</span><span class="cx"> GPRReg reg = GPRInfo::toRegister(i);
</span><span class="cx"> if (m_scratchRegisters.getGPRByIndex(i) && m_usedRegisters.get(reg))
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitScratchRegisterAllocatorh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.h (189102 => 189103)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.h 2015-08-28 17:35:07 UTC (rev 189102)
+++ trunk/Source/JavaScriptCore/jit/ScratchRegisterAllocator.h 2015-08-28 17:52:35 UTC (rev 189103)
</span><span class="lines">@@ -62,8 +62,12 @@
</span><span class="cx"> return m_numberOfReusedRegisters;
</span><span class="cx"> }
</span><span class="cx">
</span><del>- void preserveReusedRegistersByPushing(MacroAssembler& jit);
- void restoreReusedRegistersByPopping(MacroAssembler& jit);
</del><ins>+ // 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);
</ins><span class="cx">
</span><span class="cx"> RegisterSet usedRegistersForCall() const;
</span><span class="cx">
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstressregress148564js"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/regress-148564.js (0 => 189103)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/regress-148564.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/regress-148564.js 2015-08-28 17:52:35 UTC (rev 189103)
</span><span class="lines">@@ -0,0 +1,72 @@
</span><ins>+//@ 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();
</ins></span></pre>
</div>
</div>
</body>
</html>