<!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>[208311] 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/208311">208311</a></dd>
<dt>Author</dt> <dd>sbarati@apple.com</dd>
<dt>Date</dt> <dd>2016-11-02 16:15:33 -0700 (Wed, 02 Nov 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Allocation elimination of rest parameter doesn't take into account indexed properties on Array.prototype/Object.prototype
https://bugs.webkit.org/show_bug.cgi?id=164301
Reviewed by Geoffrey Garen.
JSTests:
* stress/rest-parameter-allocation-elimination-watchpoints-2.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-3.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-4.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-5.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-6.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints.js: Added.
(assert):
(foo):
Source/JavaScriptCore:
We weren't taking into account indexed properties on the __proto__
of the rest parameter. This made the code for doing out of bound
accesses incorrect since it just assumed it's safe for the result of
an out of bound access to be undefined. This broke the semantics
of JS code when there was an indexed property on the Array.prototype
or Object.prototype.
This patch makes sure we set up the proper watchpoints for making
sure out of bound accesses are safe to return undefined.
* dfg/DFGArgumentsEliminationPhase.cpp:</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="#trunkSourceJavaScriptCoredfgDFGArgumentsEliminationPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstressrestparameterallocationeliminationwatchpoints2js">trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js</a></li>
<li><a href="#trunkJSTestsstressrestparameterallocationeliminationwatchpoints3js">trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js</a></li>
<li><a href="#trunkJSTestsstressrestparameterallocationeliminationwatchpoints4js">trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js</a></li>
<li><a href="#trunkJSTestsstressrestparameterallocationeliminationwatchpoints5js">trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js</a></li>
<li><a href="#trunkJSTestsstressrestparameterallocationeliminationwatchpoints6js">trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js</a></li>
<li><a href="#trunkJSTestsstressrestparameterallocationeliminationwatchpointsjs">trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (208310 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog 2016-11-02 23:04:22 UTC (rev 208310)
+++ trunk/JSTests/ChangeLog 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -1,3 +1,29 @@
</span><ins>+2016-11-02 Saam Barati <sbarati@apple.com>
+
+ Allocation elimination of rest parameter doesn't take into account indexed properties on Array.prototype/Object.prototype
+ https://bugs.webkit.org/show_bug.cgi?id=164301
+
+ Reviewed by Geoffrey Garen.
+
+ * stress/rest-parameter-allocation-elimination-watchpoints-2.js: Added.
+ (assert):
+ (foo):
+ * stress/rest-parameter-allocation-elimination-watchpoints-3.js: Added.
+ (assert):
+ (foo):
+ * stress/rest-parameter-allocation-elimination-watchpoints-4.js: Added.
+ (assert):
+ (foo):
+ * stress/rest-parameter-allocation-elimination-watchpoints-5.js: Added.
+ (assert):
+ (foo):
+ * stress/rest-parameter-allocation-elimination-watchpoints-6.js: Added.
+ (assert):
+ (foo):
+ * stress/rest-parameter-allocation-elimination-watchpoints.js: Added.
+ (assert):
+ (foo):
+
</ins><span class="cx"> 2016-11-01 Saam Barati <sbarati@apple.com>
</span><span class="cx">
</span><span class="cx"> We should be able to eliminate rest parameter allocations
</span></span></pre></div>
<a id="trunkJSTestsstressrestparameterallocationeliminationwatchpoints2js"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js (0 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js (rev 0)
+++ trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -0,0 +1,19 @@
</span><ins>+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+ return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+ // Warm it up on in bound accesses.
+ assert(foo(i) === i);
+}
+
+Array.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+ assert(foo() === 50);
</ins></span></pre></div>
<a id="trunkJSTestsstressrestparameterallocationeliminationwatchpoints3js"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js (0 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js (rev 0)
+++ trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -0,0 +1,22 @@
</span><ins>+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+ return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+ // Warm it up on both in bound and out of bound accesses.
+ if (i % 2)
+ assert(foo(i) === i);
+ else
+ assert(foo() === undefined);
+}
+
+Object.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+ assert(foo() === 50);
</ins></span></pre></div>
<a id="trunkJSTestsstressrestparameterallocationeliminationwatchpoints4js"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js (0 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js (rev 0)
+++ trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -0,0 +1,20 @@
</span><ins>+
+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+ return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+ // Warm it up on in bound accesses.
+ assert(foo(i) === i);
+}
+
+Object.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+ assert(foo() === 50);
</ins></span></pre></div>
<a id="trunkJSTestsstressrestparameterallocationeliminationwatchpoints5js"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js (0 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js (rev 0)
+++ trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -0,0 +1,25 @@
</span><ins>+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+ return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+ // Warm it up on both in bound and out of bound accesses.
+ if (i % 2)
+ assert(foo(i) === i);
+ else
+ assert(foo() === undefined);
+}
+
+let newProto = [50];
+newProto.__proto__ = null;
+
+Array.prototype.__proto__ = newProto;
+for (let i = 0; i < 10000; i++)
+ assert(foo() === 50);
</ins></span></pre></div>
<a id="trunkJSTestsstressrestparameterallocationeliminationwatchpoints6js"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js (0 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js (rev 0)
+++ trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -0,0 +1,24 @@
</span><ins>+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+ return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+ // Warm it up on both in bound and out of bound accesses.
+ if (i % 2)
+ assert(foo(i) === i);
+ else
+ assert(foo() === undefined);
+}
+
+let newProto = [50];
+newProto.__proto__ = null;
+Object.prototype.__proto__ = newProto;
+for (let i = 0; i < 10000; i++)
+ assert(foo() === 50);
</ins></span></pre></div>
<a id="trunkJSTestsstressrestparameterallocationeliminationwatchpointsjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js (0 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js (rev 0)
+++ trunk/JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -0,0 +1,22 @@
</span><ins>+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+ return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+ // Warm it up on both in bound and out of bound accesses.
+ if (i % 2)
+ assert(foo(i) === i);
+ else
+ assert(foo() === undefined);
+}
+
+Array.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+ assert(foo() === 50);
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (208310 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-11-02 23:04:22 UTC (rev 208310)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -1,3 +1,22 @@
</span><ins>+2016-11-02 Saam Barati <sbarati@apple.com>
+
+ Allocation elimination of rest parameter doesn't take into account indexed properties on Array.prototype/Object.prototype
+ https://bugs.webkit.org/show_bug.cgi?id=164301
+
+ Reviewed by Geoffrey Garen.
+
+ We weren't taking into account indexed properties on the __proto__
+ of the rest parameter. This made the code for doing out of bound
+ accesses incorrect since it just assumed it's safe for the result of
+ an out of bound access to be undefined. This broke the semantics
+ of JS code when there was an indexed property on the Array.prototype
+ or Object.prototype.
+
+ This patch makes sure we set up the proper watchpoints for making
+ sure out of bound accesses are safe to return undefined.
+
+ * dfg/DFGArgumentsEliminationPhase.cpp:
+
</ins><span class="cx"> 2016-11-02 Geoffrey Garen <ggaren@apple.com>
</span><span class="cx">
</span><span class="cx"> One file per class for CodeBlock.h/.cpp
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGArgumentsEliminationPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp (208310 => 208311)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp 2016-11-02 23:04:22 UTC (rev 208310)
+++ trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp 2016-11-02 23:15:33 UTC (rev 208311)
</span><span class="lines">@@ -28,6 +28,7 @@
</span><span class="cx">
</span><span class="cx"> #if ENABLE(DFG_JIT)
</span><span class="cx">
</span><ins>+#include "ArrayPrototype.h"
</ins><span class="cx"> #include "BytecodeLivenessAnalysisInlines.h"
</span><span class="cx"> #include "ClonedArguments.h"
</span><span class="cx"> #include "DFGArgumentsUtilities.h"
</span><span class="lines">@@ -154,13 +155,25 @@
</span><span class="cx"> if (mode.isInBounds())
</span><span class="cx"> break;
</span><span class="cx">
</span><del>- // If we're out-of-bounds then we proceed only if the object prototype is
- // sane (i.e. doesn't have indexed properties).
</del><ins>+ // If we're out-of-bounds then we proceed only if the prototype chain
+ // for the allocation is sane (i.e. doesn't have indexed properties).
</ins><span class="cx"> JSGlobalObject* globalObject = m_graph.globalObjectFor(edge->origin.semantic);
</span><del>- if (globalObject->objectPrototypeIsSane()) {
- m_graph.watchpoints().addLazily(globalObject->objectPrototype()->structure()->transitionWatchpointSet());
- if (globalObject->objectPrototypeIsSane())
</del><ins>+ InlineWatchpointSet& objectPrototypeTransition = globalObject->objectPrototype()->structure()->transitionWatchpointSet();
+ if (edge->op() == CreateRest) {
+ InlineWatchpointSet& arrayPrototypeTransition = globalObject->arrayPrototype()->structure()->transitionWatchpointSet();
+ if (arrayPrototypeTransition.isStillValid()
+ && objectPrototypeTransition.isStillValid()
+ && globalObject->arrayPrototypeChainIsSane()) {
+ m_graph.watchpoints().addLazily(arrayPrototypeTransition);
+ m_graph.watchpoints().addLazily(objectPrototypeTransition);
</ins><span class="cx"> break;
</span><ins>+ }
+ } else {
+ if (objectPrototypeTransition.isStillValid()
+ && globalObject->objectPrototypeIsSane()) {
+ m_graph.watchpoints().addLazily(objectPrototypeTransition);
+ break;
+ }
</ins><span class="cx"> }
</span><span class="cx"> escape(edge, source);
</span><span class="cx"> break;
</span></span></pre>
</div>
</div>
</body>
</html>