ASSERTION FAILED: node->op() == Phi || node->op() == SetArgument
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Mar 2014 01:50:41 +0000 (01:50 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Mar 2014 01:50:41 +0000 (01:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=130069

Reviewed by Geoffrey Garen.

This was a great assertion, and it represents our strictest interpretation of the rules of
our intermediate representation. However, fixing DCE to actually preserve the relevant
property would be hard, and it wouldn't have an observable effect right now because nobody
actually uses the propery of CPS that this assertion is checking for.

In particular, we do always require, and rely on, the fact that non-captured variables
have variablesAtTail refer to the last interesting use of the variable: a SetLocal if the
block assigns to the variable, a GetLocal if it only reads from it, and a Flush,
PhantomLocal, or Phi otherwise. We do preserve this property successfully and DCE was not
broken in this regard. But, in the strictest sense, CPS also means that for captured
variables, variablesAtTail also continues to point to the last relevant use of the
variable. In particular, if there are multiple GetLocals, then it should point to the last
one. This is hard for DCE to preserve. Also, nobody relies on variablesAtTail for captured
variables, except to check the VariableAccessData; but in that case, we don't really need
the *last* relevant use of the variable - any node that mentions the same variable will do
just fine.

So, this change loosens the assertion and adds a detailed FIXME describing what we would
have to do if we wanted to preserve the more strict property.

This also makes changes to various debug printing paths so that validation doesn't crash
during graph dump. This also adds tests for the interesting cases of DCE failing to
preserve CPS in the strictest sense. This also attempts to win the record for longest test
name.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::hashAsStringIfPossible):
(JSC::CodeBlock::dumpAssumingJITType):
* bytecode/CodeBlock.h:
* bytecode/CodeOrigin.cpp:
(JSC::InlineCallFrame::hashAsStringIfPossible):
(JSC::InlineCallFrame::dumpBriefFunctionInformation):
* bytecode/CodeOrigin.h:
* dfg/DFGCPSRethreadingPhase.cpp:
(JSC::DFG::CPSRethreadingPhase::run):
* dfg/DFGDCEPhase.cpp:
(JSC::DFG::DCEPhase::cleanVariables):
* dfg/DFGInPlaceAbstractState.cpp:
(JSC::DFG::InPlaceAbstractState::mergeStateAtTail):
* runtime/FunctionExecutableDump.cpp:
(JSC::FunctionExecutableDump::dump):
* tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js: Added.
(foo):
* tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store.js: Added.
(foo):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@165522 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/bytecode/CodeOrigin.cpp
Source/JavaScriptCore/bytecode/CodeOrigin.h
Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp
Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
Source/JavaScriptCore/runtime/FunctionExecutableDump.cpp
Source/JavaScriptCore/tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store.js [new file with mode: 0644]

index ec6afa7..d5fffe9 100644 (file)
@@ -1,3 +1,56 @@
+2014-03-11  Filip Pizlo  <fpizlo@apple.com>
+
+        ASSERTION FAILED: node->op() == Phi || node->op() == SetArgument
+        https://bugs.webkit.org/show_bug.cgi?id=130069
+
+        Reviewed by Geoffrey Garen.
+        
+        This was a great assertion, and it represents our strictest interpretation of the rules of
+        our intermediate representation. However, fixing DCE to actually preserve the relevant
+        property would be hard, and it wouldn't have an observable effect right now because nobody
+        actually uses the propery of CPS that this assertion is checking for.
+        
+        In particular, we do always require, and rely on, the fact that non-captured variables
+        have variablesAtTail refer to the last interesting use of the variable: a SetLocal if the
+        block assigns to the variable, a GetLocal if it only reads from it, and a Flush,
+        PhantomLocal, or Phi otherwise. We do preserve this property successfully and DCE was not
+        broken in this regard. But, in the strictest sense, CPS also means that for captured
+        variables, variablesAtTail also continues to point to the last relevant use of the
+        variable. In particular, if there are multiple GetLocals, then it should point to the last
+        one. This is hard for DCE to preserve. Also, nobody relies on variablesAtTail for captured
+        variables, except to check the VariableAccessData; but in that case, we don't really need
+        the *last* relevant use of the variable - any node that mentions the same variable will do
+        just fine.
+        
+        So, this change loosens the assertion and adds a detailed FIXME describing what we would
+        have to do if we wanted to preserve the more strict property.
+        
+        This also makes changes to various debug printing paths so that validation doesn't crash
+        during graph dump. This also adds tests for the interesting cases of DCE failing to
+        preserve CPS in the strictest sense. This also attempts to win the record for longest test
+        name.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::hashAsStringIfPossible):
+        (JSC::CodeBlock::dumpAssumingJITType):
+        * bytecode/CodeBlock.h:
+        * bytecode/CodeOrigin.cpp:
+        (JSC::InlineCallFrame::hashAsStringIfPossible):
+        (JSC::InlineCallFrame::dumpBriefFunctionInformation):
+        * bytecode/CodeOrigin.h:
+        * dfg/DFGCPSRethreadingPhase.cpp:
+        (JSC::DFG::CPSRethreadingPhase::run):
+        * dfg/DFGDCEPhase.cpp:
+        (JSC::DFG::DCEPhase::cleanVariables):
+        * dfg/DFGInPlaceAbstractState.cpp:
+        (JSC::DFG::InPlaceAbstractState::mergeStateAtTail):
+        * runtime/FunctionExecutableDump.cpp:
+        (JSC::FunctionExecutableDump::dump):
+        * tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js: Added.
+        (foo):
+        * tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store.js: Added.
+        (foo):
+
 2014-03-12  Brian Burg  <bburg@apple.com>
 
         Web Replay: add infrastructure for memoizing nondeterministic DOM APIs
index 95fbe38..8433b36 100644 (file)
@@ -129,13 +129,16 @@ CString CodeBlock::sourceCodeOnOneLine() const
     return reduceWhitespace(sourceCodeForTools());
 }
 
-void CodeBlock::dumpAssumingJITType(PrintStream& out, JITCode::JITType jitType) const
+CString CodeBlock::hashAsStringIfPossible() const
 {
-    out.print(inferredName(), "#");
     if (hasHash() || isSafeToComputeHash())
-        out.print(hash());
-    else
-        out.print("<no-hash>");
+        return toCString(hash());
+    return "<no-hash>";
+}
+
+void CodeBlock::dumpAssumingJITType(PrintStream& out, JITCode::JITType jitType) const
+{
+    out.print(inferredName(), "#", hashAsStringIfPossible());
     out.print(":[", RawPointer(this), "->");
     if (!!m_alternative)
         out.print(RawPointer(m_alternative.get()), "->");
index 09d1854..a470d4b 100644 (file)
@@ -117,6 +117,7 @@ public:
     CodeBlockHash hash() const;
     bool hasHash() const;
     bool isSafeToComputeHash() const;
+    CString hashAsStringIfPossible() const;
     CString sourceCodeForTools() const; // Not quite the actual source we parsed; this will do things like prefix the source for a function with a reified signature.
     CString sourceCodeOnOneLine() const; // As sourceCodeForTools(), but replaces all whitespace runs with a single space.
     void dumpAssumingJITType(PrintStream&, JITCode::JITType) const;
index eb13cf3..7ec1ce2 100644 (file)
@@ -152,6 +152,12 @@ CodeBlockHash InlineCallFrame::hash() const
         specializationKind())->hash();
 }
 
+CString InlineCallFrame::hashAsStringIfPossible() const
+{
+    return jsCast<FunctionExecutable*>(executable.get())->codeBlockFor(
+        specializationKind())->hashAsStringIfPossible();
+}
+
 CString InlineCallFrame::inferredName() const
 {
     return jsCast<FunctionExecutable*>(executable.get())->inferredName().utf8();
@@ -164,7 +170,7 @@ CodeBlock* InlineCallFrame::baselineCodeBlock() const
 
 void InlineCallFrame::dumpBriefFunctionInformation(PrintStream& out) const
 {
-    out.print(inferredName(), "#", hash());
+    out.print(inferredName(), "#", hashAsStringIfPossible());
 }
 
 void InlineCallFrame::dumpInContext(PrintStream& out, DumpContext* context) const
index c525179..edce543 100644 (file)
@@ -151,6 +151,7 @@ struct InlineCallFrame {
     
     CString inferredName() const;
     CodeBlockHash hash() const;
+    CString hashAsStringIfPossible() const;
     
     CodeBlock* baselineCodeBlock() const;
     
index d83a471..e98cde2 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -44,6 +44,8 @@ public:
     
     bool run()
     {
+        RELEASE_ASSERT(m_graph.m_refCountState == EverythingIsLive);
+        
         if (m_graph.m_form == ThreadedCPS)
             return false;
         
index b0ba669..d3e1f4f 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -259,7 +259,39 @@ private:
                 continue;
             if (node->op() == GetLocal) {
                 node = node->child1().node();
-                ASSERT(node->op() == Phi || node->op() == SetArgument);
+                
+                // FIXME: In the case that the variable is captured, we really want to be able
+                // to replace the variable-at-tail with the last use of the variable in the same
+                // way that CPS rethreading would do. The child of the GetLocal isn't necessarily
+                // the same as what CPS rethreading would do. For example, we may have:
+                //
+                // a: SetLocal(...) // live
+                // b: GetLocal(@a) // live
+                // c: GetLocal(@a) // dead
+                //
+                // When killing @c, the code below will set the variable-at-tail to @a, while CPS
+                // rethreading would have set @b. This is a benign bug, since all clients of CPS
+                // only use the variable-at-tail of captured variables to get the
+                // VariableAccessData and observe that it is in fact captured. But, this feels
+                // like it could cause bugs in the future.
+                //
+                // It's tempting to just dethread and then invoke CPS rethreading, but CPS
+                // rethreading fails to preserve exact ref-counts. So we would need a fixpoint.
+                // It's probably the case that this fixpoint will be guaranteed to converge after
+                // the second iteration (i.e. the second run of DCE will not kill anything and so
+                // will not need to dethread), but for now the safest approach is probably just to
+                // allow for this tiny bit of sloppiness.
+                //
+                // Another possible solution would be to simply say that DCE dethreads but then
+                // we never rethread before going to the backend. That feels intuitively right
+                // because it's unlikely that any of the phases after DCE in the backend rely on
+                // ThreadedCPS.
+                //
+                // https://bugs.webkit.org/show_bug.cgi?id=130115
+                ASSERT(
+                    node->op() == Phi || node->op() == SetArgument
+                    || node->variableAccessData()->isCaptured());
+                
                 if (node->shouldGenerate()) {
                     variables[i] = node;
                     continue;
index 83e3d30..ac94307 100644 (file)
@@ -254,7 +254,7 @@ bool InPlaceAbstractState::mergeStateAtTail(AbstractValue& destination, Abstract
     if (node->variableAccessData()->isCaptured()) {
         // If it's captured then we know that whatever value was stored into the variable last is the
         // one we care about. This is true even if the variable at tail is dead, which might happen if
-        // the last thing we did to the variable was a GetLocal and then ended up now using the
+        // the last thing we did to the variable was a GetLocal and then ended up not using the
         // GetLocal's result.
         
         source = inVariable;
index d0583fc..6c6dba7 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2013, 2014 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -34,12 +34,12 @@ void FunctionExecutableDump::dump(PrintStream& out) const
 {
     out.print(m_executable->inferredName().string(), "#");
     if (m_executable->isGeneratedForCall())
-        out.print(m_executable->codeBlockForCall()->hash());
+        out.print(m_executable->codeBlockForCall()->hashAsStringIfPossible());
     else
         out.print("<nogen>");
     out.print("/");
     if (m_executable->isGeneratedForConstruct())
-        out.print(m_executable->codeBlockForConstruct()->hash());
+        out.print(m_executable->codeBlockForConstruct()->hashAsStringIfPossible());
     else
         out.print("<nogen>");
     out.print(":[", RawPointer(m_executable), "]");
diff --git a/Source/JavaScriptCore/tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js b/Source/JavaScriptCore/tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store-in-function-with-multiple-basic-blocks.js
new file mode 100644 (file)
index 0000000..ab8e3f3
--- /dev/null
@@ -0,0 +1,26 @@
+function foo(p) {
+    if (p) {
+        var x = 42;
+        (function() { x = 43; })();
+        x++;
+        var realResult = x;
+        (function() { x = 44; })();
+        var fakeResult = x;
+        return realResult;
+    }
+    var y = 45;
+    (function() { y = 46; })();
+    y++;
+    var realResult2 = y;
+    (function() { y = 47; })();
+    var fakeResult2 = y;
+    return realResult2;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = foo(i & 1);
+    if (result != ((i & 1) ? 44 : 47))
+        throw "Error: bad result with i = " + i + ": " + result;
+}
diff --git a/Source/JavaScriptCore/tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store.js b/Source/JavaScriptCore/tests/stress/dead-access-to-captured-variable-preceded-by-a-live-store.js
new file mode 100644 (file)
index 0000000..effa6f2
--- /dev/null
@@ -0,0 +1,17 @@
+function foo() {
+    var x = 42;
+    (function() { x = 43; })();
+    x++;
+    var realResult = x;
+    (function() { x = 44; })();
+    var fakeResult = x;
+    return realResult;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = foo();
+    if (result != 44)
+        throw "Error: bad result: " + result;
+}