CompareEq should be using KnownOtherUse instead of OtherUse
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jul 2018 19:16:29 +0000 (19:16 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 20 Jul 2018 19:16:29 +0000 (19:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186814
<rdar://problem/39720030>

Reviewed by Filip Pizlo.

JSTests:

* stress/compare-eq-should-use-known-other-use.js: Added.
(bar):
(i.func):

Source/JavaScriptCore:

CompareEq in fixup phase was doing this:
insertCheck(child, OtherUse)
setUseKind(child, OtherUse)
And in the DFG/FTL backend, it would not emit a check for OtherUse. This could
lead to edge verification crashing because a phase may optimize the check out
by removing the node. However, AI may not be privy to that optimization, and
AI may think the incoming value may not be Other. AI is expecting the DFG/FTL
backend to actually emit a check here, but it does not.

This exact pattern is why we have KnownXYZ use kinds. This patch introduces
KnownOtherUse and changes the above pattern to be:
insertCheck(child, OtherUse)
setUseKind(child, KnownOtherUse)

* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::SafeToExecuteEdge::operator()):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::speculate):
* dfg/DFGUseKind.cpp:
(WTF::printInternal):
* dfg/DFGUseKind.h:
(JSC::DFG::typeFilterFor):
(JSC::DFG::shouldNotHaveTypeCheck):
(JSC::DFG::checkMayCrashIfInputIsEmpty):
* dfg/DFGWatchpointCollectionPhase.cpp:
(JSC::DFG::WatchpointCollectionPhase::handle):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCompareEq):
(JSC::FTL::DFG::LowerDFGToB3::speculate):

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

JSTests/ChangeLog
JSTests/stress/compare-eq-should-use-known-other-use.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGUseKind.cpp
Source/JavaScriptCore/dfg/DFGUseKind.h
Source/JavaScriptCore/dfg/DFGWatchpointCollectionPhase.cpp
Source/JavaScriptCore/ftl/FTLCapabilities.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 46c8374..a2c16af 100644 (file)
@@ -1,3 +1,15 @@
+2018-07-20  Saam Barati  <sbarati@apple.com>
+
+        CompareEq should be using KnownOtherUse instead of OtherUse
+        https://bugs.webkit.org/show_bug.cgi?id=186814
+        <rdar://problem/39720030>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/compare-eq-should-use-known-other-use.js: Added.
+        (bar):
+        (i.func):
+
 2018-07-20  David Fenton  <david_fenton@apple.com>
 
         stress/spread-forward-varargs-stack-overflow.js is timing out in 32 bit JSC tests.
diff --git a/JSTests/stress/compare-eq-should-use-known-other-use.js b/JSTests/stress/compare-eq-should-use-known-other-use.js
new file mode 100644 (file)
index 0000000..ce5f4a7
--- /dev/null
@@ -0,0 +1,29 @@
+function bar(testArgs) {
+    (() => {
+        try {
+            testArgs.func.call(1);
+        } catch (e) {
+            if (!testArgs.qux) {
+                return e == testArgs.qux;
+            }
+        }
+    })();
+}
+for (var i = 0; i < 100000; i++) {
+    [
+        {
+            func: ()=>{},
+        },
+        {
+            func: Int8Array.prototype.values,
+            foo: 0
+        },
+        {
+            func: Int8Array.prototype.values,
+            qux: 2
+        },
+        {
+            func: Int8Array.prototype.values,
+        },
+    ].forEach(bar);
+}
index 63f76a2..3249d7c 100644 (file)
@@ -1,3 +1,45 @@
+2018-07-20  Saam Barati  <sbarati@apple.com>
+
+        CompareEq should be using KnownOtherUse instead of OtherUse
+        https://bugs.webkit.org/show_bug.cgi?id=186814
+        <rdar://problem/39720030>
+
+        Reviewed by Filip Pizlo.
+
+        CompareEq in fixup phase was doing this:
+        insertCheck(child, OtherUse)
+        setUseKind(child, OtherUse)
+        And in the DFG/FTL backend, it would not emit a check for OtherUse. This could
+        lead to edge verification crashing because a phase may optimize the check out
+        by removing the node. However, AI may not be privy to that optimization, and
+        AI may think the incoming value may not be Other. AI is expecting the DFG/FTL
+        backend to actually emit a check here, but it does not.
+        
+        This exact pattern is why we have KnownXYZ use kinds. This patch introduces
+        KnownOtherUse and changes the above pattern to be:
+        insertCheck(child, OtherUse)
+        setUseKind(child, KnownOtherUse)
+
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGSafeToExecute.h:
+        (JSC::DFG::SafeToExecuteEdge::operator()):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::speculate):
+        * dfg/DFGUseKind.cpp:
+        (WTF::printInternal):
+        * dfg/DFGUseKind.h:
+        (JSC::DFG::typeFilterFor):
+        (JSC::DFG::shouldNotHaveTypeCheck):
+        (JSC::DFG::checkMayCrashIfInputIsEmpty):
+        * dfg/DFGWatchpointCollectionPhase.cpp:
+        (JSC::DFG::WatchpointCollectionPhase::handle):
+        * ftl/FTLCapabilities.cpp:
+        (JSC::FTL::canCompile):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileCompareEq):
+        (JSC::FTL::DFG::LowerDFGToB3::speculate):
+
 2018-07-20  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] A bit performance improvement for Object.assign by cleaning up code
index 9d6ef7a..0949f6f 100644 (file)
@@ -587,21 +587,21 @@ private:
             // If either child can be proved to be Null or Undefined, comparing them is greatly simplified.
             bool oneArgumentIsUsedAsSpecOther = false;
             if (node->child1()->isUndefinedOrNullConstant()) {
-                fixEdge<OtherUse>(node->child1());
+                fixEdge<KnownOtherUse>(node->child1());
                 oneArgumentIsUsedAsSpecOther = true;
             } else if (node->child1()->shouldSpeculateOther()) {
                 m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin,
                     Edge(node->child1().node(), OtherUse));
-                fixEdge<OtherUse>(node->child1());
+                fixEdge<KnownOtherUse>(node->child1());
                 oneArgumentIsUsedAsSpecOther = true;
             }
             if (node->child2()->isUndefinedOrNullConstant()) {
-                fixEdge<OtherUse>(node->child2());
+                fixEdge<KnownOtherUse>(node->child2());
                 oneArgumentIsUsedAsSpecOther = true;
             } else if (node->child2()->shouldSpeculateOther()) {
                 m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin,
                     Edge(node->child2().node(), OtherUse));
-                fixEdge<OtherUse>(node->child2());
+                fixEdge<KnownOtherUse>(node->child2());
                 oneArgumentIsUsedAsSpecOther = true;
             }
             if (oneArgumentIsUsedAsSpecOther) {
index 267c8a1..cf3d755 100644 (file)
@@ -106,6 +106,11 @@ public:
             if (m_state.forNode(edge).m_type & ~(SpecHeapTop & ~SpecObject))
                 m_result = false;
             return;
+
+        case KnownOtherUse:
+            if (m_state.forNode(edge).m_type & ~SpecOther)
+                m_result = false;
+            return;
             
         case LastUseKind:
             RELEASE_ASSERT_NOT_REACHED();
index 7deb898..89af173 100644 (file)
@@ -10290,6 +10290,9 @@ void SpeculativeJIT::speculate(Node*, Edge edge)
     case NotCellUse:
         speculateNotCell(edge);
         break;
+    case KnownOtherUse:
+        ASSERT(!needsTypeCheck(edge, SpecOther));
+        break;
     case OtherUse:
         speculateOther(edge);
         break;
index 84a96ae..c56429c 100644 (file)
@@ -154,6 +154,9 @@ void printInternal(PrintStream& out, UseKind useKind)
     case NotCellUse:
         out.print("NotCell");
         return;
+    case KnownOtherUse:
+        out.print("KnownOther");
+        return;
     case OtherUse:
         out.print("Other");
         return;
index 24dcad8..acd1280 100644 (file)
@@ -75,6 +75,7 @@ enum UseKind {
     NotStringVarUse,
     NotSymbolUse,
     NotCellUse,
+    KnownOtherUse,
     OtherUse,
     MiscUse,
 
@@ -169,6 +170,7 @@ inline SpeculatedType typeFilterFor(UseKind useKind)
         return ~SpecSymbol;
     case NotCellUse:
         return ~SpecCellCheck;
+    case KnownOtherUse:
     case OtherUse:
         return SpecOther;
     case MiscUse:
@@ -188,6 +190,7 @@ inline bool shouldNotHaveTypeCheck(UseKind kind)
     case KnownStringUse:
     case KnownPrimitiveUse:
     case KnownBooleanUse:
+    case KnownOtherUse:
     case Int52RepUse:
     case DoubleRepUse:
         return true;
@@ -313,6 +316,7 @@ inline bool checkMayCrashIfInputIsEmpty(UseKind kind)
     case CellUse:
     case KnownCellUse:
     case CellOrOtherUse:
+    case KnownOtherUse:
     case OtherUse:
     case MiscUse:
     case NotCellUse:
index 8bf6511..3aa76db 100644 (file)
@@ -81,7 +81,7 @@ private:
             if (m_node->isBinaryUseKind(ObjectUse)
                 || (m_node->child1().useKind() == ObjectUse && m_node->child2().useKind() == ObjectOrOtherUse)
                 || (m_node->child1().useKind() == ObjectOrOtherUse && m_node->child2().useKind() == ObjectUse)
-                || (m_node->child1().useKind() == OtherUse || m_node->child2().useKind() == OtherUse))
+                || (m_node->child1().useKind() == KnownOtherUse || m_node->child2().useKind() == KnownOtherUse))
                 handleMasqueradesAsUndefined();
             break;
             
index fcef953..d8a584f 100644 (file)
@@ -447,6 +447,7 @@ CapabilityLevel canCompile(Graph& graph)
                 case DerivedArrayUse:
                 case NotCellUse:
                 case OtherUse:
+                case KnownOtherUse:
                 case MiscUse:
                 case StringIdentUse:
                 case NotStringVarUse:
index d3dc9b9..f4e4048 100644 (file)
@@ -6860,13 +6860,13 @@ private:
             return;
         }
 
-        if (m_node->child1().useKind() == OtherUse) {
+        if (m_node->child1().useKind() == KnownOtherUse) {
             ASSERT(!m_interpreter.needsTypeCheck(m_node->child1(), SpecOther));
             setBoolean(equalNullOrUndefined(m_node->child2(), AllCellsAreFalse, EqualNullOrUndefined, ManualOperandSpeculation));
             return;
         }
 
-        if (m_node->child2().useKind() == OtherUse) {
+        if (m_node->child2().useKind() == KnownOtherUse) {
             ASSERT(!m_interpreter.needsTypeCheck(m_node->child2(), SpecOther));
             setBoolean(equalNullOrUndefined(m_node->child1(), AllCellsAreFalse, EqualNullOrUndefined, ManualOperandSpeculation));
             return;
@@ -14839,6 +14839,7 @@ private:
         case KnownInt32Use:
         case KnownStringUse:
         case KnownPrimitiveUse:
+        case KnownOtherUse:
         case DoubleRepUse:
         case Int52RepUse:
             ASSERT(!m_interpreter.needsTypeCheck(edge));