- Revision
- 240563
- Author
- [email protected]
- Date
- 2019-01-28 01:40:43 -0800 (Mon, 28 Jan 2019)
Log Message
Cherry-pick r240447. rdar://problem/47586899
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/_javascript_Core:
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:
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240447 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/JSTests/ChangeLog (240562 => 240563)
--- branches/safari-607-branch/JSTests/ChangeLog 2019-01-28 09:40:40 UTC (rev 240562)
+++ branches/safari-607-branch/JSTests/ChangeLog 2019-01-28 09:40:43 UTC (rev 240563)
@@ -1,5 +1,65 @@
2019-01-28 Babak Shafiei <[email protected]>
+ Cherry-pick r240447. rdar://problem/47586899
+
+ 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/_javascript_Core:
+
+ 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:
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240447 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-01-24 Saam Barati <[email protected]>
+
+ 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-28 Babak Shafiei <[email protected]>
+
Cherry-pick r240364. rdar://problem/47586820
[DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
Added: branches/safari-607-branch/JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js (0 => 240563)
--- branches/safari-607-branch/JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js 2019-01-28 09:40:43 UTC (rev 240563)
@@ -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();
+}
Modified: branches/safari-607-branch/Source/_javascript_Core/ChangeLog (240562 => 240563)
--- branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-01-28 09:40:40 UTC (rev 240562)
+++ branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-01-28 09:40:43 UTC (rev 240563)
@@ -1,5 +1,85 @@
2019-01-28 Babak Shafiei <[email protected]>
+ Cherry-pick r240447. rdar://problem/47586899
+
+ 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/_javascript_Core:
+
+ 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:
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@240447 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-01-24 Saam Barati <[email protected]>
+
+ 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-28 Babak Shafiei <[email protected]>
+
Cherry-pick r240364. rdar://problem/47586820
[DFG] AvailabilityMap::pruneByLiveness should make non-live operands Availability::unavailable instead of Availability()
Modified: branches/safari-607-branch/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (240562 => 240563)
--- branches/safari-607-branch/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-01-28 09:40:40 UTC (rev 240562)
+++ branches/safari-607-branch/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-01-28 09:40:43 UTC (rev 240563)
@@ -1215,7 +1215,70 @@
}
}
+ 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 @@
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);
- }
+ // Create the materialization nodes.
+ forEachEscapee([&] (HashMap<Node*, Allocation>& escapees, Node* where) {
+ placeMaterializations(WTFMove(escapees), where);
+ });
- 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());
- }
- }
-
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:
