Changeset 182167 in webkit

Timestamp:

Mar 30, 2015 5:43:49 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

REGRESSION (r181993): inspector-protocol/debugger/setBreakpoint-dfg-and-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.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp (modified) (6 diffs)
• Source/JavaScriptCore/dfg/DFGGraph.cpp (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGGraph.h (modified) (2 diffs)
• Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp (modified) (1 diff)
• Source/JavaScriptCore/ftl/FTLCompile.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/TestExpectations

@@ -128 +128 @@
 
 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

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

@@ -148 +148 @@
         , m_haveBuiltOperandMaps(false)
         , m_currentInstruction(0)
+        , m_hasDebuggerEnabled(graph.hasDebuggerEnabled())
     {
         ASSERT(m_profiledBlock);
@@ -386 +387 @@
             if (argumentPosition)
                 flushDirect(operand, argumentPosition);
+            else if (m_hasDebuggerEnabled && operand == m_codeBlock->scopeRegister())
+                flush(operand);
         }
 
@@ -523 +526 @@
         int numArguments;
         if (InlineCallFrame* inlineCallFrame = inlineStackEntry->m_inlineCallFrame) {
+            ASSERT(!m_hasDebuggerEnabled);
             numArguments = inlineCallFrame->arguments.size();
             if (inlineCallFrame->isClosureCall)
@@ -532 +536 @@
         for (unsigned argument = numArguments; argument-- > 1;)
             flushDirect(inlineStackEntry->remapOperand(virtualRegisterForArgument(argument)));
+        if (m_hasDebuggerEnabled)
+            flush(m_codeBlock->scopeRegister());
     }
 
@@ -998 +1004 @@
     
     Instruction* m_currentInstruction;
+    bool m_hasDebuggerEnabled;
 };
 
@@ -1169 +1176 @@
         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) {

trunk/Source/JavaScriptCore/dfg/DFGGraph.cpp

@@ -75 +75 @@
     for (unsigned i = m_mustHandleValues.size(); i--;)
         m_mustHandleValues[i] = freezeFragile(plan.mustHandleValues[i]);
+
+    m_hasDebuggerEnabled = m_profiledBlock->globalObject()->hasDebugger()
+        || Options::forceDebuggerBytecodeGeneration();
 }
 

trunk/Source/JavaScriptCore/dfg/DFGGraph.h

@@ -710 +710 @@
         BasicBlock*, const char* file, int line, const char* function,
         const char* assertion);
-    
+
+    bool hasDebuggerEnabled() const { return m_hasDebuggerEnabled; }
+
     VM& m_vm;
     Plan& m_plan;
@@ -792 +794 @@
     PlanStage m_planStage { PlanStage::Initial };
     RefCountState m_refCountState;
+    bool m_hasDebuggerEnabled;
 private:
     

trunk/Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp

@@ -176 +176 @@
         // 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--;) {

trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp

@@ -323 +323 @@
                 inlineCallFrame->calleeRecovery.withLocalsOffset(localsOffset);
         }
+
+        if (graph.hasDebuggerEnabled())
+            codeBlock->setScopeRegister(codeBlock->scopeRegister() + localsOffset);
     }