Title: [162992] branches/jsCStack/Source/_javascript_Core
- Revision
- 162992
- Author
- fpi...@apple.com
- Date
- 2014-01-28 18:42:45 -0800 (Tue, 28 Jan 2014)
Log Message
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):
Modified Paths
Added Paths
Diff
Modified: branches/jsCStack/Source/_javascript_Core/ChangeLog (162991 => 162992)
--- branches/jsCStack/Source/_javascript_Core/ChangeLog 2014-01-29 01:44:47 UTC (rev 162991)
+++ branches/jsCStack/Source/_javascript_Core/ChangeLog 2014-01-29 02:42:45 UTC (rev 162992)
@@ -1,5 +1,38 @@
2014-01-28 Filip Pizlo <fpi...@apple.com>
+ 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 <fpi...@apple.com>
+
FTL should support ArrayPush
https://bugs.webkit.org/show_bug.cgi?id=127748
Modified: branches/jsCStack/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (162991 => 162992)
--- branches/jsCStack/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2014-01-29 01:44:47 UTC (rev 162991)
+++ branches/jsCStack/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2014-01-29 02:42:45 UTC (rev 162992)
@@ -3302,7 +3302,7 @@
// FIXME: This would not have to be here if changing the publicLength also zeroed the values between the old
// length and the new length.
m_jit.store64(
- MacroAssembler::TrustedImm64((int64_t)0), MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
+ MacroAssembler::TrustedImm64(bitwise_cast<int64_t>(QNaN)), MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
slowCase = m_jit.branchDouble(MacroAssembler::DoubleNotEqualOrUnordered, tempFPR, tempFPR);
boxDouble(tempFPR, valueGPR);
} else {
Added: branches/jsCStack/Source/_javascript_Core/tests/stress/array-pop-double-hole.js (0 => 162992)
--- branches/jsCStack/Source/_javascript_Core/tests/stress/array-pop-double-hole.js (rev 0)
+++ branches/jsCStack/Source/_javascript_Core/tests/stress/array-pop-double-hole.js 2014-01-29 02:42:45 UTC (rev 162992)
@@ -0,0 +1,14 @@
+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;
+}
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
