Attachment 359991 Details for Bug 193751 – patch

[patch] patch

b-backup.diff (text/plain), 10.48 KB, created by Saam Barati on 2019-01-23 21:24:03 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2019-01-23 21:24:03 PST

Size: 10.48 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 240403)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-01-23  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 NOBODY (OOPS!).
>+
>+        * 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-23  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
>Index: JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js
>===================================================================
>--- JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js	(nonexistent)
>+++ JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js	(working copy)
>@@ -0,0 +1,30 @@
>+//@ runDefault("--useConcurrentJIT=0", "--jitPolicyScale=0", "--collectContinuously=1")
>+
>+let thing = []
>+
>+function bar(x) {
>+    thing.push(x);
>+}
>+
>+function foo() {
>+    let hello = function () {
>+        let tmp = 1;
>+        return function (num) {
>+            if (tmp) {
>+                if (num.length) {
>+                }
>+            }
>+        };
>+    }();
>+
>+    bar();
>+    for (j = 0; j < 10000; j++) {
>+        if (/\s/.test(' ')) {
>+            hello(j);
>+        }
>+    }
>+}
>+
>+for (let i=0; i<100; i++) {
>+    foo();
>+}
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 240363)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,36 @@
>+2019-01-23  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 NOBODY (OOPS!).
>+
>+        The Object Allocation Sinking phase may move allocations around inside
>+        of the program. However, it was not ensuring that it's still possible 
>+        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-23  David Kilzer  <ddkilzer@apple.com>
> 
>         [JSC] Duplicate global variables: JSC::opcodeLengths
>Index: Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp	(revision 240363)
>+++ Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp	(working copy)
>@@ -1215,7 +1215,70 @@ private:
>             }
>         }
> 
>+        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());
> 
>@@ -1232,59 +1295,17 @@ private:
>         if (DFGObjectAllocationSinkingPhaseInternal::verbose)
>             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();
>     }
> 
>     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.
>         // The way to interpret this vector is:

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193751: 359986 | 359991 | 360027