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.

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: http://svn.webkit.org/repository/webkit/trunk@219187 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Author: [email protected]

JSTests/ChangeLog

@@ -1,3 +1,15 @@
+2017-07-05  Saam Barati  <[email protected]>
+
+        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  <[email protected]>
 
         WTF::StringImpl::copyChars segfaults when built with GCC 7

JSTests/stress/new-array-having-a-bad-time-double.js

@@ -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);
+}

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,27 @@
+2017-07-05  Saam Barati  <[email protected]>
+
+        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  <[email protected]>
 
         WTF::Thread should have the threads stack bounds.

Source/JavaScriptCore/bytecode/ArrayAllocationProfile.cpp

@@ -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;
 }
 

Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -4648,9 +4648,16 @@ class LowerDFGToB3 {
 
         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(

Source/JavaScriptCore/runtime/Options.h

@@ -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 {

Tools/ChangeLog

@@ -1,3 +1,13 @@
+2017-07-05  Saam Barati  <[email protected]>
+
+        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  <[email protected]>
 
         [WTF] Clean up StringStatics.cpp by using LazyNeverDestroyed<> for Atoms

Tools/Scripts/run-jsc-stress-tests

@@ -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)