Improve readability of break point code in debugger.

Added comments to every method in cmd_breakpoint.cpp. Also renamed some methods to make their intent more obvious. Moved a few implementation methods from the public to protected space.

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

@@ -18,18 +18,27 @@
 
 namespace HPHP { namespace Eval {
 ///////////////////////////////////////////////////////////////////////////////
+TRACE_SET_MOD(debugger);
 
+// Serializes this command into the given Thrift buffer.
 void CmdBreak::sendImpl(DebuggerThriftBuffer &thrift) {
-  DebuggerCommand::sendImpl(thrift);
+  // m_breakpoints is initially set to the breakpoints collection of the
+  // client (in validate, which indirectly calls sendImpl). When received
+  // via Thrift, m_breakpoints points to a copy that is placed in m_bps.
   assert(m_breakpoints);
+  DebuggerCommand::sendImpl(thrift);
   BreakPointInfo::SendImpl(*m_breakpoints, thrift);
 }
 
+// Deserializes a CmdBreak from the given Thrift buffer.
 void CmdBreak::recvImpl(DebuggerThriftBuffer &thrift) {
   DebuggerCommand::recvImpl(thrift);
   BreakPointInfo::RecvImpl(m_bps, thrift);
 }
 
+// Informs the client of all strings that may follow a break command.
+// Used for auto completion. The client uses the prefix of the argument
+// following the command to narrow down the list displayed to the user.
 void CmdBreak::list(DebuggerClient *client) {
   if (client->argCount() == 0 ||
       (client->argCount() == 1 &&
@@ -53,7 +62,7 @@ void CmdBreak::list(DebuggerClient *client) {
     client->addCompletion(DebuggerClient::AutoCompleteClasses);
     client->addCompletion(DebuggerClient::AutoCompleteClassMethods);
   } else if (client->argCount() == 1) {
-    if (hasUpdateArg(client)) {
+    if (hasStatusChangeArg(client)) {
       client->addCompletion("all");
     } else if (!client->arg(1, "list")) {
       client->addCompletion(DebuggerClient::AutoCompleteFileNames);
@@ -63,6 +72,7 @@ void CmdBreak::list(DebuggerClient *client) {
   }
 }
 
+// The text to display when the debugger client processes "help break".
 bool CmdBreak::help(DebuggerClient *client) {
   client->helpTitle("Break Command");
   client->helpCmds(
@@ -146,6 +156,7 @@ bool CmdBreak::help(DebuggerClient *client) {
   return true;
 }
 
+// Carries out the "break list" command.
 bool CmdBreak::processList(DebuggerClient *client) {
   m_breakpoints = client->getBreakPoints();
   for (int i = 0; i < (int)m_breakpoints->size(); i++) {
@@ -166,7 +177,8 @@ bool CmdBreak::processList(DebuggerClient *client) {
   return true;
 }
 
-bool CmdBreak::processUpdate(DebuggerClient *client) {
+// Carries out commands that change the status of a breakpoint.
+bool CmdBreak::processStatusChange(DebuggerClient *client) {
   m_breakpoints = client->getBreakPoints();
   if (m_breakpoints->empty()) {
     client->error("There is no breakpoint to clear or toggle.");
@@ -212,7 +224,7 @@ bool CmdBreak::processUpdate(DebuggerClient *client) {
       }
     }
     if (found) {
-      updateImpl(client);
+      updateServer(client);
       return true;
     }
 
@@ -223,7 +235,7 @@ bool CmdBreak::processUpdate(DebuggerClient *client) {
   if (client->arg(2, "all")) {
     if (hasClearArg(client)) {
       m_breakpoints->clear();
-      updateImpl(client);
+      updateServer(client);
       client->info("All breakpoints are cleared.");
       return true;
     }
@@ -242,7 +254,7 @@ bool CmdBreak::processUpdate(DebuggerClient *client) {
       }
     }
 
-    updateImpl(client);
+    updateServer(client);
     return processList(client);
   }
 
@@ -274,23 +286,23 @@ bool CmdBreak::processUpdate(DebuggerClient *client) {
   BreakPointInfoPtr bpi = (*m_breakpoints)[index];
   if (hasClearArg(client)) {
     m_breakpoints->erase(m_breakpoints->begin() + index);
-    updateImpl(client);
+    updateServer(client);
     client->info("Breakpoint %d cleared %s", bpi->index(),
                  bpi->desc().c_str());
   } else if (hasEnableArg(client)) {
     bpi->setState(BreakPointInfo::Always);
-    updateImpl(client);
+    updateServer(client);
     client->info("Breakpoint %d's state is changed to %s.", bpi->index(),
                  bpi->state(false).c_str());
   } else if (hasDisableArg(client)) {
     bpi->setState(BreakPointInfo::Disabled);
-    updateImpl(client);
+    updateServer(client);
     client->info("Breakpoint %d's state is changed to %s.", bpi->index(),
                  bpi->state(false).c_str());
   } else {
     assert(hasToggleArg(client));
     bpi->toggle();
-    updateImpl(client);
+    updateServer(client);
     client->info("Breakpoint %d's state is changed to %s.", bpi->index(),
                  bpi->state(false).c_str());
   }
@@ -298,19 +310,30 @@ bool CmdBreak::processUpdate(DebuggerClient *client) {
   return true;
 }
 
-bool CmdBreak::updateImpl(DebuggerClient *client) {
+// Uses the client to send this command to the server, which
+// will update its breakpoint list with the one in this command.
+// The client will block until the server echoes
+// this command back to it. The echoed command is discarded.
+bool CmdBreak::updateServer(DebuggerClient *client) {
   m_body = "update";
   client->xend<CmdBreak>(this);
   return true;
 }
 
-bool CmdBreak::update(DebuggerClient *client) {
-  m_breakpoints = client->getBreakPoints();
-  return updateImpl(client);
+// Creates a new CmdBreak instance, sets its breakpoints to the client's
+// list, sends the command to the server and waits for a response.
+bool CmdBreak::SendClientBreakpointListToServer(DebuggerClient *client) {
+  auto cmd = CmdBreak();
+  cmd.m_breakpoints = client->getBreakPoints();
+  return cmd.updateServer(client);
 }
 
-bool CmdBreak::validate(DebuggerClient *client, BreakPointInfoPtr bpi,
-                        int index) {
+// Adds conditional or watch clause to the breakpoint info if needed.
+// Then adds the breakpoint to client's list and sends this command
+// to the server so that it too can update it's list.
+// Returns false if the breakpoint is not well formed.
+bool CmdBreak::addToBreakpointListAndUpdateServer(
+    DebuggerClient *client, BreakPointInfoPtr bpi, int index) {
   ++index;
   if (client->arg(index, "if")) {
     bpi->setClause(client->lineRest(++index), true);
@@ -327,11 +350,15 @@ bool CmdBreak::validate(DebuggerClient *client, BreakPointInfoPtr bpi,
       }
     }
     m_breakpoints->push_back(bpi);
-    updateImpl(client);
+    updateServer(client);
     client->info("Breakpoint %d set %s", bpi->index(), bpi->desc().c_str());
     return true;
   }
 
+  // TODO: keep more detailed error information in the BreakPointInfo
+  // and emit that information here. Well, first factor out this code
+  // into a separate check and report method and report from there.
+  // See task 2368334.
   if (!bpi->m_url.empty()) {
     client->error("@{url} cannot be specified alone.");
   } else {
@@ -340,6 +367,9 @@ bool CmdBreak::validate(DebuggerClient *client, BreakPointInfoPtr bpi,
   return false;
 }
 
+// Carries out the Break command. This always involves an action on the
+// client and usually, but not always, involves the server by sending
+// this command to the server and waiting for its response.
 bool CmdBreak::onClient(DebuggerClient *client) {
   if (DebuggerCommand::onClient(client)) return true;
 
@@ -355,8 +385,8 @@ bool CmdBreak::onClient(DebuggerClient *client) {
     index++;
   } else if (client->arg(1, "list")) {
     return processList(client);
-  } else if (hasUpdateArg(client)) {
-    return processUpdate(client);
+  } else if (hasStatusChangeArg(client)) {
+    return processStatusChange(client);
   }
 
   string currentFile;
@@ -400,7 +430,7 @@ bool CmdBreak::onClient(DebuggerClient *client) {
     bpi = BreakPointInfoPtr(new BreakPointInfo(regex, state, interrupt, url));
   }
 
-  if (!validate(client, bpi, index)) {
+  if (!addToBreakpointListAndUpdateServer(client, bpi, index)) {
     client->tutorial(
       "This is the order of different arguments:\n"
       "\n"
@@ -434,6 +464,9 @@ static const StaticString s_if("if");
 static const StaticString s_ampamp("ampamp");
 static const StaticString s_desc("desc");
 
+// Updates the client with information about the execution of this command.
+// This information is not used by the command line client, but can be accessed
+// via the debugger client API exposed to PHP programs.
 void CmdBreak::setClientOutput(DebuggerClient *client) {
   // Output an array of current breakpoints including exceptions
   client->setOutputType(DebuggerClient::OTValues);
@@ -466,6 +499,10 @@ void CmdBreak::setClientOutput(DebuggerClient *client) {
   client->setOTValues(values);
 }
 
+// Updates the breakpoint list in the proxy with the new list
+// received from the client. Then sends the command back to the
+// client as confirmation. Returns false if the confirmation message
+// send failed.
 bool CmdBreak::onServer(DebuggerProxy *proxy) {
   if (m_body == "update") {
     proxy->setBreakPoints(m_bps);
@@ -475,24 +512,35 @@ bool CmdBreak::onServer(DebuggerProxy *proxy) {
   return false;
 }
 
-bool CmdBreak::hasUpdateArg(DebuggerClient *client) {
+// Returns true if the last command parsed by the client has
+// an argument that changes the status of a breakpoint.
+// I.e. clear, enable, disable or toggle.
+bool CmdBreak::hasStatusChangeArg(DebuggerClient *client) {
   return
     hasClearArg(client) || hasEnableArg(client) ||
     hasDisableArg(client) || hasToggleArg(client);
 }
 
+// Returns true if the last command parsed by the client has
+// the string "enable" in its first argument position.
 bool CmdBreak::hasEnableArg(DebuggerClient *client) {
   return client->arg(1, "enable");
 }
 
+// Returns true if the last command parsed by the client has
+// the string "disable" in its first argument position.
 bool CmdBreak::hasDisableArg(DebuggerClient *client) {
   return client->arg(1, "disable");
 }
 
+// Returns true if the last command parsed by the client has
+// the string "clear" in its first argument position.
 bool CmdBreak::hasClearArg(DebuggerClient *client) {
   return client->arg(1, "clear");
 }
 
+// Returns true if the last command parsed by the client has
+// the string "toggle" in its first argument position.
 bool CmdBreak::hasToggleArg(DebuggerClient *client) {
   return client->arg(1, "toggle");
 }

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

@@ -34,26 +34,30 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
-  /**
-   * Sync breakpoints from client to server.
-   */
-  bool update(DebuggerClient *client);
+  static bool SendClientBreakpointListToServer(DebuggerClient *client);
 
 protected:
-  bool validate(DebuggerClient *client, BreakPointInfoPtr bpi, int index);
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+  bool addToBreakpointListAndUpdateServer(
+      DebuggerClient *client, BreakPointInfoPtr bpi, int index);
 
 private:
+  // Either points to the breakpoint collection of a debugger client
+  // or points to m_bps. In the former case the client frees the
+  // memory. In the latter case the destructor for CmdBreak frees
+  // the memory. (The base class destructor is only invoked for instances
+  // that point to the collection in the client.)
   BreakPointInfoPtrVec *m_breakpoints;
+
+  // Holds the breakpoint collection of a CmdBreak received via Thrift.
   BreakPointInfoPtrVec m_bps;
 
-  bool updateImpl(DebuggerClient *client);
+  bool updateServer(DebuggerClient *client);
   bool processList(DebuggerClient *client);
-  bool processUpdate(DebuggerClient *client);
+  bool processStatusChange(DebuggerClient *client);
 
-  bool hasUpdateArg(DebuggerClient *client);
+  bool hasStatusChangeArg(DebuggerClient *client);
   bool hasEnableArg(DebuggerClient *client);
   bool hasDisableArg(DebuggerClient *client);
   bool hasClearArg(DebuggerClient *client);

hphp/runtime/eval/debugger/cmd/cmd_complete.h

@@ -33,6 +33,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_constant.h

@@ -33,6 +33,7 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_eval.h

@@ -31,11 +31,12 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+  virtual void handleReply(DebuggerClient *client);
+
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 
-  virtual void handleReply(DebuggerClient *client);
-
 private:
   String m_output;
   int m_frame;

hphp/runtime/eval/debugger/cmd/cmd_example.h

@@ -31,6 +31,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

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

@@ -85,7 +85,7 @@ bool CmdException::onClient(DebuggerClient *client) {
 
   BreakPointInfoPtr bpi(new BreakPointInfo(regex, state, ExceptionThrown,
                                            client->argValue(index), ""));
-  if (!validate(client, bpi, index)) {
+  if (!addToBreakpointListAndUpdateServer(client, bpi, index)) {
     client->tutorial(
       "This is the order of different arguments:\n"
       "\n"

hphp/runtime/eval/debugger/cmd/cmd_extension.h

@@ -31,6 +31,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_flow_control.h

@@ -47,9 +47,6 @@ public:
       m_count(1) { }
   virtual ~CmdFlowControl();
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
@@ -67,6 +64,9 @@ public:
   bool needsVMInterrupt() { return m_needsVMInterrupt; }
 
 protected:
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
   int decCount() { assert(m_count > 0); return --m_count;}
   int getCount() const { assert(m_count > 0); return m_count;}
   void installLocationFilterForLine(InterruptSite *site);

hphp/runtime/eval/debugger/cmd/cmd_global.h

@@ -33,6 +33,7 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

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

@@ -58,7 +58,7 @@ void CmdHelp::HelpAll(DebuggerClient *client) {
     "Documentation and Source Code", "",
     "[i]nfo",       "displays documentations and other information",
     "[l]ist     *", "displays source codes",
-    "[h]elp",       "displays this help",
+    "[h]elp    **", "displays this help",
     "?",            "displays this help",
 
     "Shell and Extended Commands", "",
@@ -70,7 +70,8 @@ void CmdHelp::HelpAll(DebuggerClient *client) {
     nullptr
   );
 
-  client->helpBody("* These commands are replayable by just hitting return.");
+  client->helpBody("* These commands are replayable by just hitting return.\n"
+      "** Type \"help help\" to get more help.");
 }
 
 void CmdHelp::HelpStarted(DebuggerClient *client) {
@@ -119,11 +120,11 @@ void CmdHelp::HelpStarted(DebuggerClient *client) {
     "\n"
     "The command to run a script normally looks like this,\n"
     "\n"
-    "  ./hhvm -f myscript.php\n"
+    "  hhvm myscript.php\n"
     "\n"
-    "Simply add \"-m debug\" to run the script in debugger,\n"
+    "Simply add \"-m debug\" to run the script in debugger,\n\n"
     "\n"
-    "  ./hhvm -m debug -f myscript.php\n"
+    "  hhvm -m debug myscript.php\n"
     "\n"
     "Once started, set breakpoints like this,\n"
     "\n"
@@ -171,7 +172,7 @@ void CmdHelp::HelpStarted(DebuggerClient *client) {
     "\n"
     "Connect to an HHVM server from command line,\n"
     "\n"
-    "  ./hhvm -m debug --debug-host mymachine.com\n"
+    "  hhvm -m debug -h mymachine.com\n"
     "\n"
     "Or, connect from within debugger,\n"
     "\n"

hphp/runtime/eval/debugger/cmd/cmd_info.h

@@ -38,14 +38,15 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
   bool parseZeroArg(DebuggerClient *client);
   void parseOneArg(DebuggerClient *client, std::string &subsymbol);
   Array getInfo() { return m_info; }
   static String FindSubSymbol(CArrRef symbols, const std::string &symbol);
 
+protected:
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
 private:
   enum SymbolType {
     KindOfUnknown,

hphp/runtime/eval/debugger/cmd/cmd_instrument.h

@@ -34,6 +34,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

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

@@ -216,7 +216,7 @@ bool CmdInterrupt::onClient(DebuggerClient *client) {
         }
       }
       if (toggled) {
-        CmdBreak().update(client);
+        CmdBreak::SendClientBreakpointListToServer(client);
       }
       if (!found) {
         if (m_interrupt == HardBreakPoint) {

hphp/runtime/eval/debugger/cmd/cmd_interrupt.h

@@ -48,14 +48,15 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
   bool shouldBreak(const BreakPointInfoPtrVec &bps);
   std::string getFileLine() const;
 
   InterruptSite *getSite() { return m_site;}
 
+protected:
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
 private:
   int16_t m_interrupt;
   std::string m_program;   // informational only

hphp/runtime/eval/debugger/cmd/cmd_list.h

@@ -36,6 +36,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_machine.h

@@ -43,6 +43,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_print.h

@@ -39,14 +39,15 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
   Variant processWatch(DebuggerClient *client, const char *format,
                     const std::string &php);
 
   virtual void handleReply(DebuggerClient *client);
 
+protected:
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
 private:
   Variant m_ret;
   String m_output;

hphp/runtime/eval/debugger/cmd/cmd_run.h

@@ -33,6 +33,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_shell.h

@@ -33,6 +33,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_signal.h

@@ -31,7 +31,7 @@ public:
   };
 
 public:
-  CmdSignal(Signal sig = SignalNone)
+  explicit CmdSignal(Signal sig = SignalNone)
       : DebuggerCommand(KindOfSignal), m_signum(sig) {}
 
   Signal getSignal() const { return (Signal)m_signum;}
@@ -39,6 +39,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_thread.h

@@ -34,6 +34,7 @@ public:
   virtual bool onClient(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_user.h

@@ -33,7 +33,7 @@ public:
   CmdUser() {
     m_type = KindOfUser;
   }
-  CmdUser(Object cmd) : m_cmd(cmd) {
+  explicit CmdUser(Object cmd) : m_cmd(cmd) {
     m_type = KindOfUser;
   }
 
@@ -43,14 +43,15 @@ public:
   virtual bool help(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
   virtual const ExtendedCommandMap &getCommandMap();
   virtual void invokeList(DebuggerClient *client, const std::string &cls);
   virtual bool invokeHelp(DebuggerClient *client, const std::string &cls);
   virtual bool invokeClient(DebuggerClient *client, const std::string &cls);
 
+protected:
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
 private:
   static Mutex s_mutex;
   static ExtendedCommandMap s_commands;

hphp/runtime/eval/debugger/cmd/cmd_variable.h

@@ -38,6 +38,7 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
+protected:
   virtual void sendImpl(DebuggerThriftBuffer &thrift);
   virtual void recvImpl(DebuggerThriftBuffer &thrift);
 

hphp/runtime/eval/debugger/cmd/cmd_where.h

@@ -33,12 +33,13 @@ public:
   virtual void setClientOutput(DebuggerClient *client);
   virtual bool onServer(DebuggerProxy *proxy);
 
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
-
   Array fetchStackTrace(DebuggerClient *client); // client side
   void processStackTrace(); // server side
 
+protected:
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
 private:
   Array m_stacktrace;
   bool m_stackArgs;

hphp/runtime/eval/debugger/debugger_client.cpp

@@ -637,7 +637,7 @@ void DebuggerClient::init(const DebuggerClientOptions &options) {
   if (!NoPrompt) {
     info("Welcome to HipHop Debugger!");
     info("Type \"help\" or \"?\" for a complete list of commands.\n");
-  }
+   }
 
   if (!options.host.empty()) {
     connectRemote(options.host, options.port);
@@ -691,7 +691,7 @@ void DebuggerClient::run() {
     }
     break;
   }
-  // We are about to exit from client and idealy we should cleanup,
+  // We are about to exit from client and ideally we should cleanup,
   // but we the reset() where will try to cleanup Socket under DMachineInfo,
   // which is created by another thread. If it's cleaned up here, later we'll
   // have a SEGV when trying to sweep the object from the other thread.
@@ -950,7 +950,7 @@ bool DebuggerClient::initializeMachine() {
     // upload breakpoints
     if (!m_breakpoints.empty()) {
       info("Updating breakpoints...");
-      CmdBreak().update(this);
+      CmdBreak::SendClientBreakpointListToServer(this);
     }
 
     // attaching to default sandbox

hphp/runtime/eval/debugger/debugger_command.h

@@ -107,8 +107,6 @@ public:
   // Server-side work for a command. Returning false indicates a failure to
   // communicate with the client (for commands that do so).
   virtual bool onServer(DebuggerProxy *proxy);
-  virtual void sendImpl(DebuggerThriftBuffer &thrift);
-  virtual void recvImpl(DebuggerThriftBuffer &thrift);
 
   virtual void handleReply(DebuggerClient *client) { assert(false); }
 
@@ -120,6 +118,14 @@ public:
   String getWireError() const { return m_wireError; }
 
 protected:
+  // Always called from send and must implement the subclass specific
+  // logic for serializing a command to send via Thrift.
+  virtual void sendImpl(DebuggerThriftBuffer &thrift);
+
+  // Always called from recv and must implement the subclass specific
+  // logic for deserializing a command received via Thrift.
+  virtual void recvImpl(DebuggerThriftBuffer &thrift);
+
   Type m_type;
   std::string m_class; // for CmdExtended
   std::string m_body;

hphp/runtime/vm/debugger_hook.cpp

@@ -85,7 +85,7 @@ bool isDebuggerAttachedProcess() {
 }
 
 // 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
+// each piece of translated code to ensure we punt to the interpreter when the
 // debugger is attached.
 static void blacklistRangesInJit(const Unit* unit,
                                  const OffsetRangeVec& offsets) {

hphp/runtime/vm/debugger_hook.h

@@ -27,7 +27,7 @@ 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
+// the debugger about various events. Some of the implementations also interact
 // with the VM to setup further notifications, though this is not the only place
 // the debugger interacts directly with the VM.
 
@@ -36,7 +36,7 @@ namespace VM {
 
 // "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.
+// execution indefinitely within one of these hooks.
 void phpDebuggerOpcodeHook(const uchar* pc);
 void phpDebuggerExceptionHook(ObjectData* e);
 void phpDebuggerEvalHook(const Func* f);