Changeset 195387 in webkit

Timestamp:

Jan 20, 2016 3:11:32 PM (8 years ago)

Author:

benjamin@webkit.org

Message:

[JSC] The register allocator can use a dangling pointer when selecting a spill candidate
​https://bugs.webkit.org/show_bug.cgi?id=153287

Reviewed by Mark Lam.

A tricky bug I discovered while experimenting with live range breaking.

We have the following initial conditions:
-UseCounts is slow, so we only compute it once for all the iterations

of the allocator.

-The only new Tmps we create are for spills and refills. They are unspillable

by definition so it is fine to not update UseCounts accordingly.

But, in selectSpill(), we go over all the spill candidates and select the best
one based on its score. The score() lambda uses useCounts, it cannot be used
with a new Tmps created for something we already spilled.

The first time we use score is correct, we started by skipping all the unspillable
Tmps from the candidate. The next use was incorrect: we were checking unspillableTmps
*after* calling score().

The existing tests did not catch this due to back luck. I added an assertion
to find similar problems in the future.

• b3/air/AirIteratedRegisterCoalescing.cpp:
• b3/air/AirUseCounts.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• b3/air/AirIteratedRegisterCoalescing.cpp (modified) (1 diff)
• b3/air/AirUseCounts.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-01-20  Benjamin Poulain  <benjamin@webkit.org>
+
+        [JSC] The register allocator can use a dangling pointer when selecting a spill candidate
+        https://bugs.webkit.org/show_bug.cgi?id=153287
+
+        Reviewed by Mark Lam.
+
+        A tricky bug I discovered while experimenting with live range breaking.
+
+        We have the following initial conditions:
+        -UseCounts is slow, so we only compute it once for all the iterations
+         of the allocator.
+        -The only new Tmps we create are for spills and refills. They are unspillable
+         by definition so it is fine to not update UseCounts accordingly.
+
+        But, in selectSpill(), we go over all the spill candidates and select the best
+        one based on its score. The score() lambda uses useCounts, it cannot be used
+        with a new Tmps created for something we already spilled.
+
+        The first time we use score is correct, we started by skipping all the unspillable
+        Tmps from the candidate. The next use was incorrect: we were checking unspillableTmps
+        *after* calling score().
+
+        The existing tests did not catch this due to back luck. I added an assertion
+        to find similar problems in the future.
+
+        * b3/air/AirIteratedRegisterCoalescing.cpp:
+        * b3/air/AirUseCounts.h:
+
 2016-01-20  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/b3/air/AirIteratedRegisterCoalescing.cpp

@@ -962 +962 @@
         ++iterator;
         for (;iterator != m_spillWorklist.end(); ++iterator) {
+            if (m_unspillableTmps.contains(*iterator))
+                continue;
+
             double tmpScore = score(AbsoluteTmpMapper<type>::tmpFromAbsoluteIndex(*iterator));
             if (tmpScore > maxScore) {
-                if (m_unspillableTmps.contains(*iterator))
-                    continue;
-
                 victimIterator = iterator;
                 maxScore = tmpScore;

trunk/Source/JavaScriptCore/b3/air/AirUseCounts.h

@@ -98 +98 @@
     }
 
-    const Counts& operator[](const Thing& arg) const { return m_counts.find(arg)->value; }
+    const Counts& operator[](const Thing& arg) const
+    {
+        auto iterator = m_counts.find(arg);
+        ASSERT(iterator != m_counts.end());
+        return iterator->value;
+    }
 
     void dump(PrintStream& out) const