Ensure flow control commands don't remove internal breakpoints set by real breakpoints

A real breakpoint makes entries in the breakpoint filter for all offsets at the given line. Various flow control commands will also use the breakpoint filter to add and remove temporary "internal breakpoints" required during the flow operation. Ensure that we never remove a breakpoint filter entry if there was already one there due to a breakpoint (or really  any other reason).

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);
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 }}

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;
 };

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;
   }
 }
 

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
 };
 

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);

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() {

hphp/test/quick/debugger/flow_break_interference.php

@@ -0,0 +1,33 @@
+<?php
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+function foo($a) {
+  return $a + 2;
+}
+
+function genFoo($a) {
+  $a = foo($a);
+  $z = yield $a+5;
+  // Step out from this line will cause us to handle an iternext, with
+  // a fall-thru destination of the first opcode of the line after the
+  // foreach in main(). If flow control interferes with breakpoints,
+  // then we'll execute that opcode (load $a) before stopping.
+  yield $z+1;
+  error_log('Finished in genFoo');
+}
+
+function main() {
+
+  $a = 42;
+  foreach (genFoo(1) as $x) {
+    var_dump($x);
+  }
+  // If we're really stopped at the beginning of this line, then
+  // chaning $a will show in $z.
+  $z = $a;
+  var_dump($z);
+}
+
+main();
+
+

hphp/test/quick/debugger/flow_break_interference.php.expectf

@@ -0,0 +1,45 @@
+Program %s/flow_break_interference.php loaded. Type '[r]un' or '[c]ontinue' to go.
+b flow_break_interference.php:15
+Breakpoint 1 set on line 15 of flow_break_interference.php
+But wont break until file flow_break_interference.php has been loaded.
+b flow_break_interference.php:27
+Breakpoint 2 set on line 27 of flow_break_interference.php
+But wont break until file flow_break_interference.php has been loaded.
+c
+int(8)
+Breakpoint 1 reached at genFoo$continuation() on line 15 of %s/flow_break_interference.php
+  14   // then we'll execute that opcode (load $a) before stopping.
+  15   yield $z+1;
+  16   error_log('Finished in genFoo');
+
+b c 1
+Breakpoint 1 cleared on line 15 of flow_break_interference.php
+o
+Break at main() on line 23 of %s/flow_break_interference.php
+  22   foreach (genFoo(1) as $x) {
+  23     var_dump($x);
+  24   }
+
+c
+int(1)
+Finished in genFoo
+Breakpoint 2 reached at main() on line 27 of %s/flow_break_interference.php
+  26   // chaning $a will show in $z.
+  27   $z = $a;
+  28   var_dump($z);
+
+p $a
+42
+$a = 7
+
+n
+Break at main() on line 28 of %s/flow_break_interference.php
+  27   $z = $a;
+  28   var_dump($z);
+  29 }
+
+p $z
+7
+p $a
+7
+q

hphp/test/quick/debugger/flow_break_interference.php.in

@@ -0,0 +1,13 @@
+b flow_break_interference.php:15
+b flow_break_interference.php:27
+c
+b c 1
+o
+c
+p $a
+$a = 7
+n
+p $z
+p $a
+q
+