Add comments to some of the debugger code to aid in understanding it more clearly, plus minor code cleanup.

Add a lot of comments to the debugger based on my current understanding of it. These may change in the future as we learn more, but they're helpful right now.

Also moved a few small things around in the code to clarify their purpose or scope. I.e., making a few things private, renaming a few functions, etc. No real logic changes, though. Also minor dead code removal. Also a few lint errors.

hphp/runtime/base/types.h

@@ -392,7 +392,7 @@ public:
 
   RequestInjectionData()
     : cflagsPtr(nullptr), surprisePage(nullptr), started(0), timeoutSeconds(-1),
-      debugger(false), debuggerIdle(0), dummySandbox(false),
+      debugger(false), dummySandbox(false),
       debuggerIntr(false), coverage(false) {
   }
 
@@ -411,7 +411,6 @@ public:
   int timeoutSeconds;  // how many seconds to timeout
 
   bool debugger;       // whether there is a DebuggerProxy attached to me
-  int  debuggerIdle;   // skipping this many interrupts while proxy is idle
   bool dummySandbox;   // indicating it is from a dummy sandbox thread
   bool debuggerIntr;   // indicating we should force interrupt for debugger
   std::stack<void *> interrupts;   // CmdInterrupts this thread's handling
@@ -552,7 +551,7 @@ public:
     m_info->m_executing =
       builtin ? ThreadInfo::ExtensionFunctions : ThreadInfo::UserFunctions;
   }
-  ExecutionProfiler(ThreadInfo::Executing executing) {
+  explicit ExecutionProfiler(ThreadInfo::Executing executing) {
     m_info = ThreadInfo::s_threadInfo.getNoCheck();
     m_executing = m_info->m_executing;
     m_info->m_executing = executing;

hphp/runtime/eval/debugger/break_point.h

@@ -28,20 +28,16 @@ enum InterruptType {
   RequestStarted,
   RequestEnded,
   PSPEnded,
-  HardBreakPoint,
+  HardBreakPoint, // From f_hphpd_break().
   BreakPointReached,
   ExceptionThrown,
 };
 
+// Represents a site in the code, at the source level.
+// Note: this class currently has just one subclass, InterruptSiteVM, which will
+// be combined with this shortly.
 class InterruptSite {
 public:
-  InterruptSite(CVarRef e = null_variant, const char *cls = nullptr,
-                const char *function = nullptr, StringData *file = nullptr,
-                int line0 = 0, int char0 = 0, int line1 = 0, int char1 = 0)
-    : m_exception(e), m_class(cls), m_function(function), m_file(file),
-      m_line0(line0), m_char0(char0), m_line1(line1), m_char1(char1),
-      m_jumping(false) { }
-
   virtual const char *getFile() const = 0;
   virtual const char *getClass() const = 0;
   virtual const char *getFunction() const = 0;
@@ -61,6 +57,14 @@ public:
   void setJumping() { m_jumping = true;}
 
 protected:
+  explicit InterruptSite(CVarRef e = null_variant, const char *cls = nullptr,
+                         const char *function = nullptr,
+                         StringData *file = nullptr, int line0 = 0,
+                         int char0 = 0, int line1 = 0, int char1 = 0)
+    : m_exception(e), m_class(cls), m_function(function), m_file(file),
+      m_line0(line0), m_char0(char0), m_line1(line1), m_char1(char1),
+      m_jumping(false) { }
+
   Variant m_exception;
 
   // cached
@@ -78,9 +82,12 @@ protected:
   bool m_jumping;
 };
 
+// Forms an InterruptSite by looking at the current thread's current PC and
+// grabbing source data out of the corresponding Unit.
 class InterruptSiteVM : public InterruptSite {
 public:
-  InterruptSiteVM(bool hardBreakPoint = false, CVarRef e = null_variant);
+  explicit InterruptSiteVM(bool hardBreakPoint = false,
+                           CVarRef e = null_variant);
   virtual const char *getFile() const;
   virtual const char *getClass() const;
   virtual const char *getFunction() const;

hphp/runtime/eval/debugger/cmd/cmd_break.cpp

@@ -450,11 +450,6 @@ void CmdBreak::setClientOutput(DebuggerClient *client) {
 
 bool CmdBreak::onServer(DebuggerProxy *proxy) {
   if (m_body == "update") {
-    if (!m_bps.empty()) {
-      RequestInjectionData &rjdata =
-        ThreadInfo::s_threadInfo->m_reqInjectionData;
-      rjdata.debuggerIdle = 0;
-    }
     proxy->setBreakPoints(m_bps);
     m_breakpoints = &m_bps;
     return proxy->send(this);

hphp/runtime/eval/debugger/debugger.cpp

@@ -109,7 +109,6 @@ void Debugger::CleanupDummySandboxThreads() {
   s_debugger.cleanupDummySandboxThreads();
 }
 
-
 void Debugger::DebuggerSession(const DebuggerClientOptions& options,
                                const std::string& file, bool restart) {
   if (options.extension.empty()) {
@@ -163,14 +162,9 @@ void Debugger::InterruptPSPEnded(const char *url) {
   }
 }
 
-void Debugger::InterruptFileLine(InterruptSite &site) {
-  Interrupt(BreakPointReached, nullptr, &site);
-}
-
-void Debugger::InterruptHard(InterruptSite &site) {
-  Interrupt(HardBreakPoint, nullptr, &site);
-}
-
+// Called directly from exception handling to indicate a user error handler
+// failed to handle an exeption. NB: this is quite distinct from the hook called
+// from iopThrow named phpDebuggerExceptionHook().
 bool Debugger::InterruptException(CVarRef e) {
   if (RuntimeOption::EnableDebugger) {
     ThreadInfo *ti = ThreadInfo::s_threadInfo.getNoCheck();
@@ -182,19 +176,23 @@ bool Debugger::InterruptException(CVarRef e) {
   return true;
 }
 
+// Primary entrypoint for the debugger from the VM. Called in response to a host
+// of VM events that the debugger is interested in. The debugger will execute
+// any logic needed to handle the event, and will block below this to wait for
+// and process more commands from the debugger client. This function will only
+// return when the debugger is letting the thread continue execution, e.g., for
+// flow control command like continue, next, etc.
 void Debugger::Interrupt(int type, const char *program,
                          InterruptSite *site /* = NULL */,
                          const char *error /* = NULL */) {
   assert(RuntimeOption::EnableDebugger);
 
-  RequestInjectionData &rjdata = ThreadInfo::s_threadInfo->m_reqInjectionData;
-  if (rjdata.debuggerIdle > 0 && type == BreakPointReached) {
-    --rjdata.debuggerIdle;
-    return;
-  }
-
   DebuggerProxyPtr proxy = GetProxy();
   if (proxy) {
+    RequestInjectionData &rjdata = ThreadInfo::s_threadInfo->m_reqInjectionData;
+    // The proxy will only service an interrupt if we've previously setup some
+    // form of flow control command (steps, breakpoints, etc.) or if it's
+    // an interrupt related to something like the session or request.
     if (proxy->needInterrupt() || type != BreakPointReached) {
       // Interrupts may execute some PHP code, causing another interruption.
       std::stack<void *> &interrupts = rjdata.interrupts;
@@ -204,9 +202,13 @@ void Debugger::Interrupt(int type, const char *program,
       proxy->interrupt(cmd);
       interrupts.pop();
     }
+    // Some cmds requires us to interpret all instructions until the cmd
+    // completes. Setting this will ensure we stay out of JIT code and in the
+    // interpreter so phpDebuggerOpcodeHook has a chance to work.
     rjdata.debuggerIntr = proxy->needInterruptForNonBreak();
   } else {
-    // debugger clients are disconnected abnormally
+    // Debugger clients are disconnected abnormally, or this sandbox is not
+    // being debugged.
     if (type == SessionStarted || type == SessionEnded) {
       // for command line programs, we need this exception to exit from
       // the infinite execution loop
@@ -215,10 +217,24 @@ void Debugger::Interrupt(int type, const char *program,
   }
 }
 
+// Primary entrypoint from the set of "debugger hooks", which the VM calls in
+// response to various events. While this function is quite general wrt. the
+// type of interrupt, practically the type will be one of the following:
+//   - ExceptionThrown
+//   - BreakPointReached
+//   - HardBreakPoint
+//
+// Note: it is indeed a bit odd that interrupts due to single stepping come in
+// as "BreakPointReached". Currently this results in spurious work in the
+// debugger.
 void Debugger::InterruptVMHook(int type /* = BreakPointReached */,
                                CVarRef e /* = null_variant */) {
+  // Computing the interrupt site here pulls in more data from the Unit to
+  // describe the current execution point.
   InterruptSiteVM site(type == HardBreakPoint, e);
   if (!site.valid()) {
+    // An invalid site is missing something like an ActRec, a func, or a
+    // Unit. Currently the debugger has no action to take at such sites.
     return;
   }
   Interrupt(type, nullptr, &site);
@@ -331,11 +347,16 @@ void Debugger::unregisterSandbox(const StringData* sandboxId) {
 
 #define FOREACH_SANDBOX_THREAD_END()    } } }                          \
 
+// Ask every thread in this proxy's sandbox and the dummy sandbox to "stop".
+// Gaining control of these threads is the intention... the mechanism is to
+// force them all to start interpreting all of their code in an effort to gain
+// control in phpDebuggerOpcodeHook().
 void Debugger::requestInterrupt(DebuggerProxyPtr proxy) {
   const StringData* sid = StringData::GetStaticString(proxy->getSandboxId());
   FOREACH_SANDBOX_THREAD_BEGIN(sid, ti)
   ti->m_reqInjectionData.debuggerIntr = true;
   FOREACH_SANDBOX_THREAD_END()
+
   sid = StringData::GetStaticString(proxy->getDummyInfo().id());
   FOREACH_SANDBOX_THREAD_BEGIN(sid, ti)
   ti->m_reqInjectionData.debuggerIntr = true;

hphp/runtime/eval/debugger/debugger.h

@@ -25,6 +25,10 @@
 
 namespace HPHP { namespace Eval {
 ///////////////////////////////////////////////////////////////////////////////
+// The Debugger generally manages proxies, sandboxes, and is the inital handler
+// of interrupts from the VM. It associates VM threads with sandboxes, and
+// sandboxes with proxies. Interrupts get minimal handling before being handed
+// off to the proper proxy.
 
 class Debugger {
 public:
@@ -76,10 +80,8 @@ public:
   static void InterruptPSPEnded(const char *url);
 
   /**
-   * A new line of PHP code is reached from execution thread.
+   * Called when a user handler fails to handle an exception.
    */
-  static void InterruptFileLine(InterruptSite &site);
-  static void InterruptHard(InterruptSite &site);
   static bool InterruptException(CVarRef e);
 
   /**
@@ -157,9 +159,12 @@ public:
   ~DebuggerDummyEnv();
 };
 
+// Suppress the debugger's ability to get interrupted while executing PHP.
+// Primarily used to suppress debugger events while evaling PHP in response
+// to commands like print, or for expressions in conditional breakpoints.
 class EvalBreakControl {
 public:
-  EvalBreakControl(bool noBreak);
+  explicit EvalBreakControl(bool noBreak);
   ~EvalBreakControl();
 private:
   bool m_noBreakSave;

hphp/runtime/eval/debugger/debugger_command.h

@@ -23,10 +23,15 @@
 
 namespace HPHP { namespace Eval {
 ///////////////////////////////////////////////////////////////////////////////
+// DebuggerCommand is the base of all commands executed by the debugger. It
+// also represents the base binary communication format between DebuggerProxy
+// and DebuggerClient.
+//
+// Each command has serialization logic, plus client- and server-side logic.
+// Client-side logic is implemented in the onClient* methods, while server-side
+// is in the onServer* methods.
+//
 
-/**
- * Binary communication format between DebuggerProxy and DebuggerClient.
- */
 DECLARE_BOOST_TYPES(DebuggerCommand);
 class DebuggerCommand {
 public:
@@ -80,7 +85,7 @@ public:
                       const char *caller);
 
 public:
-  DebuggerCommand(Type type)
+  explicit DebuggerCommand(Type type)
     : m_type(type), m_version(0), m_exitInterrupt(false),
       m_incomplete(false) {}
 

hphp/runtime/eval/debugger/debugger_proxy.cpp

@@ -135,6 +135,8 @@ void DebuggerProxy::notifyDummySandbox() {
   m_dummySandbox->notifySignal(CmdSignal::SignalBreak);
 }
 
+// Hold the entire set of breakpoints, and sift breakpoints by function and
+// class name into separate containers for later.
 void DebuggerProxy::setBreakPoints(BreakPointInfoPtrVec &breakpoints) {
   WriteLock lock(m_breakMutex);
   m_breakpoints = breakpoints;
@@ -181,7 +183,7 @@ bool DebuggerProxy::couldBreakEnterFunc(const StringData* funcFullName) {
 void DebuggerProxy::getBreakClsMethods(
   std::vector<const StringData*>& classNames) {
   classNames.clear();
-  WriteLock lock(m_breakMutex);
+  WriteLock lock(m_breakMutex); // Write lock in case iteration causes a re-hash
   for (StringDataMap::const_iterator iter = m_breaksEnterClsMethod.begin();
        iter != m_breaksEnterClsMethod.end(); ++iter) {
     classNames.push_back(iter->first);
@@ -191,7 +193,7 @@ void DebuggerProxy::getBreakClsMethods(
 void DebuggerProxy::getBreakFuncs(
   std::vector<const StringData*>& funcFullNames) {
   funcFullNames.clear();
-  WriteLock lock(m_breakMutex);
+  WriteLock lock(m_breakMutex); // Write lock in case iteration causes a re-hash
   for (StringDataMap::const_iterator iter = m_breaksEnterFunc.begin();
        iter != m_breaksEnterFunc.end(); ++iter) {
     funcFullNames.push_back(iter->first);
@@ -207,7 +209,12 @@ bool DebuggerProxy::needInterruptForNonBreak() {
   return m_flow || m_signum != CmdSignal::SignalNone;
 }
 
+// Handle an interrupt from the VM. Note: some work for breakpoints has already
+// occured in DebuggerProxyVM::interrupt().
 void DebuggerProxy::interrupt(CmdInterrupt &cmd) {
+  // Wait until this thread is the thread this proxy wants to debug.
+  // NB: breakpoints and control flow checks happen here, too, and return false
+  // if we're not done with the flow, or not at a breakpoint, etc.
   if (!blockUntilOwn(cmd, true)) {
     return;
   }
@@ -255,6 +262,13 @@ void DebuggerProxy::startSignalThread() {
   m_signalThread.start();
 }
 
+// This gets it's own thread, and polls the client once per second to see if
+// there is a signal, i.e., if the user has pressed Ctrl-C, etc. If there is a
+// signal, it is passed as an interrupt to the proxy in an attempt to get other
+// threads in the sandbox to stop.
+//
+// If another thread in the sandbox fails to stop and consume the signal then
+// it will be passed to the dummy sandbox instead.
 void DebuggerProxy::pollSignal() {
   while (!m_stopped) {
     sleep(1);
@@ -271,13 +285,14 @@ void DebuggerProxy::pollSignal() {
 
     Lock lock(m_signalMutex);
 
+    // Send CmdSignal over to the client and wait for a response.
     CmdSignal cmd;
     if (!cmd.onServerD(this)) break; // on socket error
 
     DebuggerCommandPtr res;
     while (!DebuggerCommand::Receive(m_thrift, res,
                                      "DebuggerProxy::pollSignal()")) {
-      // we will wait forever until DebuggerClient sends us something
+      // We will wait forever until DebuggerClient sends us something.
     }
     if (!res) break;
 
@@ -371,6 +386,11 @@ DThreadInfoPtr DebuggerProxy::createThreadInfo(const std::string &desc) {
   return info;
 }
 
+// Waits until this thread is the one that the proxy considers the current
+// thread. This also check to see if the given cmd has any breakpoints or
+// flow control that we should stop for. Note: while stepping, pretty much all
+// of the stepping logic is handled below here and this will return false if
+// the stepping operation has not completed.
 bool DebuggerProxy::blockUntilOwn(CmdInterrupt &cmd, bool check) {
   int64_t self = cmd.getThreadId();
 
@@ -410,12 +430,17 @@ bool DebuggerProxy::blockUntilOwn(CmdInterrupt &cmd, bool check) {
   return true;
 }
 
+// Checks whether the cmd has any breakpoints that match the current Site.
+// Also returns true for cmds that have should always break.
 bool DebuggerProxy::checkBreakPoints(CmdInterrupt &cmd) {
   ReadLock lock(m_breakMutex);
   return cmd.shouldBreak(m_breakpoints);
 }
 
+// Check if we should stop due to flow control, breakpoints, and signals.
 bool DebuggerProxy::checkJumpFlowBreak(CmdInterrupt &cmd) {
+  // If there is an outstanding Ctrl-C from the client, go ahead and break now.
+  // Note: this stops any flow control command we might have in-flight.
   if (m_signum == CmdSignal::SignalBreak) {
     Lock lock(m_signumMutex);
     if (m_signum == CmdSignal::SignalBreak) {
@@ -493,15 +518,14 @@ void DebuggerProxy::checkStop() {
 }
 
 void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
+  // Do the server-side work for this cmd.
   if (!cmd.onServerD(this)) {
     Debugger::RemoveProxy(shared_from_this()); // on socket error
     return;
   }
 
-  // Once we sent an CmdInterrupt to client side, we should be considered idle
-  RequestInjectionData &rjdata = ThreadInfo::s_threadInfo->m_reqInjectionData;
-  rjdata.debuggerIdle = 0;
-
+  // Wait for commands from the debugger client and process them. We'll stay
+  // here until we get a command that should cause the thread to continue.
   while (true) {
     DebuggerCommandPtr res;
     while (!DebuggerCommand::Receive(m_thrift, res,
@@ -511,6 +535,7 @@ void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
     }
     checkStop();
     if (res) {
+      // Any control flow command gets installed here and we continue execution.
       m_flow = dynamic_pointer_cast<CmdFlowControl>(res);
       if (m_flow) {
         m_flow->onServerD(this);
@@ -537,6 +562,7 @@ void DebuggerProxy::processInterrupt(CmdInterrupt &cmd) {
       }
     }
     try {
+      // Perform the server-size work for this command.
       if (!res || !res->onServerD(this)) {
         Debugger::RemoveProxy(shared_from_this());
         return;
@@ -617,18 +643,18 @@ BreakPointInfoPtr DebuggerProxyVM::getBreakPointAtCmd(CmdInterrupt& cmd) {
   return BreakPointInfoPtr();
 }
 
-
+// Handle an interrupt from the VM.
 void DebuggerProxyVM::interrupt(CmdInterrupt &cmd) {
   changeBreakPointDepth(cmd);
-  if (cmd.getInterruptType() != BreakPointReached &&
-      cmd.getInterruptType() != HardBreakPoint) {
-    DebuggerProxy::interrupt(cmd);
-    return;
-  }
 
-  if (cmd.getInterruptType() != HardBreakPoint) {
+  if (cmd.getInterruptType() == BreakPointReached) {
     if (!needInterrupt()) return;
-    // Modify m_lastLocFilter to save current location
+
+    // NB: stepping is represented as a BreakPointReached interrupt.
+
+    // Modify m_lastLocFilter to save current location. This will short-circuit
+    // the work done up in phpDebuggerOpcodeHook() and ensure we don't break on
+    // this line until we're completely off of it.
     InterruptSiteVM *site = (InterruptSiteVM*)cmd.getSite();
     if (g_vmContext->m_lastLocFilter) {
       g_vmContext->m_lastLocFilter->clear();
@@ -647,7 +673,7 @@ void DebuggerProxyVM::interrupt(CmdInterrupt &cmd) {
     }
     g_vmContext->m_lastLocFilter->addRanges(site->getUnit(),
                                             site->getCurOffsetRange());
-    // if the breakpoint is not to be processed, we should continue execution
+    // If the breakpoint is not to be processed, we should continue execution.
     BreakPointInfoPtr bp = getBreakPointAtCmd(cmd);
     if (bp) {
       if (!bp->breakable(getRealStackDepth())) {
@@ -662,8 +688,8 @@ void DebuggerProxyVM::interrupt(CmdInterrupt &cmd) {
 }
 
 void DebuggerProxyVM::setBreakPoints(BreakPointInfoPtrVec& breakpoints) {
-  DebuggerProxy::setBreakPoints(breakpoints);
-  VM::phpBreakPointHook(this);
+  DebuggerProxy::setBreakPoints(breakpoints); // Hold bp's in the proxy.
+  VM::phpSetBreakPointsInAllFiles(this); // Apply breakpoints to the code.
 }
 
 void DebuggerProxyVM::readInjTablesFromThread() {
@@ -719,6 +745,7 @@ int DebuggerProxyVM::getStackDepth() {
   return depth;
 }
 
+// Handle a continue cmd, or setup stepping.
 void DebuggerProxyVM::processFlowControl(CmdInterrupt &cmd) {
   switch (m_flow->getType()) {
     case DebuggerCommand::KindOfContinue:
@@ -765,6 +792,8 @@ void DebuggerProxyVM::changeBreakPointDepth(CmdInterrupt& cmd) {
   }
 }
 
+// Determine if an outstanding flow control cmd has run it's course and we
+// should stop execution.
 bool DebuggerProxyVM::breakByFlowControl(CmdInterrupt &cmd) {
   switch (m_flow->getType()) {
     case DebuggerCommand::KindOfStep: {

hphp/runtime/eval/debugger/debugger_proxy.h

@@ -26,6 +26,24 @@
 
 namespace HPHP { namespace Eval {
 ///////////////////////////////////////////////////////////////////////////////
+// A DebuggerProxy provides a conection thru which a client may talk to a VM
+// which is being debugged. The VM can also send messages to the client via the
+// proxy, either in reponse to messages from the client, or to poll the client
+// for information.
+//
+// In an basic scenario where a client is debugging a remote VM, the VM will
+// create a proxy when the client connects (via DebuggerServer) and listen for
+// commands via this proxy. It will use this proxy when completing control flow
+// commands to interrupt the client. The client sends and receives messages over
+// a socket directly to this proxy. Thus we have:
+//
+//   Client <---> Proxy <---> VM
+//
+// The client always creates its own "local proxy", which allows debugging any
+// code running on the VM within the client. The two are easily confused.
+//
+// Note: currently DebuggerProxy has a single subclass, DebuggerProxyVM.
+// There used to be other subclasses, but they're gong now. These can be merged.
 
 class CmdInterrupt;
 DECLARE_BOOST_TYPES(DebuggerProxy);
@@ -48,9 +66,6 @@ public:
                             int frame);
 
 public:
-  DebuggerProxy(SmartPtr<Socket> socket, bool local);
-  virtual ~DebuggerProxy();
-
   bool isLocal() const { return m_local;}
 
   const char *getThreadType() const;
@@ -86,6 +101,9 @@ public:
   void forceQuit();
 
 protected:
+  DebuggerProxy(SmartPtr<Socket> socket, bool local);
+  virtual ~DebuggerProxy();
+
   bool m_stopped;
 
   bool m_local;
@@ -150,10 +168,11 @@ public:
   void setInjTables(HPHP::VM::InjectionTables* tables) { m_injTables = tables;}
   void readInjTablesFromThread();
   void writeInjTablesToThread();
+
+private:
   void changeBreakPointDepth(CmdInterrupt& cmd);
   BreakPointInfoPtr getBreakPointAtCmd(CmdInterrupt& cmd);
 
-private:
   int getStackDepth();
   int getRealStackDepth();
 

hphp/runtime/vm/bytecode.cpp

@@ -2618,7 +2618,7 @@ HPHP::Eval::PhpFile* VMExecutionContext::lookupPhpFile(StringData* path,
       rpath.get()->incRefCount();
       efile->incRef();
     }
-    DEBUGGER_ATTACHED_ONLY(phpFileLoadHook(efile));
+    DEBUGGER_ATTACHED_ONLY(phpDebuggerFileLoadHook(efile));
   }
   return efile;
 }
@@ -4452,7 +4452,7 @@ inline void OPTBLD_INLINE VMExecutionContext::iopThrow(PC& pc) {
 
   Object obj(c1->m_data.pobj);
   m_stack.popC();
-  DEBUGGER_ATTACHED_ONLY(phpExceptionHook(obj.get()));
+  DEBUGGER_ATTACHED_ONLY(phpDebuggerExceptionHook(obj.get()));
   throw obj;
 }
 
@@ -7173,7 +7173,7 @@ inline void VMExecutionContext::dispatchImpl(int numInstrs) {
 
 #define O(name, imm, pusph, pop, flags)                       \
   LabelDbg##name:                                             \
-    phpDebuggerHook(pc);                                      \
+    phpDebuggerOpcodeHook(pc);                                \
   LabelInst##name:                                            \
     INST_HOOK_PC(injTable, pc);                               \
   LabelCover##name:                                           \

hphp/runtime/vm/debugger_hook.cpp

@@ -36,22 +36,31 @@ static inline Transl::Translator* transl() {
   return Transl::Translator::Get();
 }
 
-void phpDebuggerHook(const uchar* pc) {
+// Hook called from the bytecode interpreter before every opcode executed while
+// a debugger is attached. The debugger may choose to hold the thread below
+// here and execute any number of commands from the client. Return from here
+// lets the opcode execute.
+void phpDebuggerOpcodeHook(const uchar* pc) {
   TRACE(5, "in phpDebuggerHook()\n");
+  // Short-circuit when we're doing things like evaling PHP for print command,
+  // or conditional breakpoints.
   if (UNLIKELY(g_vmContext->m_dbgNoBreak)) {
     TRACE(5, "NoBreak flag is on\n");
     return;
   }
+  // Short-circuit for cases where we're executing a line of code that we know
+  // we don't need an interrupt for, e.g., stepping over a line of code.
   if (UNLIKELY(g_vmContext->m_lastLocFilter != nullptr) &&
       g_vmContext->m_lastLocFilter->checkPC(pc)) {
     TRACE(5, "same location as last interrupt\n");
     return;
   }
+  // Are we hitting a breakpoint?
   if (LIKELY(g_vmContext->m_breakPointFilter == nullptr ||
       !g_vmContext->m_breakPointFilter->checkPC(pc))) {
     TRACE(5, "not in the PC range for any breakpoints\n");
     if (LIKELY(!DEBUGGER_FORCE_INTR)) {
-      // implies we left the location for last break;
+      // Implies we left the location for last break.
       delete g_vmContext->m_lastLocFilter;
       g_vmContext->m_lastLocFilter = nullptr;
       return;
@@ -62,7 +71,9 @@ void phpDebuggerHook(const uchar* pc) {
   TRACE(5, "out phpDebuggerHook()\n");
 }
 
-void phpExceptionHook(ObjectData* e) {
+// Hook called from iopThrow to signal that we are about to throw an exception.
+// NB: this does not hook any portion of exception unwind.
+void phpDebuggerExceptionHook(ObjectData* e) {
   TRACE(5, "in phpExceptionHook()\n");
   if (UNLIKELY(g_vmContext->m_dbgNoBreak)) {
     TRACE(5, "NoBreak flag is on\n");
@@ -76,6 +87,9 @@ bool isDebuggerAttachedProcess() {
   return Eval::Debugger::CountConnectedProxy() > 0;
 }
 
+// Ensure we interpret all code at the given offsets. This sets up a guard for
+// each piece of tranlated code to ensure we punt ot the interpreter when the
+// debugger is attached.
 static void blacklistRangesInJit(const Unit* unit,
                                  const OffsetRangeVec& offsets) {
   for (OffsetRangeVec::const_iterator it = offsets.begin();
@@ -90,6 +104,7 @@ static void blacklistRangesInJit(const Unit* unit,
   }
 }
 
+// Ensure we interpret an entire function when the debugger is attached.
 static void blacklistFuncInJit(const Func* f) {
   Unit* unit = f->unit();
   OffsetRangeVec ranges;
@@ -162,14 +177,16 @@ static void addBreakPointsClass(Eval::DebuggerProxy* proxy,
     }
   }
 }
-    
+
 void phpDebuggerEvalHook(const Func* f) {
   if (RuntimeOption::EvalJit) {
     blacklistFuncInJit(f);
   }
 }
 
-void phpFileLoadHook(Eval::PhpFile* efile) {
+// Hook called by the VM when a file is loaded. Gives the debugger a chance
+// to apply any pending breakpoints that might be in the file.
+void phpDebuggerFileLoadHook(Eval::PhpFile* efile) {
   Eval::DebuggerProxyPtr proxy = Eval::Debugger::GetProxy();
   if (!proxy) {
     return;
@@ -177,7 +194,7 @@ void phpFileLoadHook(Eval::PhpFile* efile) {
   addBreakPointsInFile(proxy.get(), efile);
 }
 
-void phpDefClassHook(const Class* cls) {
+void phpDebuggerDefClassHook(const Class* cls) {
   Eval::DebuggerProxyPtr proxy = Eval::Debugger::GetProxy();
   if (!proxy) {
     return;
@@ -185,15 +202,16 @@ void phpDefClassHook(const Class* cls) {
   addBreakPointsClass(proxy.get(), cls);
 }
 
-void phpDefFuncHook(const Func* func) {
+void phpDebuggerDefFuncHook(const Func* func) {
   Eval::DebuggerProxyPtr proxy = Eval::Debugger::GetProxy();
   if (proxy && proxy->couldBreakEnterFunc(func->fullName())) {
     addBreakPointFuncEntry(func);
   }
 }
 
-void phpBreakPointHook(Eval::DebuggerProxyVM* proxy) {
-  // Set file:line breakpoints to each loaded file
+// Helper which will look at every loaded file and attempt to see if any
+// existing file:line breakpoints should be set.
+void phpSetBreakPointsInAllFiles(Eval::DebuggerProxyVM* proxy) {
   for (EvaledFilesMap::const_iterator it =
        g_vmContext->m_evaledFiles.begin();
        it != g_vmContext->m_evaledFiles.end(); ++it) {
@@ -222,12 +240,12 @@ void phpBreakPointHook(Eval::DebuggerProxyVM* proxy) {
 
 //////////////////////////////////////////////////////////////////////////
 
-struct PtrMapNode {
+struct PCFilter::PtrMapNode {
   void **m_entries;
   void clearImpl(unsigned short bits);
 };
 
-void PtrMapNode::clearImpl(unsigned short bits) {
+void PCFilter::PtrMapNode::clearImpl(unsigned short bits) {
   // clear all the sub levels and mark all slots NULL
   if (bits <= PTRMAP_LEVEL_BITS) {
     assert(bits == PTRMAP_LEVEL_BITS);
@@ -237,28 +255,29 @@ void PtrMapNode::clearImpl(unsigned short bits) {
   }
   for (int i = 0; i < PTRMAP_LEVEL_ENTRIES; i++) {
     if (m_entries[i]) {
-      ((PtrMapNode*)m_entries[i])->clearImpl(bits - PTRMAP_LEVEL_BITS);
-      free(((PtrMapNode*)m_entries[i])->m_entries);
+      ((PCFilter::PtrMapNode*)m_entries[i])->clearImpl(bits -
+                                                       PTRMAP_LEVEL_BITS);
+      free(((PCFilter::PtrMapNode*)m_entries[i])->m_entries);
       free(m_entries[i]);
       m_entries[i] = nullptr;
     }
   }
 }
 
-PtrMapNode* PtrMap::MakeNode() {
+PCFilter::PtrMapNode* PCFilter::PtrMap::MakeNode() {
   PtrMapNode* node = (PtrMapNode*)malloc(sizeof(PtrMapNode));
   node->m_entries =
     (void**)calloc(1, PTRMAP_LEVEL_ENTRIES * sizeof(void*));
   return node;
 }
 
-PtrMap::~PtrMap() {
+PCFilter::PtrMap::~PtrMap() {
   clear();
   free(m_root->m_entries);
   free(m_root);
 }
 
-void* PtrMap::getPointer(void* ptr) {
+void* PCFilter::PtrMap::getPointer(void* ptr) {
   PtrMapNode* current = m_root;
   unsigned short cursor = PTRMAP_PTR_SIZE;
   while (current && cursor) {
@@ -271,7 +290,7 @@ void* PtrMap::getPointer(void* ptr) {
   return (void*)current;
 }
 
-void PtrMap::setPointer(void* ptr, void* val) {
+void PCFilter::PtrMap::setPointer(void* ptr, void* val) {
   PtrMapNode* current = m_root;
   unsigned short cursor = PTRMAP_PTR_SIZE;
   while (true) {
@@ -290,15 +309,14 @@ void PtrMap::setPointer(void* ptr, void* val) {
   }
 }
 
-void PtrMap::clear() {
+void PCFilter::PtrMap::clear() {
   m_root->clearImpl(PTRMAP_PTR_SIZE);
 }
 
 int PCFilter::addRanges(const Unit* unit, const OffsetRangeVec& offsets) {
   int counter = 0;
-  for (OffsetRangeVec::const_iterator it = offsets.begin();
-       it != offsets.end(); ++it) {
-    for (PC pc = unit->at(it->m_base); pc < unit->at(it->m_past);
+  for (auto range = offsets.cbegin(); range != offsets.cend(); ++range) {
+    for (PC pc = unit->at(range->m_base); pc < unit->at(range->m_past);
          pc += instrLen((Opcode*)pc)) {
       addPC(pc);
       counter++;

hphp/runtime/vm/debugger_hook.h

@@ -25,21 +25,31 @@ class DebuggerProxyVM;
 class PhpFile;
 }}
 
+///////////////////////////////////////////////////////////////////////////////
+// This is a set of functions which are primarily called from the VM to notify
+// the debugger about various events. Some of the implemenatitons also interact
+// with the VM to setup further notifications, though this is not the only place
+// the debugger interacts directly with the VM.
+
 namespace HPHP {
 namespace VM {
 
-void phpDebuggerHook(const uchar* pc);
-void phpExceptionHook(ObjectData* e);
-
+// "Hooks" called by the VM at various points during program execution while
+// debugging to give the debugger a chance to act. The debugger may block
+// execution indefinetly within one of these hooks.
+void phpDebuggerOpcodeHook(const uchar* pc);
+void phpDebuggerExceptionHook(ObjectData* e);
 void phpDebuggerEvalHook(const Func* f);
-void phpBreakPointHook(Eval::DebuggerProxyVM* proxy);
-void phpFileLoadHook(Eval::PhpFile* efile);
-
+void phpDebuggerFileLoadHook(Eval::PhpFile* efile);
 class Class;
 class Func;
-void phpDefClassHook(const Class* cls);
-void phpDefFuncHook(const Func* func);
+void phpDebuggerDefClassHook(const Class* cls);
+void phpDebuggerDefFuncHook(const Func* func);
 
+// Helper to apply pending breakpoints to all files.
+void phpSetBreakPointsInAllFiles(Eval::DebuggerProxyVM* proxy);
+
+// Is this thread being debugged?
 static inline bool isDebuggerAttached() {
   return ThreadInfo::s_threadInfo.getNoCheck()->m_reqInjectionData.debugger;
 }
@@ -50,37 +60,44 @@ static inline bool isDebuggerAttached() {
   }                                                                   \
 } while(0)                                                            \
 
+// Is this process being debugged?
+bool isDebuggerAttachedProcess();
+
+// This flag ensures two things: first, that we stay in the interpreter and
+// out of JIT code. Second, that the phpDebuggerHook will continue to allow
+// debugger interrupts for every opcode executed (modulo filters.)
 #define DEBUGGER_FORCE_INTR  \
   (ThreadInfo::s_threadInfo.getNoCheck()->m_reqInjectionData.debuggerIntr)
 
+// Map which holds a set of PCs and supports reasonably fast addition and
+// lookup. Used by the debugger to decide if a given PC falls within an
+// interesting area, e.g., for breakpoints and stepping.
+class PCFilter {
+private:
+  // Radix-tree implementation of pointer map
+  struct PtrMapNode;
+  class PtrMap {
 #define PTRMAP_PTR_SIZE       (sizeof(void*) * 8)
 #define PTRMAP_LEVEL_BITS     8LL
 #define PTRMAP_LEVEL_ENTRIES  (1LL << PTRMAP_LEVEL_BITS)
 #define PTRMAP_LEVEL_MASK     (PTRMAP_LEVEL_ENTRIES - 1LL)
 
-bool isDebuggerAttachedProcess();
+  public:
+    PtrMap() {
+      static_assert(PTRMAP_PTR_SIZE % PTRMAP_LEVEL_BITS == 0,
+                    "PTRMAP_PTR_SIZE must be a multiple of PTRMAP_LEVEL_BITS");
+      m_root = MakeNode();
+    }
+    ~PtrMap();
+    void setPointer(void* ptr, void* val);
+    void* getPointer(void* ptr);
+    void clear();
 
-class PtrMapNode;
-class PtrMap {
-  // Radix-tree implementation of pointer map
-public:
-  PtrMap() {
-    static_assert(PTRMAP_PTR_SIZE % PTRMAP_LEVEL_BITS == 0,
-                  "PTRMAP_PTR_SIZE must be a multiple of PTRMAP_LEVEL_BITS");
-    m_root = MakeNode();
-  }
-  ~PtrMap();
-  void setPointer(void* ptr, void* val);
-  void* getPointer(void* ptr);
-  void clear();
+  private:
+    PtrMapNode* m_root;
+    static PtrMapNode* MakeNode();
+  };
 
-private:
-  PtrMapNode* m_root;
-  static PtrMapNode* MakeNode();
-};
-
-class PCFilter {
-private:
   PtrMap m_map;
 
 public:

hphp/runtime/vm/func_inline.h

@@ -35,7 +35,7 @@ inline ALWAYS_INLINE void setCachedFunc(Func* func, bool debugger) {
     }
   }
   *funcAddr = func;
-  if (UNLIKELY(debugger)) phpDefFuncHook(func);
+  if (UNLIKELY(debugger)) phpDebuggerDefFuncHook(func);
 }
 
 } } // HPHP::VM

hphp/runtime/vm/unit.cpp

@@ -450,7 +450,7 @@ bool Unit::compileTimeFatal(const StringData*& msg, int& line) const {
 
 class FrameRestore {
  public:
-  FrameRestore(const PreClass* preClass) {
+  explicit FrameRestore(const PreClass* preClass) {
     VMExecutionContext* ec = g_vmContext;
     ActRec* fp = ec->getFP();
     PC pc = ec->getPC();
@@ -550,7 +550,7 @@ Class* Unit::defClass(const PreClass* preClass,
       Class::Avail avail = class_->avail(parent, failIsFatal /*tryAutoload*/);
       if (LIKELY(avail == Class::AvailTrue)) {
         class_->setCached();
-        DEBUGGER_ATTACHED_ONLY(phpDefClassHook(class_));
+        DEBUGGER_ATTACHED_ONLY(phpDebuggerDefClassHook(class_));
         return class_;
       }
       if (avail == Class::AvailFail) {
@@ -614,7 +614,7 @@ Class* Unit::defClass(const PreClass* preClass,
     }
     newClass.get()->incAtomicCount();
     newClass.get()->setCached();
-    DEBUGGER_ATTACHED_ONLY(phpDefClassHook(newClass.get()));
+    DEBUGGER_ATTACHED_ONLY(phpDebuggerDefClassHook(newClass.get()));
     return newClass.get();
   }
 }
@@ -1123,7 +1123,7 @@ void Unit::mergeImpl(void* tcbase, UnitMergeInfo* mi) {
         Func* func = *it;
         assert(func->top());
         getDataRef<Func*>(tcbase, func->getCachedOffset()) = func;
-        if (debugger) phpDefFuncHook(func);
+        if (debugger) phpDebuggerDefFuncHook(func);
       } while (++it != fend);
     } else {
       do {
@@ -1169,7 +1169,7 @@ void Unit::mergeImpl(void* tcbase, UnitMergeInfo* mi) {
           }
         }
         getDataRef<Class*>(tcbase, cls->m_cachedOffset) = cls;
-        if (debugger) phpDefClassHook(cls);
+        if (debugger) phpDebuggerDefClassHook(cls);
       } else {
         if (UNLIKELY(!defClass(pre, false))) {
           redoHoistable = true;
@@ -1247,7 +1247,7 @@ void Unit::mergeImpl(void* tcbase, UnitMergeInfo* mi) {
           }
           assert(avail == Class::AvailTrue);
           getDataRef<Class*>(tcbase, cls->m_cachedOffset) = cls;
-          if (debugger) phpDefClassHook(cls);
+          if (debugger) phpDebuggerDefClassHook(cls);
           obj = mi->mergeableObj(++ix);
           k = UnitMergeKind(uintptr_t(obj) & 7);
         } while (k == UnitMergeKindUniqueDefinedClass);