REGRESSION (r181993): inspector-protocol/debugger/setBreakpoint-dfg-a…

…nd-modify-local.html crashes.

<https://webkit.org/b/143105>

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

With r181993, the DFG and FTL may elide the storing of the scope register.  As a result,
on OSR exits from DFG / FTL frames where this elision has take place, we may get baseline
JIT frames that may have its scope register not set.  The Debugger's current implementation
which relies on the scope register is not happy about this.  For example, this results in a
crash in the layout test inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local.html.

The fix is to disable inlining when the debugger is in use.  Also, we add Flush nodes to
ensure that the scope register value is flushed to the register in the stack frame.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::ByteCodeParser):
(JSC::DFG::ByteCodeParser::setLocal):
(JSC::DFG::ByteCodeParser::flush):
- Add code to flush the scope register.
(JSC::DFG::ByteCodeParser::inliningCost):
- Pretend that all codeBlocks are too expensive to inline if the debugger is in use, thereby
  disabling inlining whenever the debugger is in use.
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::Graph):
* dfg/DFGGraph.h:
(JSC::DFG::Graph::hasDebuggerEnabled):
* dfg/DFGStackLayoutPhase.cpp:
(JSC::DFG::StackLayoutPhase::run):
- Update the DFG codeBlock's scopeRegister since it can be moved during stack layout.
* ftl/FTLCompile.cpp:
(JSC::FTL::mmAllocateDataSection):
- Update the FTL codeBlock's scopeRegister since it can be moved during stack layout.

LayoutTests:

* TestExpectations:
- Undid test skipped in r182072.



Canonical link: https://commits.webkit.org/161267@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@182167 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog

@@ -1,3 +1,13 @@
+2015-03-30  Mark Lam  <mark.lam@apple.com>
+
+        REGRESSION (r181993): inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local.html crashes.
+        <https://webkit.org/b/143105>
+
+        Reviewed by Filip Pizlo.
+
+        * TestExpectations:
+        - Undid test skipped in r182072.
+
 2015-03-30  Chris Dumez  <cdumez@apple.com>
 
         Cached "Expires" header is not updated upon successful resource revalidation

LayoutTests/TestExpectations

@@ -127,7 +127,7 @@ webkit.org/b/141661 fast/text/emoji.html [ Failure ]
 webkit.org/b/142548 editing/selection/extend-by-character-007.html [ Failure ]
 
 webkit.org/b/128736 inspector-protocol/debugger/setBreakpoint-dfg.html [ Failure Pass ]
-webkit.org/b/143105 inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local.html [ Skip ] # Crashing
+webkit.org/b/134982 inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local.html [ Failure Pass ]
 
 # CSS Font Loading is not yet enabled on all platforms
 webkit.org/b/135390 fast/css/fontloader-download-error.html [ Skip ]

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,38 @@
+2015-03-30  Mark Lam  <mark.lam@apple.com>
+
+        REGRESSION (r181993): inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local.html crashes.
+        <https://webkit.org/b/143105>
+
+        Reviewed by Filip Pizlo.
+
+        With r181993, the DFG and FTL may elide the storing of the scope register.  As a result,
+        on OSR exits from DFG / FTL frames where this elision has take place, we may get baseline
+        JIT frames that may have its scope register not set.  The Debugger's current implementation
+        which relies on the scope register is not happy about this.  For example, this results in a
+        crash in the layout test inspector-protocol/debugger/setBreakpoint-dfg-and-modify-local.html.
+
+        The fix is to disable inlining when the debugger is in use.  Also, we add Flush nodes to
+        ensure that the scope register value is flushed to the register in the stack frame.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::ByteCodeParser):
+        (JSC::DFG::ByteCodeParser::setLocal):
+        (JSC::DFG::ByteCodeParser::flush):
+        - Add code to flush the scope register.
+        (JSC::DFG::ByteCodeParser::inliningCost):
+        - Pretend that all codeBlocks are too expensive to inline if the debugger is in use, thereby
+          disabling inlining whenever the debugger is in use.
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::Graph):
+        * dfg/DFGGraph.h:
+        (JSC::DFG::Graph::hasDebuggerEnabled):
+        * dfg/DFGStackLayoutPhase.cpp:
+        (JSC::DFG::StackLayoutPhase::run):
+        - Update the DFG codeBlock's scopeRegister since it can be moved during stack layout.
+        * ftl/FTLCompile.cpp:
+        (JSC::FTL::mmAllocateDataSection):
+        - Update the FTL codeBlock's scopeRegister since it can be moved during stack layout.
+
 2015-03-30  Michael Saboff  <msaboff@apple.com>
 
         Fix flakey float32-repeat-out-of-bounds.js and int8-repeat-out-of-bounds.js tests for ARM64

Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -147,6 +147,7 @@ class ByteCodeParser {
         , m_inlineStackTop(0)
         , m_haveBuiltOperandMaps(false)
         , m_currentInstruction(0)
+        , m_hasDebuggerEnabled(graph.hasDebuggerEnabled())
     {
         ASSERT(m_profiledBlock);
     }
@@ -385,6 +386,8 @@ class ByteCodeParser {
             ArgumentPosition* argumentPosition = findArgumentPositionForLocal(operand);
             if (argumentPosition)
                 flushDirect(operand, argumentPosition);
+            else if (m_hasDebuggerEnabled && operand == m_codeBlock->scopeRegister())
+                flush(operand);
         }
 
         VariableAccessData* variableAccessData = newVariableAccessData(operand);
@@ -522,6 +525,7 @@ class ByteCodeParser {
     {
         int numArguments;
         if (InlineCallFrame* inlineCallFrame = inlineStackEntry->m_inlineCallFrame) {
+            ASSERT(!m_hasDebuggerEnabled);
             numArguments = inlineCallFrame->arguments.size();
             if (inlineCallFrame->isClosureCall)
                 flushDirect(inlineStackEntry->remapOperand(VirtualRegister(JSStack::Callee)));
@@ -531,6 +535,8 @@ class ByteCodeParser {
             numArguments = inlineStackEntry->m_codeBlock->numParameters();
         for (unsigned argument = numArguments; argument-- > 1;)
             flushDirect(inlineStackEntry->remapOperand(virtualRegisterForArgument(argument)));
+        if (m_hasDebuggerEnabled)
+            flush(m_codeBlock->scopeRegister());
     }
 
     void flushForTerminal()
@@ -997,6 +1003,7 @@ class ByteCodeParser {
     StubInfoMap m_dfgStubInfos;
 
     Instruction* m_currentInstruction;
+    bool m_hasDebuggerEnabled;
 };
 
 #define NEXT_OPCODE(name) \
@@ -1168,6 +1175,12 @@ unsigned ByteCodeParser::inliningCost(CallVariant callee, int argumentCountInclu
     if (verbose)
         dataLog("Considering inlining ", callee, " into ", currentCodeOrigin(), "\n");
 
+    if (m_hasDebuggerEnabled) {
+        if (verbose)
+            dataLog("    Failing because the debugger is in use.\n");
+        return UINT_MAX;
+    }
+
     FunctionExecutable* executable = callee.functionExecutable();
     if (!executable) {
         if (verbose)

Source/JavaScriptCore/dfg/DFGGraph.cpp

@@ -74,6 +74,9 @@ Graph::Graph(VM& vm, Plan& plan, LongLivedState& longLivedState)
 
     for (unsigned i = m_mustHandleValues.size(); i--;)
         m_mustHandleValues[i] = freezeFragile(plan.mustHandleValues[i]);
+
+    m_hasDebuggerEnabled = m_profiledBlock->globalObject()->hasDebugger()
+        || Options::forceDebuggerBytecodeGeneration();
 }
 
 Graph::~Graph()

Source/JavaScriptCore/dfg/DFGGraph.h

@@ -709,7 +709,9 @@ class Graph : public virtual Scannable {
     NO_RETURN_DUE_TO_CRASH void handleAssertionFailure(
         BasicBlock*, const char* file, int line, const char* function,
         const char* assertion);
-
+
+    bool hasDebuggerEnabled() const { return m_hasDebuggerEnabled; }
+
     VM& m_vm;
     Plan& m_plan;
     CodeBlock* m_codeBlock;
@@ -791,6 +793,7 @@ class Graph : public virtual Scannable {
     UnificationState m_unificationState;
     PlanStage m_planStage { PlanStage::Initial };
     RefCountState m_refCountState;
+    bool m_hasDebuggerEnabled;
 private:
 
     void handleSuccessor(Vector<BasicBlock*, 16>& worklist, BasicBlock*, BasicBlock* successor);

Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp

@@ -175,7 +175,10 @@ class StackLayoutPhase : public Phase {
 
         // This register is never valid for DFG code blocks.
         codeBlock()->setActivationRegister(VirtualRegister());
-        codeBlock()->setScopeRegister(VirtualRegister());
+        if (LIKELY(!m_graph.hasDebuggerEnabled()))
+            codeBlock()->setScopeRegister(VirtualRegister());
+        else
+            codeBlock()->setScopeRegister(assign(allocation, codeBlock()->scopeRegister()));
 
         for (unsigned i = m_graph.m_inlineVariableData.size(); i--;) {
             InlineVariableData data = m_graph.m_inlineVariableData[i];

Source/JavaScriptCore/ftl/FTLCompile.cpp

@@ -322,6 +322,9 @@ static void fixFunctionBasedOnStackMaps(
             inlineCallFrame->calleeRecovery =
                 inlineCallFrame->calleeRecovery.withLocalsOffset(localsOffset);
         }
+
+        if (graph.hasDebuggerEnabled())
+            codeBlock->setScopeRegister(codeBlock->scopeRegister() + localsOffset);
     }
 
     MacroAssembler::Label stackOverflowException;