NewArray in FTLLowerDFGToB3 does not handle speculating on doubles when having a...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jul 2017 05:01:33 +0000 (05:01 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 6 Jul 2017 05:01:33 +0000 (05:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=174188
<rdar://problem/30581423>

Reviewed by Mark Lam.

JSTests:

* stress/new-array-having-a-bad-time-double.js: Added.
(assert):
(foo):

Source/JavaScriptCore:

We were calling lowJSValue(edge) when we were speculating the
edge as double. This isn't allowed. We should have been using
lowDouble.

This patch also adds a new option, called useArrayAllocationProfiling,
which defaults to true. When false, it will make the array allocation
profile not actually sample seen arrays. It'll force the allocation
profile's predicted indexing type to be ArrayWithUndecided. Adding
this option made it trivial to write a test for this bug.

* bytecode/ArrayAllocationProfile.cpp:
(JSC::ArrayAllocationProfile::updateIndexingType):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNewArray):
* runtime/Options.h:

Tools:

* Scripts/run-jsc-stress-tests:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219187 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/new-array-having-a-bad-time-double.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/Options.h
Tools/ChangeLog
Tools/Scripts/run-jsc-stress-tests

index 5eace44..a6f904c 100644 (file)
@@ -1,3 +1,15 @@
+2017-07-05  Saam Barati  <sbarati@apple.com>
+
+        NewArray in FTLLowerDFGToB3 does not handle speculating on doubles when having a bad time
+        https://bugs.webkit.org/show_bug.cgi?id=174188
+        <rdar://problem/30581423>
+
+        Reviewed by Mark Lam.
+
+        * stress/new-array-having-a-bad-time-double.js: Added.
+        (assert):
+        (foo):
+
 2017-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         WTF::StringImpl::copyChars segfaults when built with GCC 7
diff --git a/JSTests/stress/new-array-having-a-bad-time-double.js b/JSTests/stress/new-array-having-a-bad-time-double.js
new file mode 100644 (file)
index 0000000..5ee4f6a
--- /dev/null
@@ -0,0 +1,25 @@
+function assert(b) {
+    if (!b)
+        throw new Error("Bad");
+}
+
+function foo(a, b, c) {
+    let x = a + b + c;
+    return [a, b, c, x];
+}
+noInline(foo);
+
+Object.defineProperty(Object.prototype, "10000", {get() { return 20; }});
+
+for (let i = 0; i < 10000; ++i) {
+    let a = 10.5;
+    let b = 1.1;
+    let c = 1.2;
+    let x = a + b + c;
+    let result = foo(a, b, c);
+    assert(result.length === 4);
+    assert(result[0] === a);
+    assert(result[1] === b);
+    assert(result[2] === c);
+    assert(result[3] === x);
+}
index 4263477..9d66a0a 100644 (file)
@@ -1,3 +1,27 @@
+2017-07-05  Saam Barati  <sbarati@apple.com>
+
+        NewArray in FTLLowerDFGToB3 does not handle speculating on doubles when having a bad time
+        https://bugs.webkit.org/show_bug.cgi?id=174188
+        <rdar://problem/30581423>
+
+        Reviewed by Mark Lam.
+
+        We were calling lowJSValue(edge) when we were speculating the
+        edge as double. This isn't allowed. We should have been using
+        lowDouble.
+        
+        This patch also adds a new option, called useArrayAllocationProfiling,
+        which defaults to true. When false, it will make the array allocation
+        profile not actually sample seen arrays. It'll force the allocation
+        profile's predicted indexing type to be ArrayWithUndecided. Adding
+        this option made it trivial to write a test for this bug.
+
+        * bytecode/ArrayAllocationProfile.cpp:
+        (JSC::ArrayAllocationProfile::updateIndexingType):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileNewArray):
+        * runtime/Options.h:
+
 2017-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         WTF::Thread should have the threads stack bounds.
index 905b5bd..a931521 100644 (file)
@@ -49,7 +49,8 @@ void ArrayAllocationProfile::updateIndexingType()
     JSArray* lastArray = m_lastArray;
     if (!lastArray)
         return;
-    m_currentIndexingType = leastUpperBoundOfIndexingTypes(m_currentIndexingType, lastArray->indexingType());
+    if (LIKELY(Options::useArrayAllocationProfiling()))
+        m_currentIndexingType = leastUpperBoundOfIndexingTypes(m_currentIndexingType, lastArray->indexingType());
     m_lastArray = 0;
 }
 
index a34e862..da6a8a9 100644 (file)
@@ -4648,9 +4648,16 @@ private:
         
         for (unsigned operandIndex = 0; operandIndex < m_node->numChildren(); ++operandIndex) {
             Edge edge = m_graph.varArgChild(m_node, operandIndex);
-            m_out.store64(
-                lowJSValue(edge, ManualOperandSpeculation),
-                m_out.absolute(buffer + operandIndex));
+            LValue valueToStore;
+            switch (m_node->indexingType()) {
+            case ALL_DOUBLE_INDEXING_TYPES:
+                valueToStore = boxDouble(lowDouble(edge));
+                break;
+            default:
+                valueToStore = lowJSValue(edge, ManualOperandSpeculation);
+                break;
+            }
+            m_out.store64(valueToStore, m_out.absolute(buffer + operandIndex));
         }
         
         m_out.storePtr(
index 70d7e1a..a321ffe 100644 (file)
@@ -464,7 +464,8 @@ typedef const char* optionString;
     v(bool, useWebAssemblyFastTLS, true, Normal, "If true, we will try to use fast thread-local storage if available on the current platform.") \
     v(bool, useFastTLSForWasmContext, true, Normal, "If true (and fast TLS is enabled), we will store context in fast TLS. If false, we will pin it to a register.") \
     v(bool, useCallICsForWebAssemblyToJSCalls, true, Normal, "If true, we will use CallLinkInfo to inline cache Wasm to JS calls.") \
-    v(bool, useObjectRestSpread, true, Normal, "If true, we will enable Object Rest/Spread feature.")
+    v(bool, useObjectRestSpread, true, Normal, "If true, we will enable Object Rest/Spread feature.") \
+    v(bool, useArrayAllocationProfiling, true, Normal, "If true, we will use our normal array allocation profiling. If false, the allocation profile will always claim to be undecided.")
 
 
 enum OptionEquivalence {
index 029afff..278b0c7 100644 (file)
@@ -1,3 +1,13 @@
+2017-07-05  Saam Barati  <sbarati@apple.com>
+
+        NewArray in FTLLowerDFGToB3 does not handle speculating on doubles when having a bad time
+        https://bugs.webkit.org/show_bug.cgi?id=174188
+        <rdar://problem/30581423>
+
+        Reviewed by Mark Lam.
+
+        * Scripts/run-jsc-stress-tests:
+
 2017-07-05  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [WTF] Clean up StringStatics.cpp by using LazyNeverDestroyed<> for Atoms
index 8bba5e0..e978b67 100755 (executable)
@@ -859,7 +859,7 @@ def runFTLNoCJIT(*optionalTestSpecificOptions)
 end
 
 def runFTLNoCJITB3O1(*optionalTestSpecificOptions)
-    run("ftl-no-cjit-b3o1", *(FTL_OPTIONS + NO_CJIT_OPTIONS + B3O1_OPTIONS + optionalTestSpecificOptions))
+    run("ftl-no-cjit-b3o1", "--useArrayAllocationProfiling=false", *(FTL_OPTIONS + NO_CJIT_OPTIONS + B3O1_OPTIONS + optionalTestSpecificOptions))
 end
 
 def runFTLNoCJITValidate(*optionalTestSpecificOptions)