Title: [204360] trunk
Revision
204360
Author
[email protected]
Date
2016-08-10 16:19:49 -0700 (Wed, 10 Aug 2016)

Log Message

DFG's flushForTerminal() needs to add PhantomLocals for bytecode live locals.
https://bugs.webkit.org/show_bug.cgi?id=160755
<rdar://problem/27488507>

Reviewed by Filip Pizlo.

JSTests:

* stress/need-bytecode-liveness-for-unreachable-blocks-at-dfg-time.js: Added.

Source/_javascript_Core:

If the DFG sees that an inlined function will result in an OSR exit every time,
it will treat all downstream blocks as dead.  However, it still needs to keep
locals that are alive in the bytecode alive for the compiled function so that
those locals are properly written to the stack by the OSR exit ramp.

The existing code neglected to do this.  This patch remedies this issue.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::flushDirect):
(JSC::DFG::ByteCodeParser::addFlushOrPhantomLocal):
(JSC::DFG::ByteCodeParser::phantomLocalDirect):
(JSC::DFG::ByteCodeParser::flushForTerminal):

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (204359 => 204360)


--- trunk/JSTests/ChangeLog	2016-08-10 22:37:47 UTC (rev 204359)
+++ trunk/JSTests/ChangeLog	2016-08-10 23:19:49 UTC (rev 204360)
@@ -1,3 +1,13 @@
+2016-08-10  Mark Lam  <[email protected]>
+
+        DFG's flushForTerminal() needs to add PhantomLocals for bytecode live locals.
+        https://bugs.webkit.org/show_bug.cgi?id=160755
+        <rdar://problem/27488507>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/need-bytecode-liveness-for-unreachable-blocks-at-dfg-time.js: Added.
+
 2016-08-09  Skachkov Oleksandr  <[email protected]>
 
         [ES2016] Implement Object.values

Added: trunk/JSTests/stress/need-bytecode-liveness-for-unreachable-blocks-at-dfg-time.js (0 => 204360)


--- trunk/JSTests/stress/need-bytecode-liveness-for-unreachable-blocks-at-dfg-time.js	                        (rev 0)
+++ trunk/JSTests/stress/need-bytecode-liveness-for-unreachable-blocks-at-dfg-time.js	2016-08-10 23:19:49 UTC (rev 204360)
@@ -0,0 +1,31 @@
+//@ run("--useConcurrentJIT=false")
+
+// This test is set up delicately to:
+// 1. cause the test() function to DFG compile with just the right amount of profiling
+//    so that ...
+// 2. the DFG identifies the "return Function()" path as dead, and ...
+// 3. the DFG compiled function doesn't OSR exit too many times before ...
+// 4. we change the implementation of the inlined foo() and execute test() again.
+// 
+// This test should not crash.
+
+eval("\"use strict\"; var w;");
+foo = function() { throw 0; }
+var x;
+
+(function() {
+    eval("test = function() { ~foo(~(0 ? ~x : x) ? 0 : 0); return Function(); }");
+})();
+
+// This loop count of 2000 was empirically determined to be the right amount to get this
+// this issue to manifest.  Dropping or raising it may mask the issue and prevent it from
+// manifesting.
+for (var i = 0; i < 2000; ++i) {
+    try {
+        test();
+    } catch(e) {
+    }
+}
+
+foo = function() { };
+test();

Modified: trunk/Source/_javascript_Core/ChangeLog (204359 => 204360)


--- trunk/Source/_javascript_Core/ChangeLog	2016-08-10 22:37:47 UTC (rev 204359)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-08-10 23:19:49 UTC (rev 204360)
@@ -1,3 +1,24 @@
+2016-08-10  Mark Lam  <[email protected]>
+
+        DFG's flushForTerminal() needs to add PhantomLocals for bytecode live locals.
+        https://bugs.webkit.org/show_bug.cgi?id=160755
+        <rdar://problem/27488507>
+
+        Reviewed by Filip Pizlo.
+
+        If the DFG sees that an inlined function will result in an OSR exit every time,
+        it will treat all downstream blocks as dead.  However, it still needs to keep
+        locals that are alive in the bytecode alive for the compiled function so that
+        those locals are properly written to the stack by the OSR exit ramp.
+
+        The existing code neglected to do this.  This patch remedies this issue.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::flushDirect):
+        (JSC::DFG::ByteCodeParser::addFlushOrPhantomLocal):
+        (JSC::DFG::ByteCodeParser::phantomLocalDirect):
+        (JSC::DFG::ByteCodeParser::flushForTerminal):
+
 2016-08-09  Skachkov Oleksandr  <[email protected]>
 
         [ES2016] Implement Object.values

Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (204359 => 204360)


--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2016-08-10 22:37:47 UTC (rev 204359)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2016-08-10 23:19:49 UTC (rev 204360)
@@ -573,9 +573,15 @@
     {
         flushDirect(operand, findArgumentPosition(operand));
     }
-    
+
     void flushDirect(VirtualRegister operand, ArgumentPosition* argumentPosition)
     {
+        addFlushOrPhantomLocal<Flush>(operand, argumentPosition);
+    }
+
+    template<NodeType nodeType>
+    void addFlushOrPhantomLocal(VirtualRegister operand, ArgumentPosition* argumentPosition)
+    {
         ASSERT(!operand.isConstant());
         
         Node* node = m_currentBlock->variablesAtTail.operand(operand);
@@ -587,12 +593,17 @@
         else
             variable = newVariableAccessData(operand);
         
-        node = addToGraph(Flush, OpInfo(variable));
+        node = addToGraph(nodeType, OpInfo(variable));
         m_currentBlock->variablesAtTail.operand(operand) = node;
         if (argumentPosition)
             argumentPosition->addVariable(variable);
     }
-    
+
+    void phantomLocalDirect(VirtualRegister operand)
+    {
+        addFlushOrPhantomLocal<PhantomLocal>(operand, findArgumentPosition(operand));
+    }
+
     void flush(InlineStackEntry* inlineStackEntry)
     {
         int numArguments;
@@ -615,8 +626,32 @@
 
     void flushForTerminal()
     {
-        for (InlineStackEntry* inlineStackEntry = m_inlineStackTop; inlineStackEntry; inlineStackEntry = inlineStackEntry->m_caller)
+        CodeOrigin origin = currentCodeOrigin();
+        unsigned bytecodeIndex = origin.bytecodeIndex;
+
+        for (InlineStackEntry* inlineStackEntry = m_inlineStackTop; inlineStackEntry; inlineStackEntry = inlineStackEntry->m_caller) {
             flush(inlineStackEntry);
+
+            ASSERT(origin.inlineCallFrame == inlineStackEntry->m_inlineCallFrame);
+            InlineCallFrame* inlineCallFrame = inlineStackEntry->m_inlineCallFrame;
+            CodeBlock* codeBlock = m_graph.baselineCodeBlockFor(inlineCallFrame);
+            FullBytecodeLiveness& fullLiveness = m_graph.livenessFor(codeBlock);
+            const FastBitVector& livenessAtBytecode = fullLiveness.getLiveness(bytecodeIndex);
+
+            for (unsigned local = codeBlock->m_numCalleeLocals; local--;) {
+                if (livenessAtBytecode.get(local)) {
+                    VirtualRegister reg = virtualRegisterForLocal(local);
+                    if (inlineCallFrame)
+                        reg = inlineStackEntry->remapOperand(reg);
+                    phantomLocalDirect(reg);
+                }
+            }
+
+            if (inlineCallFrame) {
+                bytecodeIndex = inlineCallFrame->directCaller.bytecodeIndex;
+                origin = inlineCallFrame->directCaller;
+            }
+        }
     }
 
     void flushForReturn()
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to