<!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>[209673] trunk</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/209673">209673</a></dd>
<dt>Author</dt> <dd>msaboff@apple.com</dd>
<dt>Date</dt> <dd>2016-12-10 13:04:05 -0800 (Sat, 10 Dec 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>REGRESSION(<a href="http://trac.webkit.org/projects/webkit/changeset/209653">r209653</a>) Crash in CallFrameShuffler::snapshot()
https://bugs.webkit.org/show_bug.cgi?id=165728
Reviewed by Filip Pizlo.
JSTests:
New regression test.
* stress/regress-165728.js: Added.
(sum1):
(sum2):
(tailCaller):
(test):
Source/JavaScriptCore:
It can be the case that a JSValueReg's CachedRecovery is the source for mutliple
GPRs. We only store the CachedRecovery in one slot of m_newRegisters to simplify
the recovery process. This is also done for the case where the recovery source
and destination are the same GPR.
In light of this change, snapshot needs to be taught that one CacheRecovery is
the source for multiple registers. This is done by using a two step process.
First find all the argument CachedRecovery's and create a vector mapping all of
the target GPRs and the source recovery. Then use that vector to get the
recovery for each register.
* jit/CallFrameShuffler.h:
(JSC::CallFrameShuffler::snapshot):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkJSTestsChangeLog">trunk/JSTests/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorejitCallFrameShufflerh">trunk/Source/JavaScriptCore/jit/CallFrameShuffler.h</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstressregress165728js">trunk/JSTests/stress/regress-165728.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (209672 => 209673)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog 2016-12-10 20:44:50 UTC (rev 209672)
+++ trunk/JSTests/ChangeLog 2016-12-10 21:04:05 UTC (rev 209673)
</span><span class="lines">@@ -1,3 +1,18 @@
</span><ins>+2016-12-10 Michael Saboff <msaboff@apple.com>
+
+ REGRESSION(r209653) Crash in CallFrameShuffler::snapshot()
+ https://bugs.webkit.org/show_bug.cgi?id=165728
+
+ Reviewed by Filip Pizlo.
+
+ New regression test.
+
+ * stress/regress-165728.js: Added.
+ (sum1):
+ (sum2):
+ (tailCaller):
+ (test):
+
</ins><span class="cx"> 2016-12-10 Keith Miller <keith_miller@apple.com>
</span><span class="cx">
</span><span class="cx"> Fix indirect_call if the result type is used.
</span></span></pre></div>
<a id="trunkJSTestsstressregress165728js"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/regress-165728.js (0 => 209673)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/regress-165728.js (rev 0)
+++ trunk/JSTests/stress/regress-165728.js 2016-12-10 21:04:05 UTC (rev 209673)
</span><span class="lines">@@ -0,0 +1,36 @@
</span><ins>+// Test for REGRESSION(r209653) Crash in CallFrameShuffler::snapshot() - This test shouldn't crash.
+"use strict";
+
+function sum1(a, b, c)
+{
+ return a + b + c;
+}
+
+noInline(sum1);
+
+function sum2(a, b, c)
+{
+ return a + b + c;
+}
+
+noInline(sum2);
+
+function tailCaller(f, a, b)
+{
+ return f(a, b, a)
+}
+
+noInline(tailCaller);
+
+function test(a, b)
+{
+ let result = 0;
+ for (let i = 0; i < 4000000; i++)
+ result = tailCaller(i % 2 ? sum1 : sum2, a, b);
+
+ return result;
+}
+
+let result = test(20, 2);
+if (result != 42)
+ throw "Unexpected result: " + result;
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (209672 => 209673)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-12-10 20:44:50 UTC (rev 209672)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-12-10 21:04:05 UTC (rev 209673)
</span><span class="lines">@@ -1,3 +1,24 @@
</span><ins>+2016-12-10 Michael Saboff <msaboff@apple.com>
+
+ REGRESSION(r209653) Crash in CallFrameShuffler::snapshot()
+ https://bugs.webkit.org/show_bug.cgi?id=165728
+
+ Reviewed by Filip Pizlo.
+
+ It can be the case that a JSValueReg's CachedRecovery is the source for mutliple
+ GPRs. We only store the CachedRecovery in one slot of m_newRegisters to simplify
+ the recovery process. This is also done for the case where the recovery source
+ and destination are the same GPR.
+
+ In light of this change, snapshot needs to be taught that one CacheRecovery is
+ the source for multiple registers. This is done by using a two step process.
+ First find all the argument CachedRecovery's and create a vector mapping all of
+ the target GPRs and the source recovery. Then use that vector to get the
+ recovery for each register.
+
+ * jit/CallFrameShuffler.h:
+ (JSC::CallFrameShuffler::snapshot):
+
</ins><span class="cx"> 2016-12-10 Keith Miller <keith_miller@apple.com>
</span><span class="cx">
</span><span class="cx"> Fix indirect_call if the result type is used.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitCallFrameShufflerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/CallFrameShuffler.h (209672 => 209673)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/CallFrameShuffler.h 2016-12-10 20:44:50 UTC (rev 209672)
+++ trunk/Source/JavaScriptCore/jit/CallFrameShuffler.h 2016-12-10 21:04:05 UTC (rev 209673)
</span><span class="lines">@@ -113,13 +113,33 @@
</span><span class="cx"> data.callee = cachedRecovery->recovery();
</span><span class="cx"> }
</span><span class="cx"> data.args.resize(argCount());
</span><ins>+
+ Vector<ValueRecovery> registerArgRecoveries;
+#if USE(JSVALUE64)
+ // Find cached recoveries for all argument registers.
+ // We do this here, because a cached recovery may be the source for multiple
+ // argument registers, but it is only stored in one m_newRegister index.
+ if (data.argumentsInRegisters) {
+ unsigned maxArgumentRegister = std::min(static_cast<unsigned>(argCount()), NUMBER_OF_JS_FUNCTION_ARGUMENT_REGISTERS);
+ registerArgRecoveries.resize(maxArgumentRegister);
+ for (size_t i = 0; i < maxArgumentRegister; ++i) {
+ Reg reg { argumentRegisterForFunctionArgument(i) };
+ CachedRecovery* cachedRecovery { m_newRegisters[reg] };
+ if (cachedRecovery) {
+ for (auto jsValueReg : cachedRecovery->gprTargets())
+ registerArgRecoveries[jsFunctionArgumentForArgumentRegister(jsValueReg.gpr())] = cachedRecovery->recovery();
+ }
+ }
+ }
+#endif
+
</ins><span class="cx"> for (size_t i = 0; i < argCount(); ++i) {
</span><span class="cx"> if (argumentsLocation == StackArgs || i >= NUMBER_OF_JS_FUNCTION_ARGUMENT_REGISTERS)
</span><span class="cx"> data.args[i] = getNew(virtualRegisterForArgument(i))->recovery();
</span><span class="cx"> else {
</span><span class="cx"> Reg reg { argumentRegisterForFunctionArgument(i) };
</span><del>- CachedRecovery* cachedRecovery { m_newRegisters[reg] };
- data.args[i] = cachedRecovery->recovery();
</del><ins>+ ASSERT(registerArgRecoveries[i]);
+ data.args[i] = registerArgRecoveries[i];
</ins><span class="cx"> }
</span><span class="cx"> }
</span><span class="cx"> for (Reg reg = Reg::first(); reg <= Reg::last(); reg = reg.next()) {
</span><span class="lines">@@ -440,15 +460,22 @@
</span><span class="cx"> bool tryAcquireTagTypeNumber();
</span><span class="cx"> #endif
</span><span class="cx">
</span><del>- // This stores, for each register, information about the recovery
- // for the value that should eventually go into that register. The
- // only registers that have a target recovery will be callee-save
- // registers, as well as possibly one JSValueRegs for holding the
- // callee.
</del><ins>+ // This stores information about the recovery for the value that
+ // should eventually go into that register. In some cases there
+ // are recoveries that have multiple targets. For those recoveries,
+ // only the first target register in the map has the recovery.
+ // We optimize the case where there are multiple targets for one
+ // recovery where one of those targets is also the source register.
+ // Restoring the first target becomes a nop and simplifies the logic
+ // of restoring the remaining targets.
</ins><span class="cx"> //
</span><span class="cx"> // Once the correct value has been put into the registers, and
</span><span class="cx"> // contrary to what we do with m_newFrame, we keep the entry in
</span><span class="cx"> // m_newRegisters to simplify spilling.
</span><ins>+ //
+ // If a recovery has multiple target registers, we copy the value
+ // from the first target register to the remaining target registers
+ // at the end of the shuffling process.
</ins><span class="cx"> RegisterMap<CachedRecovery*> m_newRegisters;
</span><span class="cx">
</span><span class="cx"> template<typename CheckFunctor>
</span></span></pre>
</div>
</div>
</body>
</html>