<!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>[162992] branches/jsCStack/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/162992">162992</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2014-01-28 18:42:45 -0800 (Tue, 28 Jan 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>DFG ArrayPop double array mishandles the NaN hole installation
https://bugs.webkit.org/show_bug.cgi?id=127813
Reviewed by Mark Rowe.
Our object model for arrays inferred double dictates that we use quiet NaN (QNaN) to
mark holes. Holes, in this context, are any entries in the allocated array buffer
(i.e. from index 0 up to the vectorLength) that don't currently hold a value. Popping
creates a hole, since it deletes the value at publicLength - 1.
But, because of some sloppy copy-and-paste, we were storing (int64_t)0 when creating
the hole, instead of storing QNaN. That's likely because for other kinds of arrays,
64-bit zero is the hole marker, instead of QNaN.
The attached test case illustrates the problem. In the LLInt and Baseline JIT, the
result returned from foo() is "1.5,2.5,,4.5", since array.pop() removes 3.5 and
replaces it with a hole and then the assignment "array[3] = 4.5" creates an element
just beyond that hole. But, once we tier-up to the DFG, the result previously became
"1.5,2.5,0,4.5", which is wrong. The 0 appeared because the IEEE double
interpretation of 64-bit zero is simply zero.
This patch fixes that problem. Now the DFG agrees with the other engines.
This patch also fixes style. For some reason that copy-pasted code wasn't even
indented correctly.
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* tests/stress/array-pop-double-hole.js: Added.
(foo):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#branchesjsCStackSourceJavaScriptCoreChangeLog">branches/jsCStack/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#branchesjsCStackSourceJavaScriptCoredfgDFGSpeculativeJIT64cpp">branches/jsCStack/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#branchesjsCStackSourceJavaScriptCoretestsstressarraypopdoubleholejs">branches/jsCStack/Source/JavaScriptCore/tests/stress/array-pop-double-hole.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="branchesjsCStackSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: branches/jsCStack/Source/JavaScriptCore/ChangeLog (162991 => 162992)</h4>
<pre class="diff"><span>
<span class="info">--- branches/jsCStack/Source/JavaScriptCore/ChangeLog 2014-01-29 01:44:47 UTC (rev 162991)
+++ branches/jsCStack/Source/JavaScriptCore/ChangeLog 2014-01-29 02:42:45 UTC (rev 162992)
</span><span class="lines">@@ -1,5 +1,38 @@
</span><span class="cx"> 2014-01-28 Filip Pizlo <fpizlo@apple.com>
</span><span class="cx">
</span><ins>+ DFG ArrayPop double array mishandles the NaN hole installation
+ https://bugs.webkit.org/show_bug.cgi?id=127813
+
+ Reviewed by Mark Rowe.
+
+ Our object model for arrays inferred double dictates that we use quiet NaN (QNaN) to
+ mark holes. Holes, in this context, are any entries in the allocated array buffer
+ (i.e. from index 0 up to the vectorLength) that don't currently hold a value. Popping
+ creates a hole, since it deletes the value at publicLength - 1.
+
+ But, because of some sloppy copy-and-paste, we were storing (int64_t)0 when creating
+ the hole, instead of storing QNaN. That's likely because for other kinds of arrays,
+ 64-bit zero is the hole marker, instead of QNaN.
+
+ The attached test case illustrates the problem. In the LLInt and Baseline JIT, the
+ result returned from foo() is "1.5,2.5,,4.5", since array.pop() removes 3.5 and
+ replaces it with a hole and then the assignment "array[3] = 4.5" creates an element
+ just beyond that hole. But, once we tier-up to the DFG, the result previously became
+ "1.5,2.5,0,4.5", which is wrong. The 0 appeared because the IEEE double
+ interpretation of 64-bit zero is simply zero.
+
+ This patch fixes that problem. Now the DFG agrees with the other engines.
+
+ This patch also fixes style. For some reason that copy-pasted code wasn't even
+ indented correctly.
+
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * tests/stress/array-pop-double-hole.js: Added.
+ (foo):
+
+2014-01-28 Filip Pizlo <fpizlo@apple.com>
+
</ins><span class="cx"> FTL should support ArrayPush
</span><span class="cx"> https://bugs.webkit.org/show_bug.cgi?id=127748
</span><span class="cx">
</span></span></pre></div>
<a id="branchesjsCStackSourceJavaScriptCoredfgDFGSpeculativeJIT64cpp"></a>
<div class="modfile"><h4>Modified: branches/jsCStack/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp (162991 => 162992)</h4>
<pre class="diff"><span>
<span class="info">--- branches/jsCStack/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp 2014-01-29 01:44:47 UTC (rev 162991)
+++ branches/jsCStack/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp 2014-01-29 02:42:45 UTC (rev 162992)
</span><span class="lines">@@ -3302,7 +3302,7 @@
</span><span class="cx"> // FIXME: This would not have to be here if changing the publicLength also zeroed the values between the old
</span><span class="cx"> // length and the new length.
</span><span class="cx"> m_jit.store64(
</span><del>- MacroAssembler::TrustedImm64((int64_t)0), MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
</del><ins>+ MacroAssembler::TrustedImm64(bitwise_cast<int64_t>(QNaN)), MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
</ins><span class="cx"> slowCase = m_jit.branchDouble(MacroAssembler::DoubleNotEqualOrUnordered, tempFPR, tempFPR);
</span><span class="cx"> boxDouble(tempFPR, valueGPR);
</span><span class="cx"> } else {
</span></span></pre></div>
<a id="branchesjsCStackSourceJavaScriptCoretestsstressarraypopdoubleholejs"></a>
<div class="addfile"><h4>Added: branches/jsCStack/Source/JavaScriptCore/tests/stress/array-pop-double-hole.js (0 => 162992)</h4>
<pre class="diff"><span>
<span class="info">--- branches/jsCStack/Source/JavaScriptCore/tests/stress/array-pop-double-hole.js (rev 0)
+++ branches/jsCStack/Source/JavaScriptCore/tests/stress/array-pop-double-hole.js 2014-01-29 02:42:45 UTC (rev 162992)
</span><span class="lines">@@ -0,0 +1,14 @@
</span><ins>+function foo() {
+ var array = [1.5, 2, 3.5];
+ array.pop();
+ array[3] = 4.5;
+ return array;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 100000; ++i) {
+ var result = foo();
+ if (result.toString() != "1.5,2,,4.5")
+ throw "Error: bad result: " + result;
+}
</ins></span></pre>
</div>
</div>
</body>
</html>