Modified: trunk/JSTests/ChangeLog (240446 => 240447)
--- trunk/JSTests/ChangeLog 2019-01-24 21:09:37 UTC (rev 240446)
+++ trunk/JSTests/ChangeLog 2019-01-24 21:30:55 UTC (rev 240447)
@@ -1,3 +1,16 @@
+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-24 Guillaume Emont <[email protected]>
[JSC] Reenable baseline JIT on mips
Added: trunk/JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js (0 => 240447)
--- trunk/JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js (rev 0)
+++ trunk/JSTests/stress/object-allocation-sinking-phase-must-only-move-allocations-if-stack-trace-is-still-valid.js 2019-01-24 21:30:55 UTC (rev 240447)
@@ -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: trunk/Source/_javascript_Core/ChangeLog (240446 => 240447)
--- trunk/Source/_javascript_Core/ChangeLog 2019-01-24 21:09:37 UTC (rev 240446)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-01-24 21:30:55 UTC (rev 240447)
@@ -1,3 +1,36 @@
+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-24 Guillaume Emont <[email protected]>
[JSC] Reenable baseline JIT on mips
Modified: trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (240446 => 240447)
--- trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-01-24 21:09:37 UTC (rev 240446)
+++ trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-01-24 21:30:55 UTC (rev 240447)
@@ -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:
