Fixup uses KnownInt32 incorrectly in some nodes
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Mar 2019 19:27:28 +0000 (19:27 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Mar 2019 19:27:28 +0000 (19:27 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195279
<rdar://problem/47915654>

Reviewed by Yusuke Suzuki.

JSTests:

* stress/known-int32-cant-be-used-across-bytecode-boundary.js: Added.
(foo):

Source/JavaScriptCore:

Fixup was sometimes using KnownInt32 edges when it knew some
incoming value is an Int32 based on what the bytecode would return.
However, because bytecode may result in Int32 for some node does
not mean we'll pick Int32 as the value format for that local. For example,
we may choose for a value to be represented as a double. This patch
corrects such uses of KnownInt32.

* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileArrayPush):
(JSC::DFG::SpeculativeJIT::compileGetDirectPname):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileArrayPush):

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

JSTests/ChangeLog
JSTests/stress/known-int32-cant-be-used-across-bytecode-boundary.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 434bebd..4175f02 100644 (file)
@@ -1,3 +1,14 @@
+2019-03-14  Saam barati  <sbarati@apple.com>
+
+        Fixup uses KnownInt32 incorrectly in some nodes
+        https://bugs.webkit.org/show_bug.cgi?id=195279
+        <rdar://problem/47915654>
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/known-int32-cant-be-used-across-bytecode-boundary.js: Added.
+        (foo):
+
 2019-03-14  Keith Miller  <keith_miller@apple.com>
 
         DFG liveness can't skip tail caller inline frames
diff --git a/JSTests/stress/known-int32-cant-be-used-across-bytecode-boundary.js b/JSTests/stress/known-int32-cant-be-used-across-bytecode-boundary.js
new file mode 100644 (file)
index 0000000..fc0c4ab
--- /dev/null
@@ -0,0 +1,26 @@
+//@ runDefault("--useConcurrentJIT=0", "--useMaximalFlushInsertionPhase=1")
+
+function foo() {
+    var x0, x1, x2, x3, x4, x5, x6, x7, x8, x9, x10, x11, x12, x13;
+    var copy = [];
+    var value = this[1];
+
+    for (var p in this)
+        copy[copy.length] = value;
+
+    for (var i = 0; i < 1000; i++) {
+        for (var j = 0; j < 1; j++) {
+        }
+        Math.min(0 ** []);
+    }
+};
+
+noInline(foo);
+
+let array0 = new Array(3).fill(1);
+delete array0[0];
+
+([])[1000] = 0xFFFFF;
+
+for (var i = 0; i < 100; i++)
+    foo.call(array0);
index 41a35d2..96e8607 100644 (file)
@@ -1,3 +1,27 @@
+2019-03-14  Saam barati  <sbarati@apple.com>
+
+        Fixup uses KnownInt32 incorrectly in some nodes
+        https://bugs.webkit.org/show_bug.cgi?id=195279
+        <rdar://problem/47915654>
+
+        Reviewed by Yusuke Suzuki.
+
+        Fixup was sometimes using KnownInt32 edges when it knew some
+        incoming value is an Int32 based on what the bytecode would return.
+        However, because bytecode may result in Int32 for some node does
+        not mean we'll pick Int32 as the value format for that local. For example,
+        we may choose for a value to be represented as a double. This patch
+        corrects such uses of KnownInt32.
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileArrayPush):
+        (JSC::DFG::SpeculativeJIT::compileGetDirectPname):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileArrayPush):
+
 2019-03-14  Keith Miller  <keith_miller@apple.com>
 
         DFG liveness can't skip tail caller inline frames
index af87346..d397e85 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -661,6 +661,12 @@ private:
                     if (!m_candidates.contains(node))
                         break;
 
+                    ASSERT(node->origin.exitOK);
+                    ASSERT(node->child1().useKind() == Int32Use);
+                    insertionSet.insertNode(
+                        nodeIndex, SpecNone, Check, node->origin,
+                        node->child1()); 
+
                     node->setOpAndDefaultFlags(PhantomCreateRest);
                     // We don't need this parameter for OSR exit, we can find out all the information
                     // we need via the static parameter count and the dynamic argument count.
index f6448eb..f5b9889 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2019 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -1149,12 +1149,10 @@ private:
                 Edge& element = m_graph.varArgChild(node, i + elementOffset);
                 switch (node->arrayMode().type()) {
                 case Array::Int32:
-                    insertCheck<Int32Use>(element.node());
-                    fixEdge<KnownInt32Use>(element);
+                    fixEdge<Int32Use>(element);
                     break;
                 case Array::Double:
-                    insertCheck<DoubleRepRealUse>(element.node());
-                    fixEdge<DoubleRepUse>(element);
+                    fixEdge<DoubleRepRealUse>(element);
                     break;
                 case Array::Contiguous:
                 case Array::ArrayStorage:
@@ -1163,7 +1161,6 @@ private:
                 default:
                     break;
                 }
-                ASSERT(shouldNotHaveTypeCheck(element.useKind()));
             }
             break;
         }
@@ -1868,7 +1865,7 @@ private:
             
             blessArrayOperation(m_graph.varArgChild(node, 0), m_graph.varArgChild(node, 1), m_graph.varArgChild(node, 2));
             fixEdge<CellUse>(m_graph.varArgChild(node, 0));
-            fixEdge<KnownInt32Use>(m_graph.varArgChild(node, 1));
+            fixEdge<Int32Use>(m_graph.varArgChild(node, 1));
             break;
         }
         case GetDirectPname: {
@@ -1878,7 +1875,7 @@ private:
             Edge& enumerator = m_graph.varArgChild(node, 3);
             fixEdge<CellUse>(base);
             fixEdge<KnownCellUse>(property);
-            fixEdge<KnownInt32Use>(index);
+            fixEdge<Int32Use>(index);
             fixEdge<KnownCellUse>(enumerator);
             break;
         }
@@ -1889,16 +1886,16 @@ private:
         }
         case GetEnumeratorStructurePname: {
             fixEdge<KnownCellUse>(node->child1());
-            fixEdge<KnownInt32Use>(node->child2());
+            fixEdge<Int32Use>(node->child2());
             break;
         }
         case GetEnumeratorGenericPname: {
             fixEdge<KnownCellUse>(node->child1());
-            fixEdge<KnownInt32Use>(node->child2());
+            fixEdge<Int32Use>(node->child2());
             break;
         }
         case ToIndexString: {
-            fixEdge<KnownInt32Use>(node->child1());
+            fixEdge<Int32Use>(node->child1());
             break;
         }
         case ProfileType: {
@@ -1990,7 +1987,7 @@ private:
 
         case CreateRest: {
             watchHavingABadTime(node);
-            fixEdge<KnownInt32Use>(node->child1());
+            fixEdge<Int32Use>(node->child1());
             break;
         }
 
@@ -2154,7 +2151,7 @@ private:
             else
                 fixEdge<UntypedUse>(propertyEdge);
             fixEdge<UntypedUse>(m_graph.varArgChild(node, 2));
-            fixEdge<KnownInt32Use>(m_graph.varArgChild(node, 3));
+            fixEdge<Int32Use>(m_graph.varArgChild(node, 3));
             break;
         }
 
@@ -2204,7 +2201,7 @@ private:
                 fixEdge<UntypedUse>(propertyEdge);
             fixEdge<CellUse>(m_graph.varArgChild(node, 2));
             fixEdge<CellUse>(m_graph.varArgChild(node, 3));
-            fixEdge<KnownInt32Use>(m_graph.varArgChild(node, 4));
+            fixEdge<Int32Use>(m_graph.varArgChild(node, 4));
             break;
         }
 
index 98cc188..12fb510 100644 (file)
@@ -8570,12 +8570,13 @@ void SpeculativeJIT::compileArrayPush(Node* node)
     case Array::Contiguous: {
         if (elementCount == 1) {
             Edge& element = m_jit.graph().varArgChild(node, elementOffset);
+            if (node->arrayMode().type() == Array::Int32) {
+                ASSERT(element.useKind() == Int32Use);
+                speculateInt32(element);
+            }
             JSValueOperand value(this, element, ManualOperandSpeculation);
             JSValueRegs valueRegs = value.jsValueRegs();
 
-            if (node->arrayMode().type() == Array::Int32)
-                DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecInt32Only));
-
             m_jit.load32(MacroAssembler::Address(storageGPR, Butterfly::offsetOfPublicLength()), storageLengthGPR);
             MacroAssembler::Jump slowPath = m_jit.branch32(MacroAssembler::AboveOrEqual, storageLengthGPR, MacroAssembler::Address(storageGPR, Butterfly::offsetOfVectorLength()));
             m_jit.storeValue(valueRegs, MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
@@ -8590,6 +8591,14 @@ void SpeculativeJIT::compileArrayPush(Node* node)
             return;
         }
 
+        if (node->arrayMode().type() == Array::Int32) {
+            for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
+                Edge element = m_jit.graph().varArgChild(node, elementIndex + elementOffset);
+                ASSERT(element.useKind() == Int32Use);
+                speculateInt32(element);
+            }
+        }
+
         GPRTemporary buffer(this);
         GPRReg bufferGPR = buffer.gpr();
 
@@ -8615,12 +8624,9 @@ void SpeculativeJIT::compileArrayPush(Node* node)
         storageDone.link(&m_jit);
         for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
             Edge& element = m_jit.graph().varArgChild(node, elementIndex + elementOffset);
-            JSValueOperand value(this, element, ManualOperandSpeculation);
+            JSValueOperand value(this, element, ManualOperandSpeculation); // We did type checks above.
             JSValueRegs valueRegs = value.jsValueRegs();
 
-            if (node->arrayMode().type() == Array::Int32)
-                DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecInt32Only));
-
             m_jit.storeValue(valueRegs, MacroAssembler::Address(bufferGPR, sizeof(EncodedJSValue) * elementIndex));
             value.use();
         }
@@ -8643,11 +8649,10 @@ void SpeculativeJIT::compileArrayPush(Node* node)
     case Array::Double: {
         if (elementCount == 1) {
             Edge& element = m_jit.graph().varArgChild(node, elementOffset);
+            speculate(node, element);
             SpeculateDoubleOperand value(this, element);
             FPRReg valueFPR = value.fpr();
 
-            DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecDoubleReal));
-
             m_jit.load32(MacroAssembler::Address(storageGPR, Butterfly::offsetOfPublicLength()), storageLengthGPR);
             MacroAssembler::Jump slowPath = m_jit.branch32(MacroAssembler::AboveOrEqual, storageLengthGPR, MacroAssembler::Address(storageGPR, Butterfly::offsetOfVectorLength()));
             m_jit.storeDouble(valueFPR, MacroAssembler::BaseIndex(storageGPR, storageLengthGPR, MacroAssembler::TimesEight));
@@ -8662,6 +8667,12 @@ void SpeculativeJIT::compileArrayPush(Node* node)
             return;
         }
 
+        for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
+            Edge element = m_jit.graph().varArgChild(node, elementIndex + elementOffset);
+            ASSERT(element.useKind() == DoubleRepRealUse);
+            speculate(node, element);
+        }
+
         GPRTemporary buffer(this);
         GPRReg bufferGPR = buffer.gpr();
 
@@ -8690,8 +8701,6 @@ void SpeculativeJIT::compileArrayPush(Node* node)
             SpeculateDoubleOperand value(this, element);
             FPRReg valueFPR = value.fpr();
 
-            DFG_ASSERT(m_jit.graph(), node, !needsTypeCheck(element, SpecDoubleReal));
-
             m_jit.storeDouble(valueFPR, MacroAssembler::Address(bufferGPR, sizeof(double) * elementIndex));
             value.use();
         }
@@ -13086,6 +13095,7 @@ void SpeculativeJIT::compileGetDirectPname(Node* node)
 {
     Edge& baseEdge = m_jit.graph().varArgChild(node, 0);
     Edge& propertyEdge = m_jit.graph().varArgChild(node, 1);
+    Edge& indexEdge = m_jit.graph().varArgChild(node, 2);
 
     SpeculateCellOperand base(this, baseEdge);
     SpeculateCellOperand property(this, propertyEdge);
@@ -13094,6 +13104,7 @@ void SpeculativeJIT::compileGetDirectPname(Node* node)
 
 #if CPU(X86)
     // Not enough registers on X86 for this code, so always use the slow path.
+    speculate(node, indexEdge);
     flushRegisters();
     JSValueRegsFlushedCallResult result(this);
     JSValueRegs resultRegs = result.regs();
@@ -13101,7 +13112,6 @@ void SpeculativeJIT::compileGetDirectPname(Node* node)
     m_jit.exceptionCheck();
     jsValueResult(resultRegs, node);
 #else
-    Edge& indexEdge = m_jit.graph().varArgChild(node, 2);
     Edge& enumeratorEdge = m_jit.graph().varArgChild(node, 3);
     SpeculateStrictInt32Operand index(this, indexEdge);
     SpeculateCellOperand enumerator(this, enumeratorEdge);
index 64a35a6..7514d2b 100644 (file)
@@ -4674,14 +4674,12 @@ private:
                 Output::StoreType storeType;
                 
                 Edge& element = m_graph.varArgChild(m_node, elementOffset);
+                speculate(element);
                 if (m_node->arrayMode().type() != Array::Double) {
                     value = lowJSValue(element, ManualOperandSpeculation);
-                    if (m_node->arrayMode().type() == Array::Int32)
-                        DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecInt32Only));
                     storeType = Output::Store64;
                 } else {
                     value = lowDouble(element);
-                    DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecDoubleReal));
                     storeType = Output::StoreDouble;
                 }
 
@@ -4720,6 +4718,11 @@ private:
                 return;
             }
 
+            for (unsigned elementIndex = 0; elementIndex < elementCount; ++elementIndex) {
+                Edge element = m_graph.varArgChild(m_node, elementIndex + elementOffset);
+                speculate(element);
+            }
+
             LValue prevLength = m_out.load32(storage, m_heaps.Butterfly_publicLength);
             LValue newLength = m_out.add(prevLength, m_out.constInt32(elementCount));
 
@@ -4756,12 +4759,9 @@ private:
                 Output::StoreType storeType;
                 if (m_node->arrayMode().type() != Array::Double) {
                     value = lowJSValue(element, ManualOperandSpeculation);
-                    if (m_node->arrayMode().type() == Array::Int32)
-                        DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecInt32Only));
                     storeType = Output::Store64;
                 } else {
                     value = lowDouble(element);
-                    DFG_ASSERT(m_graph, m_node, !m_interpreter.needsTypeCheck(element, SpecDoubleReal));
                     storeType = Output::StoreDouble;
                 }