When inserting Unreachable in byte code parser we need to flush all the right things
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jan 2018 23:21:18 +0000 (23:21 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 11 Jan 2018 23:21:18 +0000 (23:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181509
<rdar://problem/36423110>

Reviewed by Mark Lam.

JSTests:

* stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js: Added.

Source/JavaScriptCore:

I added code in r226655 that had its own mechanism for preserving liveness when
inserting Unreachable nodes after ForceOSRExit. There are two ways to preserve
liveness: PhantomLocal and Flush. Certain values *must* be flushed to the stack.
I got some of these values wrong, which was leading to a crash when recovering the
callee value from an inlined frame. Instead of making the same mistake and repeating
similar code again, this patch refactors this logic to be shared with the other
liveness preservation code in the DFG bytecode parser. This is what I should have
done in my initial patch.

* bytecode/InlineCallFrame.h:
(JSC::remapOperand):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::flushImpl):
(JSC::DFG::flushForTerminalImpl):
(JSC::DFG::ByteCodeParser::flush):
(JSC::DFG::ByteCodeParser::flushForTerminal):
(JSC::DFG::ByteCodeParser::parse):

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

JSTests/ChangeLog
JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/InlineCallFrame.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

index b9b6c9a1b71d467941228250f098b13e7187a112..82299334ccc0f9512410706a42842fa88db2dba3 100644 (file)
@@ -1,3 +1,13 @@
+2018-01-11  Saam Barati  <sbarati@apple.com>
+
+        When inserting Unreachable in byte code parser we need to flush all the right things
+        https://bugs.webkit.org/show_bug.cgi?id=181509
+        <rdar://problem/36423110>
+
+        Reviewed by Mark Lam.
+
+        * stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js: Added.
+
 2018-01-11  Saam Barati  <sbarati@apple.com>
 
         JITMathIC code in the FTL is wrong when code gets duplicated
diff --git a/JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js b/JSTests/stress/proper-flushing-when-we-insert-unreachable-after-force-exit-in-bytecode-parser.js
new file mode 100644 (file)
index 0000000..619461d
--- /dev/null
@@ -0,0 +1,27 @@
+function test(b, f) {
+    if (b)
+        return f(b);
+}
+noInline(test);
+
+function throwError(b) {
+    if (b) {
+        try {
+            throw new Error;
+        } catch(e) { }
+    }
+    return 2;
+}
+noInline(throwError);
+
+function makeFoo() {
+    return function foo(b) {
+        throwError(b);
+        OSRExit();
+    }
+}
+
+let foos = [makeFoo(), makeFoo()];
+for (let i = 0; i < 10000; ++i) {
+    test(!!(i%2), foos[((Math.random() * 100) | 0) % foos.length]);
+}
index 5def7341ce7145c6ea5c221c100e397bdf21e9d6..474e956be48244a94ebd0b0c08f386397c87f78d 100644 (file)
@@ -1,3 +1,29 @@
+2018-01-11  Saam Barati  <sbarati@apple.com>
+
+        When inserting Unreachable in byte code parser we need to flush all the right things
+        https://bugs.webkit.org/show_bug.cgi?id=181509
+        <rdar://problem/36423110>
+
+        Reviewed by Mark Lam.
+
+        I added code in r226655 that had its own mechanism for preserving liveness when
+        inserting Unreachable nodes after ForceOSRExit. There are two ways to preserve
+        liveness: PhantomLocal and Flush. Certain values *must* be flushed to the stack.
+        I got some of these values wrong, which was leading to a crash when recovering the
+        callee value from an inlined frame. Instead of making the same mistake and repeating
+        similar code again, this patch refactors this logic to be shared with the other
+        liveness preservation code in the DFG bytecode parser. This is what I should have
+        done in my initial patch.
+
+        * bytecode/InlineCallFrame.h:
+        (JSC::remapOperand):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::flushImpl):
+        (JSC::DFG::flushForTerminalImpl):
+        (JSC::DFG::ByteCodeParser::flush):
+        (JSC::DFG::ByteCodeParser::flushForTerminal):
+        (JSC::DFG::ByteCodeParser::parse):
+
 2018-01-11  Saam Barati  <sbarati@apple.com>
 
         JITMathIC code in the FTL is wrong when code gets duplicated
index 08c04b403c6ac94938e8b475b146d6dffad2e8df..66c3e3a6ca2c297c749a8f2379eaec87c0d6acba 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -257,6 +257,13 @@ inline void CodeOrigin::walkUpInlineStack(const Function& function)
     }
 }
 
+ALWAYS_INLINE VirtualRegister remapOperand(InlineCallFrame* inlineCallFrame, VirtualRegister reg)
+{
+    if (inlineCallFrame)
+        return VirtualRegister(reg.offset() + inlineCallFrame->stackOffset);
+    return reg;
+}
+
 } // namespace JSC
 
 namespace WTF {
index 7efe433f234ca92e8f952214d7abac0727765ac2..2acb3f15d4254d60b1439fc32886f742bfcdb35d 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -82,6 +82,51 @@ if (DFGByteCodeParserInternal::verbose && Options::verboseDFGBytecodeParsing())
 dataLog(__VA_ARGS__); \
 } while (false)
 
+template <typename F1, typename F2>
+static ALWAYS_INLINE void flushImpl(Graph& graph, InlineCallFrame* inlineCallFrame, const F1& addFlushDirect, const F2& addPhantomLocalDirect)
+{
+    int numArguments;
+    if (inlineCallFrame) {
+        ASSERT(!graph.hasDebuggerEnabled());
+        numArguments = inlineCallFrame->argumentsWithFixup.size();
+        if (inlineCallFrame->isClosureCall)
+            addFlushDirect(remapOperand(inlineCallFrame, VirtualRegister(CallFrameSlot::callee)));
+        if (inlineCallFrame->isVarargs())
+            addFlushDirect(remapOperand(inlineCallFrame, VirtualRegister(CallFrameSlot::argumentCount)));
+    } else
+        numArguments = graph.baselineCodeBlockFor(inlineCallFrame)->numParameters();
+
+    for (unsigned argument = numArguments; argument-- > 1;)
+        addFlushDirect(remapOperand(inlineCallFrame, virtualRegisterForArgument(argument)));
+
+    if (!inlineCallFrame && graph.needsFlushedThis())
+        addFlushDirect(remapOperand(inlineCallFrame, virtualRegisterForArgument(0)));
+    else
+        addPhantomLocalDirect(remapOperand(inlineCallFrame, virtualRegisterForArgument(0)));
+
+    if (graph.needsScopeRegister())
+        addFlushDirect(graph.m_codeBlock->scopeRegister());
+}
+
+template <typename F1, typename F2>
+static ALWAYS_INLINE void flushForTerminalImpl(Graph& graph, CodeOrigin origin, const F1& addFlushDirect, const F2& addPhantomLocalDirect)
+{
+    origin.walkUpInlineStack([&] (CodeOrigin origin) {
+        unsigned bytecodeIndex = origin.bytecodeIndex;
+        InlineCallFrame* inlineCallFrame = origin.inlineCallFrame;
+        flushImpl(graph, inlineCallFrame, addFlushDirect, addPhantomLocalDirect);
+
+        CodeBlock* codeBlock = graph.baselineCodeBlockFor(inlineCallFrame);
+        FullBytecodeLiveness& fullLiveness = graph.livenessFor(codeBlock);
+        const FastBitVector& livenessAtBytecode = fullLiveness.getLiveness(bytecodeIndex);
+
+        for (unsigned local = codeBlock->m_numCalleeLocals; local--;) {
+            if (livenessAtBytecode[local])
+                addPhantomLocalDirect(remapOperand(inlineCallFrame, virtualRegisterForLocal(local)));
+        }
+    });
+}
+
 // === ByteCodeParser ===
 //
 // This class is used to compile the dataflow graph from a CodeBlock.
@@ -561,55 +606,16 @@ private:
 
     void flush(InlineStackEntry* inlineStackEntry)
     {
-        int numArguments;
-        if (InlineCallFrame* inlineCallFrame = inlineStackEntry->m_inlineCallFrame) {
-            ASSERT(!m_hasDebuggerEnabled);
-            numArguments = inlineCallFrame->argumentsWithFixup.size();
-            if (inlineCallFrame->isClosureCall)
-                flushDirect(inlineStackEntry->remapOperand(VirtualRegister(CallFrameSlot::callee)));
-            if (inlineCallFrame->isVarargs())
-                flushDirect(inlineStackEntry->remapOperand(VirtualRegister(CallFrameSlot::argumentCount)));
-        } else
-            numArguments = inlineStackEntry->m_codeBlock->numParameters();
-        for (unsigned argument = numArguments; argument-- > 1;)
-            flushDirect(inlineStackEntry->remapOperand(virtualRegisterForArgument(argument)));
-        if (!inlineStackEntry->m_inlineCallFrame && m_graph.needsFlushedThis())
-            flushDirect(virtualRegisterForArgument(0));
-        else
-            phantomLocalDirect(virtualRegisterForArgument(0));
-
-        if (m_graph.needsScopeRegister())
-            flushDirect(m_codeBlock->scopeRegister());
+        auto addFlushDirect = [&] (VirtualRegister reg) { flushDirect(reg); };
+        auto addPhantomLocalDirect = [&] (VirtualRegister reg) { phantomLocalDirect(reg); };
+        flushImpl(m_graph, inlineStackEntry->m_inlineCallFrame, addFlushDirect, addPhantomLocalDirect);
     }
 
     void flushForTerminal()
     {
-        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[local]) {
-                    VirtualRegister reg = virtualRegisterForLocal(local);
-                    if (inlineCallFrame)
-                        reg = inlineStackEntry->remapOperand(reg);
-                    phantomLocalDirect(reg);
-                }
-            }
-
-            if (inlineCallFrame) {
-                bytecodeIndex = inlineCallFrame->directCaller.bytecodeIndex;
-                origin = inlineCallFrame->directCaller;
-            }
-        }
+        auto addFlushDirect = [&] (VirtualRegister reg) { flushDirect(reg); };
+        auto addPhantomLocalDirect = [&] (VirtualRegister reg) { phantomLocalDirect(reg); };
+        flushForTerminalImpl(m_graph, currentCodeOrigin(), addFlushDirect, addPhantomLocalDirect);
     }
 
     void flushForReturn()
@@ -6606,18 +6612,19 @@ void ByteCodeParser::parse()
                     block->resize(nodeIndex + 1);
 
                     insertionSet.insertNode(block->size(), SpecNone, ExitOK, endOrigin);
-                    m_graph.forAllLiveInBytecode(endOrigin.semantic, [&] (VirtualRegister operand) {
+
+                    auto insertLivenessPreservingOp = [&] (NodeType op, VirtualRegister operand) {
                         VariableAccessData* variable = mapping.operand(operand);
-                        if (!variable)
+                        if (!variable) {
                             variable = newVariableAccessData(operand);
-
-                        auto op = PhantomLocal;
-                        if ((m_graph.needsScopeRegister() && operand == m_codeBlock->scopeRegister())
-                            || (operand.isArgument() && (operand != virtualRegisterForArgument(0) || m_graph.needsFlushedThis()))) {
-                            op = Flush;
+                            mapping.operand(operand) = variable;
                         }
                         insertionSet.insertNode(block->size(), SpecNone, op, endOrigin, OpInfo(variable));
-                    });
+                    };
+                    auto addFlushDirect = [&] (VirtualRegister operand) { insertLivenessPreservingOp(Flush, operand); };
+                    auto addPhantomLocalDirect = [&] (VirtualRegister operand) { insertLivenessPreservingOp(PhantomLocal, operand); };
+
+                    flushForTerminalImpl(m_graph, endOrigin.semantic, addFlushDirect, addPhantomLocalDirect);
 
                     insertionSet.insertNode(block->size(), SpecNone, Unreachable, endOrigin);
                     insertionSet.execute(block);