Title: [219317] trunk/Source/_javascript_Core
- Revision
- 219317
- Author
- [email protected]
- Date
- 2017-07-10 17:29:36 -0700 (Mon, 10 Jul 2017)
Log Message
Allocation sinking phase should consider a CheckStructure that would fail as an escape
https://bugs.webkit.org/show_bug.cgi?id=174321
<rdar://problem/32604963>
Reviewed by Filip Pizlo.
When the allocation sinking phase was generating stores to materialize
objects in a cycle with each other, it would assume that each materialized
object had a valid, non empty, set of structures. This is an OK assumption for
the phase to make because how do you materialize an object with no structure?
The abstract interpretation part of the phase will model what's in the heap.
However, it would sometimes model that a CheckStructure would fail. The phase
did nothing special for this; it just stored the empty set of structures for
its representation of a particular allocation. However, what the phase proved
in such a scenario is that, had the CheckStructure executed, it would have exited.
This patch treats such CheckStructures and MultiGetByOffsets as escape points.
This will cause the allocation in question to be materialized just before
the CheckStructure, and then at execution time, the CheckStructure will exit.
I wasn't able to write a test case for this. However, I was able to reproduce
this crash by manually editing the IR. I've opened a separate bug to help us
create a testing framework for writing tests for hard to reproduce bugs like this:
https://bugs.webkit.org/show_bug.cgi?id=174322
* dfg/DFGObjectAllocationSinkingPhase.cpp:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (219316 => 219317)
--- trunk/Source/_javascript_Core/ChangeLog 2017-07-11 00:01:55 UTC (rev 219316)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-07-11 00:29:36 UTC (rev 219317)
@@ -1,3 +1,33 @@
+2017-07-10 Saam Barati <[email protected]>
+
+ Allocation sinking phase should consider a CheckStructure that would fail as an escape
+ https://bugs.webkit.org/show_bug.cgi?id=174321
+ <rdar://problem/32604963>
+
+ Reviewed by Filip Pizlo.
+
+ When the allocation sinking phase was generating stores to materialize
+ objects in a cycle with each other, it would assume that each materialized
+ object had a valid, non empty, set of structures. This is an OK assumption for
+ the phase to make because how do you materialize an object with no structure?
+
+ The abstract interpretation part of the phase will model what's in the heap.
+ However, it would sometimes model that a CheckStructure would fail. The phase
+ did nothing special for this; it just stored the empty set of structures for
+ its representation of a particular allocation. However, what the phase proved
+ in such a scenario is that, had the CheckStructure executed, it would have exited.
+
+ This patch treats such CheckStructures and MultiGetByOffsets as escape points.
+ This will cause the allocation in question to be materialized just before
+ the CheckStructure, and then at execution time, the CheckStructure will exit.
+
+ I wasn't able to write a test case for this. However, I was able to reproduce
+ this crash by manually editing the IR. I've opened a separate bug to help us
+ create a testing framework for writing tests for hard to reproduce bugs like this:
+ https://bugs.webkit.org/show_bug.cgi?id=174322
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+
2017-07-10 Devin Rousso <[email protected]>
Web Inspector: Highlight matching CSS canvas clients when hovering contexts in the Resources tab
Modified: trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (219316 => 219317)
--- trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2017-07-11 00:01:55 UTC (rev 219316)
+++ trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2017-07-11 00:29:36 UTC (rev 219317)
@@ -218,6 +218,7 @@
{
ASSERT(hasStructures());
m_structures.filter(structures);
+ RELEASE_ASSERT(!m_structures.isEmpty());
return *this;
}
@@ -902,7 +903,15 @@
case CheckStructure: {
Allocation* allocation = m_heap.onlyLocalAllocation(node->child1().node());
if (allocation && allocation->isObjectAllocation()) {
- allocation->filterStructures(node->structureSet());
+ RegisteredStructureSet filteredStructures = allocation->structures();
+ filteredStructures.filter(node->structureSet());
+ if (filteredStructures.isEmpty()) {
+ // FIXME: Write a test for this:
+ // https://bugs.webkit.org/show_bug.cgi?id=174322
+ m_heap.escape(node->child1().node());
+ break;
+ }
+ allocation->setStructures(filteredStructures);
if (Node* value = heapResolve(PromotedHeapLocation(allocation->identifier(), StructurePLoc)))
node->convertToCheckStructureImmediate(value);
} else
@@ -946,7 +955,7 @@
RELEASE_ASSERT_NOT_REACHED();
}
}
- if (hasInvalidStructures) {
+ if (hasInvalidStructures || validStructures.isEmpty()) {
m_heap.escape(node->child1().node());
break;
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
