fix the platform session process launch implementation (#143)

## Purpose
Add proper support for the `A` packet (program arguments) in the
platform session implementation, launching programs on request from the
connected debugger. Along with PR #146, addresses issue #137.

## Overview
* Implement `ProcessLaunchMixin::onSetProgramArguments` to spawn the
specified executable with stdio and environment variables memoized in
previous calls to `onSetStdFile` and `onSetEnvironmentVariable`. Use the
existing `ProcessSpawner` implementation for the heavy lifting.
* Implement `ProcessLaunchMixin::onQueryProcessInfo` to enable the
debugger to get details about the most recently launched process. Reuse
the exiting `Platform::GetProcessInfo` function to retrieve the process
information.
* Rename the pid-based `SessionDelegate::onQueryProcessInfo` method to
`SessionDelegate::onQueryProcessInfoPID` to fix a `-Woverloaded-virtual`
compiler warning. This also makes the method name consistent with its
associated command packet `qProcessInfoPID` (as `onQueryProcessInfo` is
with the `qProcessInfo` packet).
* Implement `ProcessLaunchMixin::onTerminate` to kill the specified
process and wait for it to exit. This method is invoked when a `vKill`
packet is received. Only allow processes that were previously launched
via this mechanism to be killed in this way by maintaining a list of all
launched processes.
* Implement the POSIX version of a new `Platform::TerminateProcess`
function, which uses `kill(2)` and `waitpid(2)` to kill the requested
process.
* Stub-out the Windows version of `Platform::TerminateProcess` for
future implementation. There is no platform mode for Windows, so this
code path is not currently reachable.
* Special-case the scenario where the program being launched is ds2 in
gdbserver mode and propagate logging settings to make it easier to debug
test failures.


## Problem Details
Typically, the `qLaunchGDBServer` packet is used to launch another
instance of ds2 running in "gdb server" mode. Many of the lldb tests use
this mechanism. However, some tests use the `A` (program arguments)
packet to launch new ds2 instances instead, and this method doesn't work
correctly when ds2 is running in "platform" mode.

Currently, ds2 uses the `ProcessLaunchMixin` implementation in
`PlatformSession` to ostensibly support launching processes. However, it
only memoizes the program details (environment, stdio files) and never
actually launches the program even though it reports success to the
connected debugger.

It is worth noting that the `A` packet *is* handled correctly by
`DebugSession`, but that implementation is specific to launching
inferiors to be debugged (setting up ptrace, etc). `PlatformSession`
cannot share the same implementation because platform mode launches
programs without the intent of debugging them. They can both use
`ProcessSpawner` for the bulk of the implementation.

## Validation
Once this PR and #146 are merged, most of the `TestGdbRemote*` and
`TestLldbGdbServer*` lldb tests succeed with ds2 running on Android and
Linux and can be enabled in CI. They do not all pass with this PR alone,
but local validation confirms the following:
* Child debug server instances are launching in response to `A` packets
and connect to the debugger/test over independent sockets
* Child processes are being killed and cleaned up in response to `vKill`
packets received during test cleanup

Author: andrurogerz

Headers/DebugServer2/GDBRemote/DummySessionDelegateImpl.h

@@ -252,8 +252,8 @@ class DummySessionDelegateImpl : public SessionDelegate {
 
   ErrorCode onQueryProcessList(Session &session, ProcessInfoMatch const &match,
                                bool first, ProcessInfo &info) const override;
-  ErrorCode onQueryProcessInfo(Session &session, ProcessId pid,
-                               ProcessInfo &info) const override;
+  ErrorCode onQueryProcessInfoPID(Session &session, ProcessId pid,
+                                  ProcessInfo &info) const override;
 
   ErrorCode onLaunchDebugServer(Session &session, std::string const &host,
                                 uint16_t &port, ProcessId &pid) override;

Headers/DebugServer2/GDBRemote/Mixins/ProcessLaunchMixin.h

@@ -34,12 +34,15 @@ template <typename T> class ProcessLaunchMixin : public T {
   EnvironmentMap _environment;
   std::string _stdFile[3];
   StringCollection _arguments;
+  std::vector<ProcessId> _processList;
+  ErrorCode _lastLaunchResult;
 
 public:
   template <typename... Args>
   explicit ProcessLaunchMixin(Args &&... args)
       : T(std::forward<Args>(args)...), _disableASLR(false),
-        _workingDirectory(Platform::GetWorkingDirectory()) {}
+        _workingDirectory(Platform::GetWorkingDirectory()),
+        _lastLaunchResult(kSuccess) {}
 
 public:
   ErrorCode onDisableASLR(Session &session, bool disable) override;
@@ -57,6 +60,14 @@ template <typename T> class ProcessLaunchMixin : public T {
                                   StringCollection const &args) override;
   ErrorCode onQueryLaunchSuccess(Session &session,
                                  ProcessId pid) const override;
+  ErrorCode onQueryProcessInfo(Session &session,
+                               ProcessInfo &info) const override;
+  ErrorCode onTerminate(Session &session, ProcessThreadId const &ptid,
+                        StopInfo &stop) override;
+
+private:
+  ErrorCode spawnProcess();
+  static bool isDebugServer(StringCollection const &args);
 };
 } // namespace GDBRemote
 } // namespace ds2

Headers/DebugServer2/GDBRemote/PlatformSessionImpl.h

@@ -29,8 +29,8 @@ class PlatformSessionImplBase : public DummySessionDelegateImpl {
 protected:
   ErrorCode onQueryProcessList(Session &session, ProcessInfoMatch const &match,
                                bool first, ProcessInfo &info) const override;
-  ErrorCode onQueryProcessInfo(Session &session, ProcessId pid,
-                               ProcessInfo &info) const override;
+  ErrorCode onQueryProcessInfoPID(Session &session, ProcessId pid,
+                                  ProcessInfo &info) const override;
 
   ErrorCode onExecuteProgram(Session &session, std::string const &command,
                              uint32_t timeout,

Headers/DebugServer2/GDBRemote/SessionDelegate.h

@@ -280,8 +280,8 @@ class SessionDelegate {
   virtual ErrorCode onQueryProcessList(Session &session,
                                        ProcessInfoMatch const &match,
                                        bool first, ProcessInfo &info) const = 0;
-  virtual ErrorCode onQueryProcessInfo(Session &session, ProcessId pid,
-                                       ProcessInfo &info) const = 0;
+  virtual ErrorCode onQueryProcessInfoPID(Session &session, ProcessId pid,
+                                          ProcessInfo &info) const = 0;
 
   virtual ErrorCode onLaunchDebugServer(Session &session,
                                         std::string const &host, uint16_t &port,

Headers/DebugServer2/Host/Platform.h

@@ -74,6 +74,7 @@ class Platform {
   static void
   EnumerateProcesses(bool allUsers, UserId const &uid,
                      std::function<void(ProcessInfo const &info)> const &cb);
+  static bool TerminateProcess(ProcessId pid);
 
 public:
   static std::string GetThreadName(ProcessId pid, ThreadId tid);

Sources/GDBRemote/DummySessionDelegateImpl.cpp

@@ -300,7 +300,7 @@ DUMMY_IMPL_EMPTY_CONST(onFileFstat, Session &, int, ByteVector &)
 DUMMY_IMPL_EMPTY_CONST(onQueryProcessList, Session &, ProcessInfoMatch const &,
                        bool, ProcessInfo &)
 
-DUMMY_IMPL_EMPTY_CONST(onQueryProcessInfo, Session &, ProcessId, ProcessInfo &)
+DUMMY_IMPL_EMPTY_CONST(onQueryProcessInfoPID, Session &, ProcessId, ProcessInfo &)
 
 DUMMY_IMPL_EMPTY(onLaunchDebugServer, Session &, std::string const &,
                  uint16_t &, ProcessId &)

Sources/GDBRemote/Mixins/ProcessLaunchMixin.hpp

@@ -12,6 +12,8 @@
 
 #include "DebugServer2/GDBRemote/Mixins/ProcessLaunchMixin.h"
 
+#include <algorithm>
+
 namespace ds2 {
 namespace GDBRemote {
 
@@ -46,8 +48,8 @@ ProcessLaunchMixin<T>::onQueryWorkingDirectory(Session &,
 }
 
 template <typename T>
-ErrorCode ProcessLaunchMixin<T>::onSetEnvironmentVariable(
-    Session &, std::string const &name, std::string const &value) {
+ErrorCode ProcessLaunchMixin<T>::onSetEnvironmentVariable(Session &,
+    std::string const &name, std::string const &value) {
   if (value.empty()) {
     _environment.erase(name);
   } else {
@@ -88,15 +90,129 @@ ErrorCode
 ProcessLaunchMixin<T>::onSetProgramArguments(Session &,
                                              StringCollection const &args) {
   _arguments = args;
-  for (auto const &arg : _arguments) {
-    DS2LOG(Debug, "arg=%s", arg.c_str());
+  if (isDebugServer(args)) {
+    // Propagate current logging settings when launching an instance of
+    // the debug server to match qLaunchGDBServer behavior.
+    if (GetLogLevel() == kLogLevelDebug)
+      _arguments.push_back("--debug");
+    else if (GetLogLevel() == kLogLevelPacket)
+      _arguments.push_back("--remote-debug");
   }
-  return kSuccess;
+
+  return spawnProcess();
+
 }
 
 template <typename T>
 ErrorCode ProcessLaunchMixin<T>::onQueryLaunchSuccess(Session &,
                                                       ProcessId) const {
+  return _lastLaunchResult;
+}
+
+template <typename T>
+ErrorCode
+ProcessLaunchMixin<T>::onQueryProcessInfo(Session &,
+                                          ProcessInfo &info) const {
+  if (_processList.empty())
+    return kErrorProcessNotFound;
+
+  const ProcessId pid = _processList.back();
+  if (!Platform::GetProcessInfo(pid, info))
+    return kErrorUnknown;
+
+  return kSuccess;
+}
+
+template <typename T>
+ErrorCode ProcessLaunchMixin<T>::onTerminate(Session &session,
+                                             ProcessThreadId const &ptid,
+                                             StopInfo &stop) {
+  auto it = std::find(_processList.begin(), _processList.end(), ptid.pid);
+  if (it == _processList.end())
+    return kErrorNotFound;
+
+  if (!Platform::TerminateProcess(ptid.pid))
+    return kErrorUnknown;
+
+  DS2LOG(Debug, "killed spawned process %" PRI_PID, *it);
+  _processList.erase(it);
+
+  // StopInfo is not populated by this implemenation of onTerminate since it is
+  // only ever called in the context of a platform session in response to a
+  // qKillSpawnedProcess packet.
+  stop.clear();
+  return kSuccess;
+}
+
+// Some test cases use this code path to launch additional instances of the
+// debug server rather than sending a qLaunchGDBServer packet. Allow detection
+// of this scenario so we can propagate logging settings and make debugging
+// test failures easier.
+template <typename T>
+bool ProcessLaunchMixin<T>::isDebugServer(StringCollection const &args) {
+  return (args.size() > 1)
+      && (args[0] == Platform::GetSelfExecutablePath())
+      && (args[1] == "gdbserver");
+}
+
+template <typename T>
+ErrorCode ProcessLaunchMixin<T>::spawnProcess() {
+  const bool displayArgs = _arguments.size() > 1;
+  const bool displayEnv = !_environment.empty();
+  auto it = _arguments.begin();
+  DS2LOG(Debug, "spawning process '%s'%s", (it++)->c_str(),
+         displayArgs ? " with args:" : "");
+  while (it != _arguments.end()) {
+    DS2LOG(Debug, "  %s", (it++)->c_str());
+  }
+
+  if (displayEnv) {
+    DS2LOG(Debug, "%swith environment:", displayArgs ? "and " : "");
+    for (auto const &val : _environment) {
+      DS2LOG(Debug, "  %s=%s", val.first.c_str(), val.second.c_str());
+    }
+  }
+
+  if (!_workingDirectory.empty()) {
+    DS2LOG(Debug, "%swith working directory: %s",
+           displayArgs || displayEnv ? "and " : "",
+           _workingDirectory.c_str());
+  }
+
+  Host::ProcessSpawner ps;
+  if (!ps.setExecutable(_arguments[0]) ||
+      !ps.setArguments(StringCollection(_arguments.begin() + 1,
+                                        _arguments.end())) ||
+      !ps.setWorkingDirectory(_workingDirectory) ||
+      !ps.setEnvironment(_environment))
+    return kErrorInvalidArgument;
+
+  if (isDebugServer(_arguments)) {
+    // Always log to the console when launching an instance of the debug server
+    // to match qLaunchGDBServer behavior.
+    ps.redirectInputToNull();
+    ps.redirectOutputToConsole();
+    ps.redirectErrorToConsole();
+  } else {
+    if (!_stdFile[0].empty() && !ps.redirectInputToFile(_stdFile[0]))
+      return kErrorInvalidArgument;
+    if (!_stdFile[1].empty() && !ps.redirectOutputToFile(_stdFile[1]))
+      return kErrorInvalidArgument;
+    if (!_stdFile[2].empty() && !ps.redirectErrorToFile(_stdFile[2]))
+      return kErrorInvalidArgument;
+  }
+
+  _lastLaunchResult = ps.run();
+  if (_lastLaunchResult != kSuccess)
+    return _lastLaunchResult;
+
+  // Add the pid to the list of launched processes. It will be removed when
+  // onTerminate is called.
+  _processList.push_back(ps.pid());
+
+  DS2LOG(Debug, "launched %s as process %" PRI_PID, _arguments[0].c_str(),
+         ps.pid());
+
   return kSuccess;
 }
 } // namespace GDBRemote

Sources/GDBRemote/PlatformSessionImpl.cpp

@@ -38,8 +38,9 @@ ErrorCode PlatformSessionImplBase::onQueryProcessList(
   return kSuccess;
 }
 
-ErrorCode PlatformSessionImplBase::onQueryProcessInfo(Session &, ProcessId pid,
-                                                      ProcessInfo &info) const {
+ErrorCode
+PlatformSessionImplBase::onQueryProcessInfoPID(Session &, ProcessId pid,
+                                               ProcessInfo &info) const {
   if (!Platform::GetProcessInfo(pid, info))
     return kErrorProcessNotFound;
   else

Sources/GDBRemote/Session.cpp

@@ -2019,7 +2019,7 @@ void Session::Handle_qProcessInfoPID(ProtocolInterpreter::Handler const &,
   ProcessId pid = std::strtoul(args.c_str(), nullptr, 10);
 
   ProcessInfo info;
-  CHK_SEND(_delegate->onQueryProcessInfo(*this, pid, info));
+  CHK_SEND(_delegate->onQueryProcessInfoPID(*this, pid, info));
 
   send(info.encode(_compatMode, true));
 }

Sources/Host/POSIX/Platform.cpp

@@ -19,9 +19,12 @@
 #include <grp.h>
 #include <netdb.h>
 #include <pwd.h>
+#include <signal.h>
 #include <string>
 #include <unistd.h>
 
+#include <sys/wait.h>
+
 #if defined(OS_FREEBSD) || defined(OS_DARWIN)
 #include <sys/socket.h>
 #include <sys/types.h>
@@ -124,6 +127,23 @@ bool Platform::SetWorkingDirectory(std::string const &directory) {
 
 ds2::ProcessId Platform::GetCurrentProcessId() { return ::getpid(); }
 
+// Terminates the process and waits for it to exit before returning.
+bool Platform::TerminateProcess(ProcessId pid) {
+  if (::kill(pid, SIGKILL) < 0 && errno != ESRCH)
+    return false;
+
+  int status;
+  pid_t ret;
+  do {
+    ret = ::waitpid(pid, &status, 0);
+  } while (ret == -1 && errno == EINTR);
+
+  if (ret != pid)
+    return false;
+
+  return WIFEXITED(status) || WIFSIGNALED(status);
+}
+
 bool Platform::GetCurrentEnvironment(EnvironmentBlock &env) {
   for (int i = 0; environ[i] != nullptr; ++i) {
     char *equal = strchr(environ[i], '=');