Changeset 240447 in webkit

Timestamp:

Jan 24, 2019 1:30:55 PM (5 years ago)

Author:

sbarati@apple.com

Message:

Object Allocation Sinking phase can move a node that walks the stack into a place where the InlineCallFrame is no longer valid
​https://bugs.webkit.org/show_bug.cgi?id=193751
<rdar://problem/47280215>

Reviewed by Michael Saboff.

JSTests:

• stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js: Added.

(let.thing):
(foo.let.hello):
(foo):

Source/JavaScriptCore:

The Object Allocation Sinking phase may move allocations around inside
of the program. However, it was not ensuring that it's still possible
to walk the stack at the point in the program that it moved the allocation to.
Certain InlineCallFrames rely on data in the stack when taking a stack trace.
All allocation sites can do a stack walk (we do a stack walk when we GC).
Conservatively, this patch says we're ok to move this allocation if we are
moving within the same InlineCallFrame. We could be more precise and do an
analysis of stack writes. However, this scenario is so rare that we just
take the conservative-and-straight-forward approach of checking that the place
we're moving to is the same InlineCallFrame as the allocation site.

In general, this issue arises anytime we do any kind of code motion.
Interestingly, LICM gets this right. It gets it right because the only
InlineCallFrames we can't move out of are the InlineCallFrames that
have metadata stored on the stack (callee for closure calls and argument
count for varargs calls). LICM doesn't have this issue because it relies
on Clobberize for doing its effects analysis. In clobberize, we model every
node within an InlineCallFrame that meets the above criteria as reading
from those stack fields. Consequently, LICM won't hoist any node in that
InlineCallFrame past the beginning of the InlineCallFrame since the IR
we generate to set up such an InlineCallFrame contains writes to that
stack location.

• dfg/DFGObjectAllocationSinkingPhase.cpp:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-01-24  Saam Barati  <sbarati@apple.com>
+
+        Object Allocation Sinking phase can move a node that walks the stack into a place where the InlineCallFrame is no longer valid
+        https://bugs.webkit.org/show_bug.cgi?id=193751
+        <rdar://problem/47280215>
+
+        Reviewed by Michael Saboff.
+
+        * stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js: Added.
+        (let.thing):
+        (foo.let.hello):
+        (foo):
+
 2019-01-24  Guillaume Emont  <guijemont@igalia.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-01-24  Saam Barati  <sbarati@apple.com>
+
+        Object Allocation Sinking phase can move a node that walks the stack into a place where the InlineCallFrame is no longer valid
+        https://bugs.webkit.org/show_bug.cgi?id=193751
+        <rdar://problem/47280215>
+
+        Reviewed by Michael Saboff.
+
+        The Object Allocation Sinking phase may move allocations around inside
+        of the program. However, it was not ensuring that it's still possible 
+        to walk the stack at the point in the program that it moved the allocation to.
+        Certain InlineCallFrames rely on data in the stack when taking a stack trace.
+        All allocation sites can do a stack walk (we do a stack walk when we GC).
+        Conservatively, this patch says we're ok to move this allocation if we are
+        moving within the same InlineCallFrame. We could be more precise and do an
+        analysis of stack writes. However, this scenario is so rare that we just
+        take the conservative-and-straight-forward approach of checking that the place
+        we're moving to is the same InlineCallFrame as the allocation site.
+        
+        In general, this issue arises anytime we do any kind of code motion.
+        Interestingly, LICM gets this right. It gets it right because the only
+        InlineCallFrames we can't move out of are the InlineCallFrames that
+        have metadata stored on the stack (callee for closure calls and argument
+        count for varargs calls). LICM doesn't have this issue because it relies
+        on Clobberize for doing its effects analysis. In clobberize, we model every
+        node within an InlineCallFrame that meets the above criteria as reading
+        from those stack fields. Consequently, LICM won't hoist any node in that
+        InlineCallFrame past the beginning of the InlineCallFrame since the IR
+        we generate to set up such an InlineCallFrame contains writes to that
+        stack location.
+
+        * dfg/DFGObjectAllocationSinkingPhase.cpp:
+
 2019-01-24  Guillaume Emont  <guijemont@igalia.com>
 

trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -1216 +1216 @@
         }
 
+        auto forEachEscapee = [&] (auto callback) {
+            for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
+                m_heap = m_heapAtHead[block];
+                m_heap.setWantEscapees();
+
+                for (Node* node : *block) {
+                    handleNode(
+                        node,
+                        [] (PromotedHeapLocation, LazyNode) { },
+                        [] (PromotedHeapLocation) -> Node* {
+                            return nullptr;
+                        });
+                    auto escapees = m_heap.takeEscapees();
+                    escapees.removeIf([&] (const auto& entry) { return !m_sinkCandidates.contains(entry.key); });
+                    callback(escapees, node);
+                }
+
+                m_heap.pruneByLiveness(m_combinedLiveness.liveAtTail[block]);
+
+                {
+                    HashMap<Node*, Allocation> escapingOnEdge;
+                    for (const auto& entry : m_heap.allocations()) {
+                        if (entry.value.isEscapedAllocation())
+                            continue;
+
+                        bool mustEscape = false;
+                        for (BasicBlock* successorBlock : block->successors()) {
+                            if (!m_heapAtHead[successorBlock].isAllocation(entry.key)
+                                || m_heapAtHead[successorBlock].getAllocation(entry.key).isEscapedAllocation())
+                                mustEscape = true;
+                        }
+
+                        if (mustEscape && m_sinkCandidates.contains(entry.key))
+                            escapingOnEdge.add(entry.key, entry.value);
+                    }
+                    callback(escapingOnEdge, block->terminal());
+                }
+            }
+        };
+
+        if (m_sinkCandidates.size()) {
+            // If we're moving an allocation to `where` in the program, we need to ensure
+            // we can still walk the stack at that point in the program for the
+            // InlineCallFrame of the original allocation. Certain InlineCallFrames rely on
+            // data in the stack when taking a stack trace. All allocation sites can do a
+            // stack walk (we do a stack walk when we GC). Conservatively, we say we're
+            // still ok to move this allocation if we are moving within the same InlineCallFrame.
+            // We could be more precise here and do an analysis of stack writes. However,
+            // this scenario is so rare that we just take the conservative-and-straight-forward 
+            // approach of checking that we're in the same InlineCallFrame.
+
+            forEachEscapee([&] (HashMap<Node*, Allocation>& escapees, Node* where) {
+                for (Node* allocation : escapees.keys()) {
+                    InlineCallFrame* inlineCallFrame = allocation->origin.semantic.inlineCallFrame;
+                    if (!inlineCallFrame)
+                        continue;
+                    if ((inlineCallFrame->isClosureCall || inlineCallFrame->isVarargs()) && inlineCallFrame != where->origin.semantic.inlineCallFrame)
+                        m_sinkCandidates.remove(allocation);
+                }
+            });
+        }
+
         // Ensure that the set of sink candidates is closed for put operations
+        // This is (2) as described above.
         Vector<Node*> worklist;
         worklist.appendRange(m_sinkCandidates.begin(), m_sinkCandidates.end());
@@ -1233 +1296 @@
             dataLog("Candidates: ", listDump(m_sinkCandidates), "\n");
 
-        // Create the materialization nodes
-        for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
-            m_heap = m_heapAtHead[block];
-            m_heap.setWantEscapees();
-
-            for (Node* node : *block) {
-                handleNode(
-                    node,
-                    [] (PromotedHeapLocation, LazyNode) { },
-                    [] (PromotedHeapLocation) -> Node* {
-                        return nullptr;
-                    });
-                auto escapees = m_heap.takeEscapees();
-                if (!escapees.isEmpty())
-                    placeMaterializations(escapees, node);
-            }
-
-            m_heap.pruneByLiveness(m_combinedLiveness.liveAtTail[block]);
-
-            {
-                HashMap<Node*, Allocation> escapingOnEdge;
-                for (const auto& entry : m_heap.allocations()) {
-                    if (entry.value.isEscapedAllocation())
-                        continue;
-
-                    bool mustEscape = false;
-                    for (BasicBlock* successorBlock : block->successors()) {
-                        if (!m_heapAtHead[successorBlock].isAllocation(entry.key)
-                            || m_heapAtHead[successorBlock].getAllocation(entry.key).isEscapedAllocation())
-                            mustEscape = true;
-                    }
-
-                    if (mustEscape)
-                        escapingOnEdge.add(entry.key, entry.value);
-                }
-                placeMaterializations(WTFMove(escapingOnEdge), block->terminal());
-            }
-        }
+
+        // Create the materialization nodes.
+        forEachEscapee([&] (HashMap<Node*, Allocation>& escapees, Node* where) {
+            placeMaterializations(WTFMove(escapees), where);
+        });
 
         return hasUnescapedReads || !m_sinkCandidates.isEmpty();
@@ -1277 +1307 @@
     void placeMaterializations(HashMap<Node*, Allocation> escapees, Node* where)
     {
-        // We don't create materializations if the escapee is not a
-        // sink candidate
-        escapees.removeIf(
-            [&] (const auto& entry) {
-                return !m_sinkCandidates.contains(entry.key);
-            });
-        if (escapees.isEmpty())
-            return;
-
         // First collect the hints that will be needed when the node
         // we materialize is still stored into other unescaped sink candidates.