Title: [179457] trunk
Revision
179457
Author
msab...@apple.com
Date
2015-01-31 19:58:39 -0800 (Sat, 31 Jan 2015)

Log Message

Crash (DFG assertion) beneath AbstractInterpreter::verifyEdge() @ http://experilous.com/1/planet-generator/2014-09-28/version-1
https://bugs.webkit.org/show_bug.cgi?id=141111

Reviewed by Filip Pizlo.

Source/_javascript_Core:

In LowerDFGToLLVM::compileNode(), if we determine while compiling a node that we would have
exited, we don't need to process the OSR availability or abstract interpreter.

* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::safelyInvalidateAfterTermination): Broke this out a a separate
method since we need to call it at the top and near the bottom of compileNode().
(JSC::FTL::LowerDFGToLLVM::compileNode):

LayoutTests:

New tests.

* js/regress-141111-expected.txt: Added.
* js/regress-141111.html: Added.
* js/script-tests/regress-141111.js: Added.
(MyObject):
(foo):
(.result):
(bar):

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (179456 => 179457)


--- trunk/LayoutTests/ChangeLog	2015-02-01 03:11:14 UTC (rev 179456)
+++ trunk/LayoutTests/ChangeLog	2015-02-01 03:58:39 UTC (rev 179457)
@@ -1,3 +1,20 @@
+2015-01-31  Michael Saboff  <msab...@apple.com>
+
+        Crash (DFG assertion) beneath AbstractInterpreter::verifyEdge() @ http://experilous.com/1/planet-generator/2014-09-28/version-1
+        https://bugs.webkit.org/show_bug.cgi?id=141111
+
+        Reviewed by Filip Pizlo.
+
+        New tests.
+
+        * js/regress-141111-expected.txt: Added.
+        * js/regress-141111.html: Added.
+        * js/script-tests/regress-141111.js: Added.
+        (MyObject):
+        (foo):
+        (.result):
+        (bar):
+
 2015-01-31  Antti Koivisto  <an...@apple.com>
 
         Enable WebKit disk cache on OS X

Added: trunk/LayoutTests/js/regress-141111-expected.txt (0 => 179457)


--- trunk/LayoutTests/js/regress-141111-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/js/regress-141111-expected.txt	2015-02-01 03:58:39 UTC (rev 179457)
@@ -0,0 +1,9 @@
+Regression test for https://webkit.org/b/141111. This test should run without crashing.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/js/regress-141111.html (0 => 179457)


--- trunk/LayoutTests/js/regress-141111.html	                        (rev 0)
+++ trunk/LayoutTests/js/regress-141111.html	2015-02-01 03:58:39 UTC (rev 179457)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>

Added: trunk/LayoutTests/js/script-tests/regress-141111.js (0 => 179457)


--- trunk/LayoutTests/js/script-tests/regress-141111.js	                        (rev 0)
+++ trunk/LayoutTests/js/script-tests/regress-141111.js	2015-02-01 03:58:39 UTC (rev 179457)
@@ -0,0 +1,57 @@
+description(
+"Regression test for https://webkit.org/b/141111. This test should run without crashing."
+);
+
+function MyObject(v) {
+    this.v = v;
+}
+
+function foo(o, a, b, c) {
+    // Don't do anything real but have some control flow. This causes the PutLocals for a,
+    // b, and c to survive into SSA form. But we don't have any effects, so sinking will be
+    // successful.
+    if (o.v)
+        return o;
+    else
+        return z;
+}
+
+function bar(o, y) {
+    var a = y;
+    var b = y + 1;
+    var c = y + 2;
+    var d = y + 3;
+    var e = y + 4;
+    var f = y + 5;
+    var g = y + 6;
+    var h = y + 7;
+    var i = y + 8;
+    var j = y + 9;
+    var k = y + 10;
+    var result = function(p, q) {
+        var x = new MyObject(a + b + c + d + e + f + g + h + i + j + k);
+        if (q) {
+            // Make it appear that it's possible to clobber those closure variables, so that we
+            // load from them again down below.
+            a = b = c = d = e = f = g = h = i = j = k = 42;
+        }
+        if (p)
+            x = foo(o, 1, 2, 3)
+        else
+            x = five;
+        return x.v + a + b + c + d + e + f + g + h + i + j + k;
+    };
+    noInline(result);
+    return result;
+}
+
+var o = new MyObject(42);
+var z = new MyObject(0);
+var five = new MyObject(5);
+
+for (var i = 0; i < 100000; ++i) {
+    var result = bar(o, i)(true, false);
+    if (result != 42 + 11 * i + 55)
+        throw "Error: bad result: " + result;
+}
+

Modified: trunk/Source/_javascript_Core/ChangeLog (179456 => 179457)


--- trunk/Source/_javascript_Core/ChangeLog	2015-02-01 03:11:14 UTC (rev 179456)
+++ trunk/Source/_javascript_Core/ChangeLog	2015-02-01 03:58:39 UTC (rev 179457)
@@ -1,3 +1,18 @@
+2015-01-31  Michael Saboff  <msab...@apple.com>
+
+        Crash (DFG assertion) beneath AbstractInterpreter::verifyEdge() @ http://experilous.com/1/planet-generator/2014-09-28/version-1
+        https://bugs.webkit.org/show_bug.cgi?id=141111
+
+        Reviewed by Filip Pizlo.
+
+        In LowerDFGToLLVM::compileNode(), if we determine while compiling a node that we would have
+        exited, we don't need to process the OSR availability or abstract interpreter.
+
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::safelyInvalidateAfterTermination): Broke this out a a separate
+        method since we need to call it at the top and near the bottom of compileNode().
+        (JSC::FTL::LowerDFGToLLVM::compileNode):
+
 2015-01-31  Sam Weinig  <s...@webkit.org>
 
         Remove even more Mountain Lion support

Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp (179456 => 179457)


--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp	2015-02-01 03:11:14 UTC (rev 179456)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToLLVM.cpp	2015-02-01 03:58:39 UTC (rev 179457)
@@ -306,30 +306,34 @@
                 break;
         }
     }
-    
+
+    void safelyInvalidateAfterTermination()
+    {
+        if (verboseCompilationEnabled())
+            dataLog("Bailing.\n");
+        crash(m_highBlock->index, m_node->index());
+
+        // Invalidate dominated blocks. Under normal circumstances we would expect
+        // them to be invalidated already. But you can have the CFA become more
+        // precise over time because the structures of objects change on the main
+        // thread. Failing to do this would result in weird crashes due to a value
+        // being used but not defined. Race conditions FTW!
+        for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
+            BasicBlock* target = m_graph.block(blockIndex);
+            if (!target)
+                continue;
+            if (m_graph.m_dominators.dominates(m_highBlock, target)) {
+                if (verboseCompilationEnabled())
+                    dataLog("Block ", *target, " will bail also.\n");
+                target->cfaHasVisited = false;
+            }
+        }
+    }
+
     bool compileNode(unsigned nodeIndex)
     {
         if (!m_state.isValid()) {
-            if (verboseCompilationEnabled())
-                dataLog("Bailing.\n");
-            crash(m_highBlock->index, m_node->index());
-            
-            // Invalidate dominated blocks. Under normal circumstances we would expect
-            // them to be invalidated already. But you can have the CFA become more
-            // precise over time because the structures of objects change on the main
-            // thread. Failing to do this would result in weird crashes due to a value
-            // being used but not defined. Race conditions FTW!
-            for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
-                BasicBlock* target = m_graph.block(blockIndex);
-                if (!target)
-                    continue;
-                if (m_graph.m_dominators.dominates(m_highBlock, target)) {
-                    if (verboseCompilationEnabled())
-                        dataLog("Block ", *target, " will bail also.\n");
-                    target->cfaHasVisited = false;
-                }
-            }
-            
+            safelyInvalidateAfterTermination();
             return false;
         }
         
@@ -749,7 +753,12 @@
             DFG_CRASH(m_graph, m_node, "Unrecognized node in FTL backend");
             break;
         }
-        
+
+        if (!m_state.isValid()) {
+            safelyInvalidateAfterTermination();
+            return false;
+        }
+
         m_availabilityCalculator.executeNode(m_node);
         m_interpreter.executeEffects(nodeIndex);
         
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to