Title: [195387] trunk/Source/_javascript_Core
Revision
195387
Author
[email protected]
Date
2016-01-20 15:11:32 -0800 (Wed, 20 Jan 2016)

Log 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:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (195386 => 195387)


--- trunk/Source/_javascript_Core/ChangeLog	2016-01-20 23:00:19 UTC (rev 195386)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-01-20 23:11:32 UTC (rev 195387)
@@ -1,3 +1,32 @@
+2016-01-20  Benjamin Poulain  <[email protected]>
+
+        [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  <[email protected]>
 
         Fix CLoop build after bug https://bugs.webkit.org/show_bug.cgi?id=152766

Modified: trunk/Source/_javascript_Core/b3/air/AirIteratedRegisterCoalescing.cpp (195386 => 195387)


--- trunk/Source/_javascript_Core/b3/air/AirIteratedRegisterCoalescing.cpp	2016-01-20 23:00:19 UTC (rev 195386)
+++ trunk/Source/_javascript_Core/b3/air/AirIteratedRegisterCoalescing.cpp	2016-01-20 23:11:32 UTC (rev 195387)
@@ -961,11 +961,11 @@
 
         ++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;
             }

Modified: trunk/Source/_javascript_Core/b3/air/AirUseCounts.h (195386 => 195387)


--- trunk/Source/_javascript_Core/b3/air/AirUseCounts.h	2016-01-20 23:00:19 UTC (rev 195386)
+++ trunk/Source/_javascript_Core/b3/air/AirUseCounts.h	2016-01-20 23:11:32 UTC (rev 195387)
@@ -97,7 +97,12 @@
         }
     }
 
-    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
     {
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to