diff --git a/hphp/runtime/debugger/cmd/cmd_flow_control.cpp b/hphp/runtime/debugger/cmd/cmd_flow_control.cpp index 3c65f70b6..fe26f3c2e 100644 --- a/hphp/runtime/debugger/cmd/cmd_flow_control.cpp +++ b/hphp/runtime/debugger/cmd/cmd_flow_control.cpp @@ -23,9 +23,8 @@ namespace HPHP { namespace Eval { TRACE_SET_MOD(debuggerflow); CmdFlowControl::~CmdFlowControl() { - // Remove any location filter or step outs that may have been setup. + // Remove any location filter that may have been setup. removeLocationFilter(); - cleanupStepOuts(); } void CmdFlowControl::sendImpl(DebuggerThriftBuffer &thrift) { @@ -123,12 +122,11 @@ void CmdFlowControl::removeLocationFilter() { } bool CmdFlowControl::hasStepOuts() { - return m_stepOutUnit != nullptr; + return m_stepOut1.valid() || m_stepOut2.valid(); } bool CmdFlowControl::atStepOutOffset(Unit* unit, Offset o) { - return (unit == m_stepOutUnit) && - ((o == m_stepOutOffset1) || (o == m_stepOutOffset2)); + return m_stepOut1.at(unit, o) || m_stepOut2.at(unit, o); } // Place internal breakpoints to get out of the current function. This may place @@ -165,50 +163,95 @@ void CmdFlowControl::setupStepOuts() { assert(!isSwitch(*returnPC) && (numSuccs((Op*)returnPC) <= 2)); // Set an internal breakpoint after the instruction if it can fall thru. if (instrAllowsFallThru(toOp(*returnPC))) { - m_stepOutUnit = returnUnit; - m_stepOutOffset1 = returnOffset + instrLen((Op*)returnPC); + Offset nextOffset = returnOffset + instrLen((Op*)returnPC); TRACE(2, "CmdFlowControl: step out to '%s' offset %d (fall-thru)\n", - fp->m_func->fullName()->data(), m_stepOutOffset1); - phpAddBreakPoint(m_stepOutUnit, m_stepOutOffset1); + fp->m_func->fullName()->data(), nextOffset); + m_stepOut1 = StepDestination(returnUnit, nextOffset); } // Set an internal breakpoint at the target of a control flow instruction. // A good example of a control flow op that invokes PHP is IterNext. if (instrIsControlFlow(toOp(*returnPC))) { Offset target = instrJumpTarget((Op*)returnPC, 0); if (target != InvalidAbsoluteOffset) { - m_stepOutUnit = returnUnit; - m_stepOutOffset2 = returnOffset + target; + Offset targetOffset = returnOffset + target; TRACE(2, "CmdFlowControl: step out to '%s' offset %d (jump target)\n", - fp->m_func->fullName()->data(), m_stepOutOffset2); - phpAddBreakPoint(m_stepOutUnit, m_stepOutOffset2); + fp->m_func->fullName()->data(), targetOffset); + m_stepOut2 = StepDestination(returnUnit, targetOffset); } } // If we have no place to step out to, then unwind another frame and try // again. The most common case that leads here is Ret*, which does not // fall-thru and has no encoded target. } else { - m_stepOutUnit = returnUnit; - m_stepOutOffset1 = returnOffset; TRACE(2, "CmdFlowControl: step out to '%s' offset %d\n", - fp->m_func->fullName()->data(), m_stepOutOffset1); - phpAddBreakPoint(m_stepOutUnit, m_stepOutOffset1); + fp->m_func->fullName()->data(), returnOffset); + m_stepOut1 = StepDestination(returnUnit, returnOffset); } } } void CmdFlowControl::cleanupStepOuts() { - if (m_stepOutUnit) { - if (m_stepOutOffset1 != InvalidAbsoluteOffset) { - phpRemoveBreakPoint(m_stepOutUnit, m_stepOutOffset1); - m_stepOutOffset1 = InvalidAbsoluteOffset; - } - if (m_stepOutOffset2 != InvalidAbsoluteOffset) { - phpRemoveBreakPoint(m_stepOutUnit, m_stepOutOffset2); - m_stepOutOffset2 = InvalidAbsoluteOffset; - } - m_stepOutUnit = nullptr; + if (m_stepOut1.valid()) { + m_stepOut1 = StepDestination(); + } + if (m_stepOut2.valid()) { + m_stepOut2 = StepDestination(); } } +/////////////////////////////////////////////////////////////////////////////// +// StepDestination +// +// NB: a StepDestination also manages an internal breakpoint at the +// given location. If there is already an internal breakpoint set when +// a StepDestination is constructed then it will not remove the +// breakpoint when it is destructed. The move assignment operator +// handles the transfer of ownership, and we delete the copy +// constructor/assignment operators explictly to ensure no two +// StepDestinations believe they can remove the same internal +// breakpoint. +// +// We can manage internal breakpoints without a true refcount like +// this because no other operation can manipulate the breakpoint +// filter while a single flow control command is active, and because +// the lifetime of a StepDestination is scoped by the lifetime of its +// flow control command. + +CmdFlowControl::StepDestination::StepDestination() : + m_unit(nullptr), m_offset(InvalidAbsoluteOffset), + m_ownsInternalBreakpoint(false) +{ +} + +CmdFlowControl::StepDestination::StepDestination(const Unit* unit, + Offset offset) : + m_unit(unit), m_offset(offset) +{ + m_ownsInternalBreakpoint = !phpHasBreakpoint(m_unit, m_offset); + if (m_ownsInternalBreakpoint) phpAddBreakPoint(m_unit, m_offset); +} + +CmdFlowControl::StepDestination::StepDestination(StepDestination&& other) { + *this = std::move(other); +} + +CmdFlowControl::StepDestination& +CmdFlowControl::StepDestination::operator=(StepDestination&& other) { + if (this != &other) { + if (m_ownsInternalBreakpoint) phpRemoveBreakPoint(m_unit, m_offset); + m_unit = other.m_unit; + m_offset = other.m_offset; + m_ownsInternalBreakpoint = other.m_ownsInternalBreakpoint; + other.m_unit = nullptr; + other.m_offset = InvalidAbsoluteOffset; + other.m_ownsInternalBreakpoint = false; + } + return *this; +} + +CmdFlowControl::StepDestination::~StepDestination() { + if (m_ownsInternalBreakpoint) phpRemoveBreakPoint(m_unit, m_offset); +} + /////////////////////////////////////////////////////////////////////////////// }} diff --git a/hphp/runtime/debugger/cmd/cmd_flow_control.h b/hphp/runtime/debugger/cmd/cmd_flow_control.h index 0c90f7edb..de5c64931 100644 --- a/hphp/runtime/debugger/cmd/cmd_flow_control.h +++ b/hphp/runtime/debugger/cmd/cmd_flow_control.h @@ -59,9 +59,7 @@ class CmdFlowControl : public DebuggerCommand { public: explicit CmdFlowControl(Type type) : DebuggerCommand(type), m_complete(false), m_needsVMInterrupt(false), - m_stackDepth(0), m_vmDepth(0), m_stepOutUnit(nullptr), - m_stepOutOffset1(InvalidAbsoluteOffset), - m_stepOutOffset2(InvalidAbsoluteOffset), m_count(1) { } + m_stackDepth(0), m_vmDepth(0), m_count(1) { } virtual ~CmdFlowControl(); virtual bool onServer(DebuggerProxy &proxy); @@ -101,11 +99,32 @@ protected: int m_vmDepth; std::string m_loc; // last break's source location - HPHP::Unit *m_stepOutUnit; - Offset m_stepOutOffset1; - Offset m_stepOutOffset2; + // Represents the destination of an internal stepping operation by + // unit and offset. Implictly maintains the breakpoint filter. + class StepDestination { + public: + StepDestination(); + StepDestination(const HPHP::Unit* unit, Offset offset); + StepDestination(const StepDestination& other) = delete; + StepDestination& operator=(const StepDestination& other) = delete; + StepDestination(StepDestination&& other); + StepDestination& operator=(StepDestination&& other); + ~StepDestination(); + + bool valid() const { return m_unit != nullptr; } + bool at(const Unit* unit, Offset o) const { + return (m_unit == unit) && (m_offset == o); + } + + private: + const HPHP::Unit* m_unit; + Offset m_offset; + bool m_ownsInternalBreakpoint; + }; private: + StepDestination m_stepOut1; + StepDestination m_stepOut2; int16_t m_count; bool m_smallStep; }; diff --git a/hphp/runtime/debugger/cmd/cmd_next.cpp b/hphp/runtime/debugger/cmd/cmd_next.cpp index 43fb1c1ef..b29e39dbb 100644 --- a/hphp/runtime/debugger/cmd/cmd_next.cpp +++ b/hphp/runtime/debugger/cmd/cmd_next.cpp @@ -24,10 +24,6 @@ namespace HPHP { namespace Eval { TRACE_SET_MOD(debuggerflow); -CmdNext::~CmdNext() { - cleanupStepCont(); -} - void CmdNext::help(DebuggerClient& client) { client.helpTitle("Next Command"); client.helpCmds( @@ -199,11 +195,11 @@ void CmdNext::stepCurrentLine(CmdInterrupt& interrupt, ActRec* fp, PC pc) { } bool CmdNext::hasStepCont() { - return m_stepContUnit != nullptr; + return m_stepCont.valid(); } bool CmdNext::atStepContOffset(Unit* unit, Offset o) { - return (unit == m_stepContUnit) && (o == m_stepContOffset); + return m_stepCont.at(unit, o); } // A ContSuspend is followed by code to support ContRaise, then code for @@ -220,22 +216,16 @@ void CmdNext::setupStepCont(ActRec* fp, PC pc) { assert(ops[3] == OpThrow); // One byte assert(ops[4] == OpNull); // One byte Offset nextInst = fp->m_func->unit()->offsetOf(pc + 5); - m_stepContUnit = fp->m_func->unit(); - m_stepContOffset = nextInst; m_stepContTag = getContinuationTag(fp); TRACE(2, "CmdNext: patch for cont step at '%s' offset %d\n", fp->m_func->fullName()->data(), nextInst); - phpAddBreakPoint(m_stepContUnit, m_stepContOffset); + m_stepCont = StepDestination(fp->m_func->unit(), nextInst); } void CmdNext::cleanupStepCont() { - if (m_stepContUnit) { - if (m_stepContOffset != InvalidAbsoluteOffset) { - phpRemoveBreakPoint(m_stepContUnit, m_stepContOffset); - m_stepContOffset = InvalidAbsoluteOffset; - } + if (m_stepCont.valid()) { + m_stepCont = StepDestination(); m_stepContTag = nullptr; - m_stepContUnit = nullptr; } } diff --git a/hphp/runtime/debugger/cmd/cmd_next.h b/hphp/runtime/debugger/cmd/cmd_next.h index 3c3897d7e..ffb2c5224 100644 --- a/hphp/runtime/debugger/cmd/cmd_next.h +++ b/hphp/runtime/debugger/cmd/cmd_next.h @@ -25,11 +25,7 @@ namespace HPHP { namespace Eval { DECLARE_BOOST_TYPES(CmdNext); class CmdNext : public CmdFlowControl { public: - CmdNext() : CmdFlowControl(KindOfNext), - m_stepContUnit(nullptr), - m_stepContOffset(InvalidAbsoluteOffset), - m_stepContTag(nullptr) {} - virtual ~CmdNext(); + CmdNext() : CmdFlowControl(KindOfNext), m_stepContTag(nullptr) {} virtual void help(DebuggerClient& client); virtual void onSetup(DebuggerProxy& proxy, CmdInterrupt& interrupt); @@ -43,8 +39,7 @@ private: void cleanupStepCont(); void* getContinuationTag(ActRec* fp); - HPHP::Unit* m_stepContUnit; - Offset m_stepContOffset; + StepDestination m_stepCont; void* m_stepContTag; // Unique identifier for the continuation we're stepping }; diff --git a/hphp/runtime/vm/debugger_hook.cpp b/hphp/runtime/vm/debugger_hook.cpp index 1983e26c5..f44b98d1d 100644 --- a/hphp/runtime/vm/debugger_hook.cpp +++ b/hphp/runtime/vm/debugger_hook.cpp @@ -252,6 +252,14 @@ void phpRemoveBreakPoint(const Unit* unit, Offset offset) { } } +bool phpHasBreakpoint(const Unit* unit, Offset offset) { + if (g_vmContext->m_breakPointFilter) { + PC pc = unit->at(offset); + return g_vmContext->m_breakPointFilter->checkPC(pc); + } + return false; +} + void phpDebuggerEvalHook(const Func* f) { if (RuntimeOption::EvalJit) { blacklistFuncInJit(f); diff --git a/hphp/runtime/vm/debugger_hook.h b/hphp/runtime/vm/debugger_hook.h index 898e6c414..ba04171c4 100644 --- a/hphp/runtime/vm/debugger_hook.h +++ b/hphp/runtime/vm/debugger_hook.h @@ -54,6 +54,7 @@ void phpSetBreakPoints(Eval::DebuggerProxy* proxy); // Add/remove breakpoints at a specific offset. void phpAddBreakPoint(const Unit* unit, Offset offset); void phpRemoveBreakPoint(const Unit* unit, Offset offset); +bool phpHasBreakpoint(const Unit* unit, Offset offset); // Is this thread being debugged? inline bool isDebuggerAttached() { diff --git a/hphp/test/quick/debugger/flow_break_interference.php b/hphp/test/quick/debugger/flow_break_interference.php new file mode 100644 index 000000000..ed53fcc93 --- /dev/null +++ b/hphp/test/quick/debugger/flow_break_interference.php @@ -0,0 +1,33 @@ +