Changeset 228742 in webkit


Ignore:
Timestamp:
Feb 19, 2018 11:35:00 PM (6 years ago)
Author:
Carlos Garcia Campos
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/JavaScriptCore:

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

Location:
releases/WebKitGTK/webkit-2.20
Files:
1 added
10 edited

Legend:

Unmodified
Added
Removed

  • releases/WebKitGTK/webkit-2.20/JSTests/ChangeLog

    r228738 r228742  
     12018-02-16  Saam Barati  <sbarati@apple.com>
     2
     3        Fix bugs from r228411
     4        https://bugs.webkit.org/show_bug.cgi?id=182851
     5        <rdar://problem/37577732>
     6
     7        Reviewed by JF Bastien.
     8
     9        * stress/constant-folding-phase-insert-check-handle-varargs.js: Added.
     10
    1112018-02-12  Saam Barati  <sbarati@apple.com>
    212

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/ChangeLog

    r228741 r228742  
     12018-02-16  Saam Barati  <sbarati@apple.com>
     2
     3        Fix bugs from r228411
     4        https://bugs.webkit.org/show_bug.cgi?id=182851
     5        <rdar://problem/37577732>
     6
     7        Reviewed by JF Bastien.
     8
     9        There was a bug from r228411 where inside the constant folding phase,
     10        we used an insertCheck method that didn't handle varargs. This would
     11        lead to a crash. When thinking about the fix for that function, I realized
     12        a made a couple of mistakes in r228411. One is probably a security bug, and
     13        the other is a performance bug because it'll prevent CSE for certain flavors
     14        of GetByVal nodes. Both blunders are similar in nature.
     15       
     16        In r228411, I added code in LICM that inserted a CheckVarargs node with children
     17        of another varargs node. However, to construct this new node's children,
     18        I just copied the AdjacencyList. This does a shallow copy. What we needed
     19        was a deep copy. We needed to create a new vararg AdjacencyList that points
     20        to edges that are deep copies of the original varargs children. This patch
     21        fixes this goof in LICM.
     22       
     23        r228411 made it so that PureValue over a varargs node would just compare actual
     24        AdjacencyLists structs. So, if you had two GetByVals that had equal santized
     25        children, their actual AdjacencyList structs are *not* bitwise equal, since they'll
     26        have different firstChild values. Instead, we need to do a deep compare of their
     27        adjacency lists. This patch teaches PureValue how to do that.
     28
     29        * dfg/DFGClobberize.h:
     30        (JSC::DFG::clobberize):
     31        * dfg/DFGConstantFoldingPhase.cpp:
     32        (JSC::DFG::ConstantFoldingPhase::foldConstants):
     33        * dfg/DFGGraph.h:
     34        (JSC::DFG::Graph::copyVarargChildren):
     35        * dfg/DFGInsertionSet.h:
     36        (JSC::DFG::InsertionSet::insertCheck):
     37        * dfg/DFGLICMPhase.cpp:
     38        (JSC::DFG::LICMPhase::attemptHoist):
     39        * dfg/DFGPureValue.cpp:
     40        (JSC::DFG::PureValue::dump const):
     41        * dfg/DFGPureValue.h:
     42        (JSC::DFG::PureValue::PureValue):
     43        (JSC::DFG::PureValue::op const):
     44        (JSC::DFG::PureValue::hash const):
     45        (JSC::DFG::PureValue::operator== const):
     46        (JSC::DFG::PureValue::isVarargs const):
     47        (JSC::DFG::PureValue::children const): Deleted.
     48        * dfg/DFGStrengthReductionPhase.cpp:
     49        (JSC::DFG::StrengthReductionPhase::handleNode):
     50        (JSC::DFG::StrengthReductionPhase::convertToIdentityOverChild):
     51
    1522018-02-13  Saam Barati  <sbarati@apple.com>
    253

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGClobberize.h

    r228740 r228742  
    789789            }
    790790            // This appears to read nothing because it's only reading immutable data.
    791             def(PureValue(node, mode.asWord()));
     791            def(PureValue(graph, node, mode.asWord()));
    792792            return;
    793793           
    … …  
    841841
    842842        case Array::Undecided:
    843             def(PureValue(node));
     843            def(PureValue(graph, node));
    844844            return;
    845845           

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp

    r228740 r228742  
    805805                m_graph.dethread();
    806806            } else
    807                 m_insertionSet.insertCheck(indexInBlock, node->origin, node->children);
     807                m_insertionSet.insertCheck(m_graph, indexInBlock, node);
    808808            m_graph.convertToConstant(node, value);
    809809           

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGGraph.h

    r227136 r228742  
    525525            return varArgNumChildren(node);
    526526        return AdjacencyList::Size;
     527    }
     528
     529    template <typename Function = bool(*)(Edge)>
     530    AdjacencyList copyVarargChildren(Node* node, Function filter = [] (Edge) { return true; })
     531    {
     532        ASSERT(node->flags() & NodeHasVarArgs);
     533        unsigned firstChild = m_varArgChildren.size();
     534        unsigned numChildren = 0;
     535        doToChildren(node, [&] (Edge edge) {
     536            if (filter(edge)) {
     537                ++numChildren;
     538                m_varArgChildren.append(edge);
     539            }
     540        });
     541
     542        return AdjacencyList(AdjacencyList::Variable, firstChild, numChildren);
    527543    }
    528544   

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGInsertionSet.h

    r206525 r228742  
    117117    }
    118118   
    119     Node* insertCheck(size_t index, Node* node)
     119    Node* insertCheck(Graph& graph, size_t index, Node* node)
    120120    {
    121         return insertCheck(index, node->origin, node->children);
     121        if (!(node->flags() & NodeHasVarArgs))
     122            return insertCheck(index, node->origin, node->children);
     123
     124        AdjacencyList children = graph.copyVarargChildren(node, [] (Edge edge) { return edge.willHaveCheck(); });
     125        if (!children.numChildren())
     126            return nullptr;
     127        return insertNode(index, SpecNone, CheckVarargs, node->origin, children);
    122128    }
    123129   

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGLICMPhase.cpp

    r228740 r228742  
    344344        }
    345345
    346         nodeRef = m_graph.addNode((node->flags() & NodeHasVarArgs) ? CheckVarargs : Check, originalOrigin, node->children);
     346        if (node->flags() & NodeHasVarArgs)
     347            nodeRef = m_graph.addNode(CheckVarargs, originalOrigin, m_graph.copyVarargChildren(node));
     348        else
     349            nodeRef = m_graph.addNode(Check, originalOrigin, node->children);
    347350       
    348351        return true;

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGPureValue.cpp

    r228740 r228742  
    3838    out.print("(");
    3939    CommaPrinter comma;
    40     if (defaultFlags(m_op) & NodeHasVarArgs)
    41         out.print(comma, " VarArgs: firstChild=", m_children.firstChild(), " numChildren=", m_children.numChildren());
    42     else {
     40    if (isVarargs()) {
     41        for (unsigned i = 0; i < m_children.numChildren(); ++i)
     42            out.print(comma, m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized());
     43    } else {
    4344        for (unsigned i = 0; i < AdjacencyList::Size; ++i) {
    44             if (children().child(i))
    45                 out.print(comma, children().child(i));
     45            if (m_children.child(i))
     46                out.print(comma, m_children.child(i));
    4647        }
    4748    }

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGPureValue.h

    r228740 r228742  
    4242    PureValue(NodeType op, const AdjacencyList& children, uintptr_t info)
    4343        : m_op(op)
    44         , m_children(defaultFlags(op) & NodeHasVarArgs ? children : children.sanitized())
     44        , m_children(children.sanitized())
    4545        , m_info(info)
    4646    {
     47        ASSERT(!(defaultFlags(op) & NodeHasVarArgs));
    4748    }
    4849   
    … …  
    7172    {
    7273    }
    73    
     74
     75    PureValue(Graph& graph, Node* node, uintptr_t info)
     76        : m_op(node->op())
     77        , m_children(node->children)
     78        , m_info(info)
     79        , m_graph(&graph)
     80    {
     81        ASSERT(node->flags() & NodeHasVarArgs);
     82        ASSERT(isVarargs());
     83    }
     84
     85    PureValue(Graph& graph, Node* node)
     86        : PureValue(graph, node, static_cast<uintptr_t>(0))
     87    {
     88    }
     89
    7490    PureValue(WTF::HashTableDeletedValueType)
    7591        : m_op(LastNodeType)
    … …  
    8197   
    8298    NodeType op() const { return m_op; }
    83     const AdjacencyList& children() const { return m_children; }
    8499    uintptr_t info() const { return m_info; }
    85100
    86101    unsigned hash() const
    87102    {
    88         return WTF::IntHash<int>::hash(static_cast<int>(m_op)) + m_children.hash() + m_info;
     103        unsigned hash = WTF::IntHash<int>::hash(static_cast<int>(m_op)) + m_info;
     104        if (!isVarargs())
     105            return hash ^ m_children.hash();
     106        for (unsigned i = 0; i < m_children.numChildren(); ++i)
     107            hash ^= m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized().hash();
     108        return hash;
    89109    }
    90110   
    91111    bool operator==(const PureValue& other) const
    92112    {
    93         return m_op == other.m_op
    94             && m_children == other.m_children
    95             && m_info == other.m_info;
     113        if (isVarargs() != other.isVarargs() || m_op != other.m_op || m_info != other.m_info)
     114            return false;
     115        if (!isVarargs())
     116            return m_children == other.m_children;
     117        if (m_children.numChildren() != other.m_children.numChildren())
     118            return false;
     119        for (unsigned i = 0; i < m_children.numChildren(); ++i) {
     120            Edge a = m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized();
     121            Edge b = m_graph->m_varArgChildren[other.m_children.firstChild() + i].sanitized();
     122            if (a != b)
     123                return false;
     124        }
     125        return true;
    96126    }
    97127   
    … …  
    104134   
    105135private:
     136    bool isVarargs() const { return !!m_graph; }
     137
    106138    NodeType m_op;
    107139    AdjacencyList m_children;
    108140    uintptr_t m_info;
     141    Graph* m_graph { nullptr };
    109142};
    110143

  • releases/WebKitGTK/webkit-2.20/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp

    r227107 r228742  
    232232                if (canonicalResultRepresentation(node->result()) ==
    233233                    canonicalResultRepresentation(m_node->result())) {
    234                     m_insertionSet.insertCheck(m_nodeIndex, m_node);
     234                    m_insertionSet.insertCheck(m_graph, m_nodeIndex, m_node);
    235235                    if (hadInt32Check) {
    236236                        // FIXME: Consider adding Int52RepInt32Use or even DoubleRepInt32Use,
    … …  
    913913    void convertToIdentityOverChild(unsigned childIndex)
    914914    {
    915         m_insertionSet.insertCheck(m_nodeIndex, m_node);
     915        ASSERT(!(m_node->flags() & NodeHasVarArgs));
     916        m_insertionSet.insertCheck(m_graph, m_nodeIndex, m_node);
    916917        m_node->children.removeEdge(childIndex ^ 1);
    917918        m_node->convertToIdentity();
Note: See TracChangeset for help on using the changeset viewer.