Title: [228742] releases/WebKitGTK/webkit-2.20
Revision
228742
Author
carlo...@webkit.org
Date
2018-02-19 23:35:00 -0800 (Mon, 19 Feb 2018)

Log Message

Merge r228565 - Fix bugs from r228411
https://bugs.webkit.org/show_bug.cgi?id=182851
<rdar://problem/37577732>

Reviewed by JF Bastien.

JSTests:

* stress/constant-folding-phase-insert-check-handle-varargs.js: Added.

Source/_javascript_Core:

There was a bug from r228411 where inside the constant folding phase,
we used an insertCheck method that didn't handle varargs. This would
lead to a crash. When thinking about the fix for that function, I realized
a made a couple of mistakes in r228411. One is probably a security bug, and
the other is a performance bug because it'll prevent CSE for certain flavors
of GetByVal nodes. Both blunders are similar in nature.

In r228411, I added code in LICM that inserted a CheckVarargs node with children
of another varargs node. However, to construct this new node's children,
I just copied the AdjacencyList. This does a shallow copy. What we needed
was a deep copy. We needed to create a new vararg AdjacencyList that points
to edges that are deep copies of the original varargs children. This patch
fixes this goof in LICM.

r228411 made it so that PureValue over a varargs node would just compare actual
AdjacencyLists structs. So, if you had two GetByVals that had equal santized
children, their actual AdjacencyList structs are *not* bitwise equal, since they'll
have different firstChild values. Instead, we need to do a deep compare of their
adjacency lists. This patch teaches PureValue how to do that.

* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGGraph.h:
(JSC::DFG::Graph::copyVarargChildren):
* dfg/DFGInsertionSet.h:
(JSC::DFG::InsertionSet::insertCheck):
* dfg/DFGLICMPhase.cpp:
(JSC::DFG::LICMPhase::attemptHoist):
* dfg/DFGPureValue.cpp:
(JSC::DFG::PureValue::dump const):
* dfg/DFGPureValue.h:
(JSC::DFG::PureValue::PureValue):
(JSC::DFG::PureValue::op const):
(JSC::DFG::PureValue::hash const):
(JSC::DFG::PureValue::operator== const):
(JSC::DFG::PureValue::isVarargs const):
(JSC::DFG::PureValue::children const): Deleted.
* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::handleNode):
(JSC::DFG::StrengthReductionPhase::convertToIdentityOverChild):

Modified Paths

Added Paths

Diff

Modified: releases/WebKitGTK/webkit-2.20/JSTests/ChangeLog (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/JSTests/ChangeLog	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/JSTests/ChangeLog	2018-02-20 07:35:00 UTC (rev 228742)
@@ -1,3 +1,13 @@
+2018-02-16  Saam Barati  <sbar...@apple.com>
+
+        Fix bugs from r228411
+        https://bugs.webkit.org/show_bug.cgi?id=182851
+        <rdar://problem/37577732>
+
+        Reviewed by JF Bastien.
+
+        * stress/constant-folding-phase-insert-check-handle-varargs.js: Added.
+
 2018-02-12  Saam Barati  <sbar...@apple.com>
 
         DFG::emitCodeToGetArgumentsArrayLength needs to handle NewArrayBuffer/PhantomNewArrayBuffer

Added: releases/WebKitGTK/webkit-2.20/JSTests/stress/constant-folding-phase-insert-check-handle-varargs.js (0 => 228742)


--- releases/WebKitGTK/webkit-2.20/JSTests/stress/constant-folding-phase-insert-check-handle-varargs.js	                        (rev 0)
+++ releases/WebKitGTK/webkit-2.20/JSTests/stress/constant-folding-phase-insert-check-handle-varargs.js	2018-02-20 07:35:00 UTC (rev 228742)
@@ -0,0 +1,151 @@
+function __getRandomObject() {
+    for (let obj of __getObjects()) {
+    }
+}
+function __getRandomProperty() {
+    let properties = __getproperties(obj);
+    return properties[seed % properties.length];
+}
+try {
+} catch (e) {
+    this.WScript = new Proxy({}, { });
+}
+(function () { function ValueOf() { switch (value) { } } })(this);
+
+(function (global) { })(this);
+
+function f1() {
+    v = v.map(() => { })
+}
+
+try {
+    f1();
+} catch (e) {}
+
+try {
+    f1();
+} catch (e) {}
+
+try {
+    __getRandomObject(983831)[__getRandomProperty(__getRandomObject(983831))]();
+} catch (e) {}
+
+if (!'next') throw "Error: iterator prototype should have next method.";
+
+function f2() {
+    var v = a[0];
+    if (v.next !== a[randoF2].next) throw "Error: next method is not the same.";
+}
+var a1 = [];
+var sym = a1[Symbol.iterator]();
+try {
+    __callRandomFunction(keys, 457796, keys, __getRandomObject(860068), keys, true, __getRandomObject(32567), -1073741825, null);
+    Object.defineProperty(keys, __getRandomProperty(), { });
+    f2([a1[Symbol.iterator](),
+             keys.keys(), a1.entries()]);
+} catch (e) {}
+
+var set1 = new Set([]);
+var sym = set1[Symbol.iterator]();
+var keys = set1.keys();
+var entries = set1.entries();
+try {
+    f2([set1[Symbol.iterator](), set1.keys(), set1.entries()]);
+} catch (e) {}
+
+var map1 = new Map();
+try {
+    [[][  -10], [][ 2][ 1]].forEach();
+} catch (e) {}
+var sym = map1[Symbol.iterator]();
+var keys = map1.keys();
+var entries = map1.entries();
+try {
+    f2([map1[Symbol.iterator](), map1.keys(), map1.entries()]);
+} catch (e) {}
+
+function f3() {
+    if (vl !== vl) throw new Error('bad value: ' + JSON.stringify(__v_19176));
+}
+
+var arr2 = [];
+function f4() {
+    arr2.push(randov)
+    arr2.push(randov)
+}
+try {
+    f4`Hello`;
+} catch (e) {}
+try {
+    f4`World`;
+    f4`Hello`;
+} catch (e) {}
+try {
+    __callRandomFunction(arr2, 247938, set1, new Boolean(true), __getRandomObject(692620), -1e-15, __getRandomObject(276888));
+} catch (e) {}
+if (set1 != null && typeof set1 == "object") try {
+    Object.defineProperty(set1, __getRandomProperty(), {
+    });
+    f3();
+    f3(arr2[0] !== arr2[1]);
+    f3(arr2[0] !== arr2[2]);
+    f4`Hello\n`;
+    f4`Hello\r`;
+} catch (e) {}
+try {
+    f3(arr2[1] !== arr2[2]);
+} catch (e) {}
+if (a1 != null && typeof a1 == "object") try {
+    Object.defineProperty(a1, __getRandomProperty(), {
+    });
+} catch (e) {}
+try {
+    f3(arr2[1] !== arr2[3]);
+} catch (e) {}
+try {
+    f3(arr2[2] !== arr2[3]);
+} catch (e) {}
+try {
+    eval("tag`Hello\n${v}world`");
+    eval("tag`Hello\n${v}world`");
+    f3();
+    f3(arr2[0] !== arr2[1]);
+} catch (e) {}
+try {
+    arr10[1][1] = set1 * 100 + 1 * 10 + 1;
+} catch (e) {}
+try {
+    arr10[1][1] = set1 * 100 + 1 * 10 + 1;
+} catch (e) {}
+try {
+    eval("tag`Hello${v}\nworld`");
+    eval("tag`Hello${v}\nworld`");
+    eval("tag`Hello\n${v}world`");
+    delete a1[__getRandomProperty()]();
+} catch (e) {}
+try {
+    106779[__getRandomProperty()]();
+} catch (e) {}
+try {
+    entries[__getRandomProperty()]();
+} catch (e) {}
+try {
+    set1[__getRandomProperty()]();
+} catch (e) {}
+try {
+    f3();
+} catch (e) {}
+arr2[0] !== arr2[1]
+gc();
+for (arr10 = 0; arr10 < 3; ++arr10) {
+    try {
+    } catch (e) {}
+
+}
+try {
+    f4`Hello${
+        4}world`;
+} catch (e) {}
+for (var counter = 0x10000; counter < 0x10ffff; ++counter) {
+    var charCode = String.fromCharCode();
+}

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/ChangeLog (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/ChangeLog	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/ChangeLog	2018-02-20 07:35:00 UTC (rev 228742)
@@ -1,3 +1,54 @@
+2018-02-16  Saam Barati  <sbar...@apple.com>
+
+        Fix bugs from r228411
+        https://bugs.webkit.org/show_bug.cgi?id=182851
+        <rdar://problem/37577732>
+
+        Reviewed by JF Bastien.
+
+        There was a bug from r228411 where inside the constant folding phase,
+        we used an insertCheck method that didn't handle varargs. This would
+        lead to a crash. When thinking about the fix for that function, I realized
+        a made a couple of mistakes in r228411. One is probably a security bug, and
+        the other is a performance bug because it'll prevent CSE for certain flavors
+        of GetByVal nodes. Both blunders are similar in nature.
+        
+        In r228411, I added code in LICM that inserted a CheckVarargs node with children
+        of another varargs node. However, to construct this new node's children,
+        I just copied the AdjacencyList. This does a shallow copy. What we needed
+        was a deep copy. We needed to create a new vararg AdjacencyList that points
+        to edges that are deep copies of the original varargs children. This patch
+        fixes this goof in LICM.
+        
+        r228411 made it so that PureValue over a varargs node would just compare actual
+        AdjacencyLists structs. So, if you had two GetByVals that had equal santized
+        children, their actual AdjacencyList structs are *not* bitwise equal, since they'll
+        have different firstChild values. Instead, we need to do a deep compare of their
+        adjacency lists. This patch teaches PureValue how to do that.
+
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::foldConstants):
+        * dfg/DFGGraph.h:
+        (JSC::DFG::Graph::copyVarargChildren):
+        * dfg/DFGInsertionSet.h:
+        (JSC::DFG::InsertionSet::insertCheck):
+        * dfg/DFGLICMPhase.cpp:
+        (JSC::DFG::LICMPhase::attemptHoist):
+        * dfg/DFGPureValue.cpp:
+        (JSC::DFG::PureValue::dump const):
+        * dfg/DFGPureValue.h:
+        (JSC::DFG::PureValue::PureValue):
+        (JSC::DFG::PureValue::op const):
+        (JSC::DFG::PureValue::hash const):
+        (JSC::DFG::PureValue::operator== const):
+        (JSC::DFG::PureValue::isVarargs const):
+        (JSC::DFG::PureValue::children const): Deleted.
+        * dfg/DFGStrengthReductionPhase.cpp:
+        (JSC::DFG::StrengthReductionPhase::handleNode):
+        (JSC::DFG::StrengthReductionPhase::convertToIdentityOverChild):
+
 2018-02-13  Saam Barati  <sbar...@apple.com>
 
         Follup fix to r228411 for 32-bit builds. I missed a place where we used non vararg getter for child2().

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGClobberize.h (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGClobberize.h	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGClobberize.h	2018-02-20 07:35:00 UTC (rev 228742)
@@ -788,7 +788,7 @@
                 return;
             }
             // This appears to read nothing because it's only reading immutable data.
-            def(PureValue(node, mode.asWord()));
+            def(PureValue(graph, node, mode.asWord()));
             return;
             
         case Array::DirectArguments:
@@ -840,7 +840,7 @@
             return;
 
         case Array::Undecided:
-            def(PureValue(node));
+            def(PureValue(graph, node));
             return;
             
         case Array::ArrayStorage:

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGConstantFoldingPhase.cpp	2018-02-20 07:35:00 UTC (rev 228742)
@@ -804,7 +804,7 @@
                     OpInfo(node->variableAccessData()));
                 m_graph.dethread();
             } else
-                m_insertionSet.insertCheck(indexInBlock, node->origin, node->children);
+                m_insertionSet.insertCheck(m_graph, indexInBlock, node);
             m_graph.convertToConstant(node, value);
             
             changed = true;

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGGraph.h (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGGraph.h	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGGraph.h	2018-02-20 07:35:00 UTC (rev 228742)
@@ -525,6 +525,22 @@
             return varArgNumChildren(node);
         return AdjacencyList::Size;
     }
+
+    template <typename Function = bool(*)(Edge)>
+    AdjacencyList copyVarargChildren(Node* node, Function filter = [] (Edge) { return true; })
+    {
+        ASSERT(node->flags() & NodeHasVarArgs);
+        unsigned firstChild = m_varArgChildren.size();
+        unsigned numChildren = 0;
+        doToChildren(node, [&] (Edge edge) {
+            if (filter(edge)) {
+                ++numChildren;
+                m_varArgChildren.append(edge);
+            }
+        });
+
+        return AdjacencyList(AdjacencyList::Variable, firstChild, numChildren);
+    }
     
     Edge& varArgChild(Node* node, unsigned index)
     {

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGInsertionSet.h (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGInsertionSet.h	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGInsertionSet.h	2018-02-20 07:35:00 UTC (rev 228742)
@@ -116,9 +116,15 @@
         return insertNode(index, SpecNone, Check, origin, children);
     }
     
-    Node* insertCheck(size_t index, Node* node)
+    Node* insertCheck(Graph& graph, size_t index, Node* node)
     {
-        return insertCheck(index, node->origin, node->children);
+        if (!(node->flags() & NodeHasVarArgs))
+            return insertCheck(index, node->origin, node->children);
+
+        AdjacencyList children = graph.copyVarargChildren(node, [] (Edge edge) { return edge.willHaveCheck(); });
+        if (!children.numChildren())
+            return nullptr;
+        return insertNode(index, SpecNone, CheckVarargs, node->origin, children);
     }
     
     Node* insertCheck(size_t index, NodeOrigin origin, Edge edge)

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGLICMPhase.cpp (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGLICMPhase.cpp	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGLICMPhase.cpp	2018-02-20 07:35:00 UTC (rev 228742)
@@ -343,7 +343,10 @@
             m_interpreter.execute(node);
         }
 
-        nodeRef = m_graph.addNode((node->flags() & NodeHasVarArgs) ? CheckVarargs : Check, originalOrigin, node->children);
+        if (node->flags() & NodeHasVarArgs)
+            nodeRef = m_graph.addNode(CheckVarargs, originalOrigin, m_graph.copyVarargChildren(node));
+        else
+            nodeRef = m_graph.addNode(Check, originalOrigin, node->children);
         
         return true;
     }

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGPureValue.cpp (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGPureValue.cpp	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGPureValue.cpp	2018-02-20 07:35:00 UTC (rev 228742)
@@ -37,12 +37,13 @@
     out.print(Graph::opName(op()));
     out.print("(");
     CommaPrinter comma;
-    if (defaultFlags(m_op) & NodeHasVarArgs)
-        out.print(comma, " VarArgs: firstChild=", m_children.firstChild(), " numChildren=", m_children.numChildren());
-    else {
+    if (isVarargs()) {
+        for (unsigned i = 0; i < m_children.numChildren(); ++i)
+            out.print(comma, m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized());
+    } else {
         for (unsigned i = 0; i < AdjacencyList::Size; ++i) {
-            if (children().child(i))
-                out.print(comma, children().child(i));
+            if (m_children.child(i))
+                out.print(comma, m_children.child(i));
         }
     }
     if (m_info)

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGPureValue.h (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGPureValue.h	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGPureValue.h	2018-02-20 07:35:00 UTC (rev 228742)
@@ -41,9 +41,10 @@
     
     PureValue(NodeType op, const AdjacencyList& children, uintptr_t info)
         : m_op(op)
-        , m_children(defaultFlags(op) & NodeHasVarArgs ? children : children.sanitized())
+        , m_children(children.sanitized())
         , m_info(info)
     {
+        ASSERT(!(defaultFlags(op) & NodeHasVarArgs));
     }
     
     PureValue(NodeType op, const AdjacencyList& children, const void* ptr)
@@ -70,7 +71,22 @@
         : PureValue(node->op(), node->children)
     {
     }
-    
+
+    PureValue(Graph& graph, Node* node, uintptr_t info)
+        : m_op(node->op())
+        , m_children(node->children)
+        , m_info(info)
+        , m_graph(&graph)
+    {
+        ASSERT(node->flags() & NodeHasVarArgs);
+        ASSERT(isVarargs());
+    }
+
+    PureValue(Graph& graph, Node* node)
+        : PureValue(graph, node, static_cast<uintptr_t>(0))
+    {
+    }
+
     PureValue(WTF::HashTableDeletedValueType)
         : m_op(LastNodeType)
         , m_info(1)
@@ -80,19 +96,33 @@
     bool operator!() const { return m_op == LastNodeType && !m_info; }
     
     NodeType op() const { return m_op; }
-    const AdjacencyList& children() const { return m_children; }
     uintptr_t info() const { return m_info; }
 
     unsigned hash() const
     {
-        return WTF::IntHash<int>::hash(static_cast<int>(m_op)) + m_children.hash() + m_info;
+        unsigned hash = WTF::IntHash<int>::hash(static_cast<int>(m_op)) + m_info;
+        if (!isVarargs())
+            return hash ^ m_children.hash();
+        for (unsigned i = 0; i < m_children.numChildren(); ++i)
+            hash ^= m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized().hash();
+        return hash;
     }
     
     bool operator==(const PureValue& other) const
     {
-        return m_op == other.m_op
-            && m_children == other.m_children
-            && m_info == other.m_info;
+        if (isVarargs() != other.isVarargs() || m_op != other.m_op || m_info != other.m_info)
+            return false;
+        if (!isVarargs())
+            return m_children == other.m_children;
+        if (m_children.numChildren() != other.m_children.numChildren())
+            return false;
+        for (unsigned i = 0; i < m_children.numChildren(); ++i) {
+            Edge a = m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized();
+            Edge b = m_graph->m_varArgChildren[other.m_children.firstChild() + i].sanitized();
+            if (a != b)
+                return false;
+        }
+        return true;
     }
     
     bool isHashTableDeletedValue() const
@@ -103,9 +133,12 @@
     void dump(PrintStream& out) const;
     
 private:
+    bool isVarargs() const { return !!m_graph; }
+
     NodeType m_op;
     AdjacencyList m_children;
     uintptr_t m_info;
+    Graph* m_graph { nullptr };
 };
 
 struct PureValueHash {

Modified: releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGStrengthReductionPhase.cpp (228741 => 228742)


--- releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGStrengthReductionPhase.cpp	2018-02-20 07:34:53 UTC (rev 228741)
+++ releases/WebKitGTK/webkit-2.20/Source/_javascript_Core/dfg/DFGStrengthReductionPhase.cpp	2018-02-20 07:35:00 UTC (rev 228742)
@@ -231,7 +231,7 @@
             for (Node* node = m_node->child1().node(); ; node = node->child1().node()) {
                 if (canonicalResultRepresentation(node->result()) ==
                     canonicalResultRepresentation(m_node->result())) {
-                    m_insertionSet.insertCheck(m_nodeIndex, m_node);
+                    m_insertionSet.insertCheck(m_graph, m_nodeIndex, m_node);
                     if (hadInt32Check) {
                         // FIXME: Consider adding Int52RepInt32Use or even DoubleRepInt32Use,
                         // which would be super weird. The latter would only arise in some
@@ -912,7 +912,8 @@
             
     void convertToIdentityOverChild(unsigned childIndex)
     {
-        m_insertionSet.insertCheck(m_nodeIndex, m_node);
+        ASSERT(!(m_node->flags() & NodeHasVarArgs));
+        m_insertionSet.insertCheck(m_graph, m_nodeIndex, m_node);
         m_node->children.removeEdge(childIndex ^ 1);
         m_node->convertToIdentity();
         m_changed = true;
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to