Changeset 219317 in webkit

Timestamp:

Jul 10, 2017 5:29:36 PM (7 years ago)

Author:

sbarati@apple.com

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:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGObjectAllocationSinkingPhase.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-07-10  Saam Barati  <sbarati@apple.com>
+
+        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  <drousso@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -219 +219 @@
         ASSERT(hasStructures());
         m_structures.filter(structures);
+        RELEASE_ASSERT(!m_structures.isEmpty());
         return *this;
     }
@@ -903 +904 @@
             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);
@@ -947 +956 @@
                     }
                 }
-                if (hasInvalidStructures) {
+                if (hasInvalidStructures || validStructures.isEmpty()) {
                     m_heap.escape(node->child1().node());
                     break;