CRASH in operationCreateDirectArgumentsDuringExit()
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Apr 2015 05:19:07 +0000 (05:19 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Apr 2015 05:19:07 +0000 (05:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=143962

Reviewed by Geoffrey Garen.

We shouldn't assume that constant-like OSR exit values are always recoverable. They are only
recoverable so long as they are live. Therefore, OSR exit should track liveness of
constants instead of assuming that they are always live.

* dfg/DFGGenerationInfo.h:
(JSC::DFG::GenerationInfo::noticeOSRBirth):
(JSC::DFG::GenerationInfo::appendBirth):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileCurrentBlock):
* dfg/DFGVariableEvent.cpp:
(JSC::DFG::VariableEvent::dump):
* dfg/DFGVariableEvent.h:
(JSC::DFG::VariableEvent::birth):
(JSC::DFG::VariableEvent::id):
(JSC::DFG::VariableEvent::dataFormat):
* dfg/DFGVariableEventStream.cpp:
(JSC::DFG::VariableEventStream::reconstruct):
* tests/stress/phantom-direct-arguments-clobber-argument-count.js: Added.
(foo):
(bar):
* tests/stress/phantom-direct-arguments-clobber-callee.js: Added.
(foo):
(bar):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGGenerationInfo.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGVariableEvent.cpp
Source/JavaScriptCore/dfg/DFGVariableEvent.h
Source/JavaScriptCore/dfg/DFGVariableEventStream.cpp
Source/JavaScriptCore/tests/stress/phantom-direct-arguments-clobber-argument-count.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/phantom-direct-arguments-clobber-callee.js [new file with mode: 0644]

index 3229f99..7460251 100644 (file)
@@ -1,3 +1,34 @@
+2015-04-24  Filip Pizlo  <fpizlo@apple.com>
+
+        CRASH in operationCreateDirectArgumentsDuringExit()
+        https://bugs.webkit.org/show_bug.cgi?id=143962
+
+        Reviewed by Geoffrey Garen.
+        
+        We shouldn't assume that constant-like OSR exit values are always recoverable. They are only
+        recoverable so long as they are live. Therefore, OSR exit should track liveness of
+        constants instead of assuming that they are always live.
+
+        * dfg/DFGGenerationInfo.h:
+        (JSC::DFG::GenerationInfo::noticeOSRBirth):
+        (JSC::DFG::GenerationInfo::appendBirth):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileCurrentBlock):
+        * dfg/DFGVariableEvent.cpp:
+        (JSC::DFG::VariableEvent::dump):
+        * dfg/DFGVariableEvent.h:
+        (JSC::DFG::VariableEvent::birth):
+        (JSC::DFG::VariableEvent::id):
+        (JSC::DFG::VariableEvent::dataFormat):
+        * dfg/DFGVariableEventStream.cpp:
+        (JSC::DFG::VariableEventStream::reconstruct):
+        * tests/stress/phantom-direct-arguments-clobber-argument-count.js: Added.
+        (foo):
+        (bar):
+        * tests/stress/phantom-direct-arguments-clobber-callee.js: Added.
+        (foo):
+        (bar):
+
 2015-04-24  Benjamin Poulain  <bpoulain@apple.com>
 
         [JSC] When inserting a NaN into a Int32 array, we convert it to DoubleArray then to ContiguousArray
index ac2b6d3..b30f350 100644 (file)
@@ -153,8 +153,6 @@ public:
     
     void noticeOSRBirth(VariableEventStream& stream, Node* node, VirtualRegister virtualRegister)
     {
-        if (m_isConstant)
-            return;
         if (m_node != node)
             return;
         if (!alive())
@@ -164,7 +162,9 @@ public:
         
         m_bornForOSR = true;
         
-        if (m_registerFormat != DataFormatNone)
+        if (m_isConstant)
+            appendBirth(stream);
+        else if (m_registerFormat != DataFormatNone)
             appendFill(BirthToFill, stream);
         else if (m_spillFormat != DataFormatNone)
             appendSpill(BirthToSpill, stream, virtualRegister);
@@ -379,6 +379,11 @@ public:
     }
 
 private:
+    void appendBirth(VariableEventStream& stream)
+    {
+        stream.appendAndLog(VariableEvent::birth(MinifiedID(m_node)));
+    }
+    
     void appendFill(VariableEventKind kind, VariableEventStream& stream)
     {
         ASSERT(m_bornForOSR);
index 1c7dde5..6a13659 100644 (file)
@@ -1428,33 +1428,29 @@ void SpeculativeJIT::compileCurrentBlock()
         m_codeOriginForExitTarget = m_currentNode->origin.forExit;
         m_codeOriginForExitProfile = m_currentNode->origin.semantic;
         m_lastGeneratedNode = m_currentNode->op();
-        if (!m_currentNode->shouldGenerate()) {
-            if (belongsInMinifiedGraph(m_currentNode->op()))
-                m_minifiedGraph->append(MinifiedNode::fromNode(m_currentNode));
-        } else {
-            if (verboseCompilationEnabled()) {
-                dataLogF(
-                    "SpeculativeJIT generating Node @%d (bc#%u) at JIT offset 0x%x",
-                    (int)m_currentNode->index(),
-                    m_currentNode->origin.semantic.bytecodeIndex, m_jit.debugOffset());
-                dataLog("\n");
-            }
-            
-            compile(m_currentNode);
-
+        
+        ASSERT(m_currentNode->shouldGenerate());
+        
+        if (verboseCompilationEnabled()) {
+            dataLogF(
+                "SpeculativeJIT generating Node @%d (bc#%u) at JIT offset 0x%x",
+                (int)m_currentNode->index(),
+                m_currentNode->origin.semantic.bytecodeIndex, m_jit.debugOffset());
+            dataLog("\n");
+        }
+        
+        compile(m_currentNode);
+        
+        if (belongsInMinifiedGraph(m_currentNode->op()))
+            m_minifiedGraph->append(MinifiedNode::fromNode(m_currentNode));
+        
 #if ENABLE(DFG_REGISTER_ALLOCATION_VALIDATION)
-            m_jit.clearRegisterAllocationOffsets();
+        m_jit.clearRegisterAllocationOffsets();
 #endif
-
-            if (!m_compileOkay) {
-                bail(DFGBailedAtEndOfNode);
-                return;
-            }
-            
-            if (belongsInMinifiedGraph(m_currentNode->op())) {
-                m_minifiedGraph->append(MinifiedNode::fromNode(m_currentNode));
-                noticeOSRBirth(m_currentNode);
-            }
+        
+        if (!m_compileOkay) {
+            bail(DFGBailedAtEndOfNode);
+            return;
         }
         
         // Make sure that the abstract state is rematerialized for the next node.
index a012ed1..28e437f 100644 (file)
@@ -46,6 +46,9 @@ void VariableEvent::dump(PrintStream& out) const
     case BirthToSpill:
         dumpSpillInfo("BirthToSpill", out);
         break;
+    case Birth:
+        out.print("Birth(", id(), ")");
+        break;
     case Fill:
         dumpFillInfo("Fill", out);
         break;
index 9e76e55..5fa4bb6 100644 (file)
@@ -47,6 +47,7 @@ enum VariableEventKind {
     // that we start to care about this node.
     BirthToFill,
     BirthToSpill,
+    Birth,
     
     // Events related to how a node is represented.
     Fill,
@@ -133,6 +134,14 @@ public:
         return event;
     }
     
+    static VariableEvent birth(MinifiedID id)
+    {
+        VariableEvent event;
+        event.m_which.id = id.bits();
+        event.m_kind = Birth;
+        return event;
+    }
+    
     static VariableEvent spill(VariableEventKind kind, MinifiedID id, VirtualRegister virtualRegister, DataFormat format)
     {
         ASSERT(kind == BirthToSpill || kind == Spill);
@@ -179,17 +188,17 @@ public:
     
     MinifiedID id() const
     {
-        ASSERT(m_kind == BirthToFill || m_kind == Fill
-               || m_kind == BirthToSpill || m_kind == Spill
-               || m_kind == Death || m_kind == MovHintEvent);
+        ASSERT(
+            m_kind == BirthToFill || m_kind == Fill || m_kind == BirthToSpill || m_kind == Spill
+            || m_kind == Death || m_kind == MovHintEvent || m_kind == Birth);
         return MinifiedID::fromBits(m_which.id);
     }
     
     DataFormat dataFormat() const
     {
-        ASSERT(m_kind == BirthToFill || m_kind == Fill
-               || m_kind == BirthToSpill || m_kind == Spill
-               || m_kind == SetLocalEvent);
+        ASSERT(
+            m_kind == BirthToFill || m_kind == Fill || m_kind == BirthToSpill || m_kind == Spill
+            || m_kind == SetLocalEvent);
         return static_cast<DataFormat>(m_dataFormat);
     }
     
index 50212fa..e3f413c 100644 (file)
@@ -48,11 +48,14 @@ namespace {
 
 struct MinifiedGenerationInfo {
     bool filled; // true -> in gpr/fpr/pair, false -> spilled
+    bool alive;
     VariableRepresentation u;
     DataFormat format;
     
     MinifiedGenerationInfo()
-        : format(DataFormatNone)
+        : filled(false)
+        , alive(false)
+        , format(DataFormatNone)
     {
     }
     
@@ -62,13 +65,19 @@ struct MinifiedGenerationInfo {
         case BirthToFill:
         case Fill:
             filled = true;
+            alive = true;
             break;
         case BirthToSpill:
         case Spill:
             filled = false;
+            alive = true;
             break;
+        case Birth:
+            alive = true;
+            return;
         case Death:
             format = DataFormatNone;
+            alive = false;
             return;
         default:
             return;
@@ -146,7 +155,8 @@ void VariableEventStream::reconstruct(
             // nothing to do.
             break;
         case BirthToFill:
-        case BirthToSpill: {
+        case BirthToSpill:
+        case Birth: {
             MinifiedGenerationInfo info;
             info.update(event);
             generationInfos.add(event.id(), info);
@@ -185,14 +195,14 @@ void VariableEventStream::reconstruct(
         
         ASSERT(source.kind() == HaveNode);
         MinifiedNode* node = graph.at(source.id());
-        if (tryToSetConstantRecovery(valueRecoveries[i], node))
-            continue;
-        
         MinifiedGenerationInfo info = generationInfos.get(source.id());
-        if (info.format == DataFormatNone) {
+        if (!info.alive) {
             valueRecoveries[i] = ValueRecovery::constant(jsUndefined());
             continue;
         }
+
+        if (tryToSetConstantRecovery(valueRecoveries[i], node))
+            continue;
         
         ASSERT(info.format != DataFormatNone);
         
diff --git a/Source/JavaScriptCore/tests/stress/phantom-direct-arguments-clobber-argument-count.js b/Source/JavaScriptCore/tests/stress/phantom-direct-arguments-clobber-argument-count.js
new file mode 100644 (file)
index 0000000..eb50477
--- /dev/null
@@ -0,0 +1,20 @@
+function foo() {
+    return effectful42.apply(this, arguments);
+}
+
+function bar(a, b) {
+    var result = foo.apply(this, b);
+    return [a, a, a, a, a, a, a, a, result + a];
+}
+
+noInline(bar);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = "" + bar(1, []);
+    if (result != "1,1,1,1,1,1,1,1,43")
+        throw "Error: bad result: " + result;
+}
+
+var result = "" + bar(2147483647, []);
+if (result != "2147483647,2147483647,2147483647,2147483647,2147483647,2147483647,2147483647,2147483647,2147483689")
+    throw "Error: bad result at end: " + result;
diff --git a/Source/JavaScriptCore/tests/stress/phantom-direct-arguments-clobber-callee.js b/Source/JavaScriptCore/tests/stress/phantom-direct-arguments-clobber-callee.js
new file mode 100644 (file)
index 0000000..70588f8
--- /dev/null
@@ -0,0 +1,21 @@
+function foo() {
+    return function() { return effectful42.apply(this, arguments) };
+}
+noInline(foo);
+
+function bar(a) {
+    var result = foo()();
+    return [result, result, result, result, result, result, result, result, result + a];
+}
+
+noInline(bar);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = "" + bar(1);
+    if (result != "42,42,42,42,42,42,42,42,43")
+        throw "Error: bad result: " + result;
+}
+
+var result = "" + bar(2147483647);
+if (result != "42,42,42,42,42,42,42,42,2147483689")
+    throw "Error: bad result at end: " + result;