[lldb] Clean up StartDebugserverProcess before I start refactoring it

[labath]

• use early exits where possible
• avoid the listen thread by using Socket APIs which allow separate "listen" and "accept" steps
• use formatv-like log statements

There "should" be no functional changes from this patch.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

• use early exits where possible
• avoid the listen thread by using Socket APIs which allow separate "listen" and "accept" steps
• use formatv-like log statements

There "should" be no functional changes from this patch.

---

Patch is 25.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/135342.diff

2 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+200-285)
• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h (+2-26)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 77eadfc8c9f6c..e438126a0d67c 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -10,18 +10,15 @@
 
 #include <climits>
 #include <cstring>
-#include <future>
+#include <memory>
 #include <sys/stat.h>
 
-#include "lldb/Host/Config.h"
-#include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/FileSystem.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/HostInfo.h"
 #include "lldb/Host/Pipe.h"
 #include "lldb/Host/ProcessLaunchInfo.h"
 #include "lldb/Host/Socket.h"
-#include "lldb/Host/ThreadLauncher.h"
 #include "lldb/Host/common/TCPSocket.h"
 #include "lldb/Host/posix/ConnectionFileDescriptorPosix.h"
 #include "lldb/Target/Platform.h"
@@ -30,10 +27,10 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/RegularExpression.h"
 #include "lldb/Utility/StreamString.h"
-#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Config/llvm-config.h" // for LLVM_ENABLE_ZLIB
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ScopedPrinter.h"
 
 #include "ProcessGDBRemoteLog.h"
@@ -68,7 +65,7 @@ GDBRemoteCommunication::GDBRemoteCommunication()
 #endif
       m_echo_number(0), m_supports_qEcho(eLazyBoolCalculate), m_history(512),
       m_send_acks(true), m_is_platform(false),
-      m_compression_type(CompressionType::None), m_listen_url() {
+      m_compression_type(CompressionType::None) {
 }
 
 // Destructor
@@ -840,53 +837,6 @@ GDBRemoteCommunication::CheckForPacket(const uint8_t *src, size_t src_len,
   return GDBRemoteCommunication::PacketType::Invalid;
 }
 
-Status GDBRemoteCommunication::StartListenThread(const char *hostname,
-                                                 uint16_t port) {
-  if (m_listen_thread.IsJoinable())
-    return Status::FromErrorString("listen thread already running");
-
-  char listen_url[512];
-  if (hostname && hostname[0])
-    snprintf(listen_url, sizeof(listen_url), "listen://%s:%i", hostname, port);
-  else
-    snprintf(listen_url, sizeof(listen_url), "listen://%i", port);
-  m_listen_url = listen_url;
-  SetConnection(std::make_unique<ConnectionFileDescriptor>());
-  llvm::Expected<HostThread> listen_thread = ThreadLauncher::LaunchThread(
-      listen_url, [this] { return GDBRemoteCommunication::ListenThread(); });
-  if (!listen_thread)
-    return Status::FromError(listen_thread.takeError());
-  m_listen_thread = *listen_thread;
-
-  return Status();
-}
-
-bool GDBRemoteCommunication::JoinListenThread() {
-  if (m_listen_thread.IsJoinable())
-    m_listen_thread.Join(nullptr);
-  return true;
-}
-
-lldb::thread_result_t GDBRemoteCommunication::ListenThread() {
-  Status error;
-  ConnectionFileDescriptor *connection =
-      (ConnectionFileDescriptor *)GetConnection();
-
-  if (connection) {
-    // Do the listen on another thread so we can continue on...
-    if (connection->Connect(
-            m_listen_url.c_str(),
-            [this](llvm::StringRef port_str) {
-              uint16_t port = 0;
-              llvm::to_integer(port_str, port, 10);
-              m_port_promise.set_value(port);
-            },
-            &error) != eConnectionStatusSuccess)
-      SetConnection(nullptr);
-  }
-  return {};
-}
-
 FileSpec GDBRemoteCommunication::GetDebugserverPath(Platform *platform) {
   Log *log = GetLog(GDBRLog::Process);
   // If we locate debugserver, keep that located version around
@@ -950,277 +900,242 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
     const char *url, Platform *platform, ProcessLaunchInfo &launch_info,
     uint16_t *port, const Args *inferior_args, shared_fd_t pass_comm_fd) {
   Log *log = GetLog(GDBRLog::Process);
-  LLDB_LOGF(log, "GDBRemoteCommunication::%s(url=%s, port=%" PRIu16 ")",
-            __FUNCTION__, url ? url : "<empty>", port ? *port : uint16_t(0));
+  LLDB_LOG(log, "Starting debug server: url={0}, port={1}",
+           url ? url : "<empty>", port ? *port : uint16_t(0));
 
-  Status error;
-  FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
-  if ((debugserver_file_spec = GetDebugserverPath(platform))) {
-    std::string debugserver_path = debugserver_file_spec.GetPath();
+  FileSpec debugserver_file_spec = GetDebugserverPath(platform);
+  if (!debugserver_file_spec)
+    return Status::FromErrorString("unable to locate " DEBUGSERVER_BASENAME);
 
-    Args &debugserver_args = launch_info.GetArguments();
-    debugserver_args.Clear();
+  std::string debugserver_path = debugserver_file_spec.GetPath();
+  launch_info.SetExecutableFile(debugserver_file_spec,
+                                /*add_exe_file_as_first_arg=*/true);
 
-    // Start args with "debugserver /file/path -r --"
-    debugserver_args.AppendArgument(llvm::StringRef(debugserver_path));
+  Args &debugserver_args = launch_info.GetArguments();
 
 #if !defined(__APPLE__)
-    // First argument to lldb-server must be mode in which to run.
-    debugserver_args.AppendArgument(llvm::StringRef("gdbserver"));
+  // First argument to lldb-server must be mode in which to run.
+  debugserver_args.AppendArgument(llvm::StringRef("gdbserver"));
 #endif
 
-    // If a url is supplied then use it
-    if (url && url[0])
-      debugserver_args.AppendArgument(llvm::StringRef(url));
+  // If a url is supplied then use it
+  if (url && url[0])
+    debugserver_args.AppendArgument(llvm::StringRef(url));
 
-    if (pass_comm_fd != SharedSocket::kInvalidFD) {
-      StreamString fd_arg;
-      fd_arg.Printf("--fd=%" PRIi64, (int64_t)pass_comm_fd);
-      debugserver_args.AppendArgument(fd_arg.GetString());
-      // Send "pass_comm_fd" down to the inferior so it can use it to
-      // communicate back with this process. Ignored on Windows.
+  if (pass_comm_fd != SharedSocket::kInvalidFD) {
+    StreamString fd_arg;
+    fd_arg.Printf("--fd=%" PRIi64, (int64_t)pass_comm_fd);
+    debugserver_args.AppendArgument(fd_arg.GetString());
+    // Send "pass_comm_fd" down to the inferior so it can use it to
+    // communicate back with this process. Ignored on Windows.
 #ifndef _WIN32
-      launch_info.AppendDuplicateFileAction((int)pass_comm_fd,
-                                            (int)pass_comm_fd);
+    launch_info.AppendDuplicateFileAction((int)pass_comm_fd, (int)pass_comm_fd);
 #endif
-    }
+  }
 
-    // use native registers, not the GDB registers
-    debugserver_args.AppendArgument(llvm::StringRef("--native-regs"));
+  // use native registers, not the GDB registers
+  debugserver_args.AppendArgument(llvm::StringRef("--native-regs"));
 
-    if (launch_info.GetLaunchInSeparateProcessGroup()) {
-      debugserver_args.AppendArgument(llvm::StringRef("--setsid"));
-    }
+  if (launch_info.GetLaunchInSeparateProcessGroup())
+    debugserver_args.AppendArgument(llvm::StringRef("--setsid"));
 
-    llvm::SmallString<128> named_pipe_path;
-    // socket_pipe is used by debug server to communicate back either
-    // TCP port or domain socket name which it listens on.
-    // The second purpose of the pipe to serve as a synchronization point -
-    // once data is written to the pipe, debug server is up and running.
-    Pipe socket_pipe;
-
-    // port is null when debug server should listen on domain socket - we're
-    // not interested in port value but rather waiting for debug server to
-    // become available.
-    if (pass_comm_fd == SharedSocket::kInvalidFD) {
-      if (url) {
+  llvm::SmallString<128> named_pipe_path;
+  // socket_pipe is used by debug server to communicate back either
+  // TCP port or domain socket name which it listens on.
+  // The second purpose of the pipe to serve as a synchronization point -
+  // once data is written to the pipe, debug server is up and running.
+  Pipe socket_pipe;
+
+  std::unique_ptr<TCPSocket> sock_up;
+
+  // port is null when debug server should listen on domain socket - we're
+  // not interested in port value but rather waiting for debug server to
+  // become available.
+  if (pass_comm_fd == SharedSocket::kInvalidFD) {
+    if (url) {
 // Create a temporary file to get the stdout/stderr and redirect the output of
 // the command into this file. We will later read this file if all goes well
 // and fill the data into "command_output_ptr"
 #if defined(__APPLE__)
-        // Binding to port zero, we need to figure out what port it ends up
-        // using using a named pipe...
-        error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
-                                                 false, named_pipe_path);
-        if (error.Fail()) {
-          LLDB_LOGF(log,
-                    "GDBRemoteCommunication::%s() "
-                    "named pipe creation failed: %s",
-                    __FUNCTION__, error.AsCString());
-          return error;
-        }
-        debugserver_args.AppendArgument(llvm::StringRef("--named-pipe"));
-        debugserver_args.AppendArgument(named_pipe_path);
+      // Binding to port zero, we need to figure out what port it ends up
+      // using using a named pipe...
+      Status error = socket_pipe.CreateWithUniqueName("debugserver-named-pipe",
+                                                      false, named_pipe_path);
+      if (error.Fail()) {
+        LLDB_LOG(log, "named pipe creation failed: {0}", error);
+        return error;
+      }
+      debugserver_args.AppendArgument(llvm::StringRef("--named-pipe"));
+      debugserver_args.AppendArgument(named_pipe_path);
 #else
-        // Binding to port zero, we need to figure out what port it ends up
-        // using using an unnamed pipe...
-        error = socket_pipe.CreateNew(true);
-        if (error.Fail()) {
-          LLDB_LOGF(log,
-                    "GDBRemoteCommunication::%s() "
-                    "unnamed pipe creation failed: %s",
-                    __FUNCTION__, error.AsCString());
-          return error;
-        }
-        pipe_t write = socket_pipe.GetWritePipe();
-        debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
-        debugserver_args.AppendArgument(llvm::to_string(write));
-        launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
-#endif
-      } else {
-        // No host and port given, so lets listen on our end and make the
-        // debugserver connect to us..
-        error = StartListenThread("127.0.0.1", 0);
-        if (error.Fail()) {
-          LLDB_LOGF(log,
-                    "GDBRemoteCommunication::%s() unable to start listen "
-                    "thread: %s",
-                    __FUNCTION__, error.AsCString());
-          return error;
-        }
-
-        // Wait for 10 seconds to resolve the bound port
-        std::future<uint16_t> port_future = m_port_promise.get_future();
-        uint16_t port_ = port_future.wait_for(std::chrono::seconds(10)) ==
-                                 std::future_status::ready
-                             ? port_future.get()
-                             : 0;
-        if (port_ > 0) {
-          char port_cstr[32];
-          snprintf(port_cstr, sizeof(port_cstr), "127.0.0.1:%i", port_);
-          // Send the host and port down that debugserver and specify an option
-          // so that it connects back to the port we are listening to in this
-          // process
-          debugserver_args.AppendArgument(llvm::StringRef("--reverse-connect"));
-          debugserver_args.AppendArgument(llvm::StringRef(port_cstr));
-          if (port)
-            *port = port_;
-        } else {
-          LLDB_LOGF(log, "GDBRemoteCommunication::%s() failed: %s",
-                    __FUNCTION__, error.AsCString());
-          return Status::FromErrorString(
-              "failed to bind to port 0 on 127.0.0.1");
-        }
+      // Binding to port zero, we need to figure out what port it ends up
+      // using using an unnamed pipe...
+      Status error = socket_pipe.CreateNew(true);
+      if (error.Fail()) {
+        LLDB_LOG(log, "unnamed pipe creation failed: {0}", error);
+        return error;
       }
-    }
+      pipe_t write = socket_pipe.GetWritePipe();
+      debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
+      debugserver_args.AppendArgument(llvm::to_string(write));
+      launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
+#endif
+    } else {
+      // No host and port given, so lets listen on our end and make the
+      // debugserver connect to us..
+      if (llvm::Expected<std::unique_ptr<TCPSocket>> expected_sock =
+              Socket::TcpListen("127.0.0.1:0"))
+        sock_up = std::move(*expected_sock);
+      else
+        return Status::FromError(expected_sock.takeError());
 
-    Environment host_env = Host::GetEnvironment();
-    std::string env_debugserver_log_file =
-        host_env.lookup("LLDB_DEBUGSERVER_LOG_FILE");
-    if (!env_debugserver_log_file.empty()) {
+      uint16_t port_ = sock_up->GetLocalPortNumber();
+      // Send the host and port down that debugserver and specify an option
+      // so that it connects back to the port we are listening to in this
+      // process
+      debugserver_args.AppendArgument(llvm::StringRef("--reverse-connect"));
       debugserver_args.AppendArgument(
-          llvm::formatv("--log-file={0}", env_debugserver_log_file).str());
+          llvm::formatv("127.0.0.1:{0}", port_).str());
+      if (port)
+        *port = port_;
     }
+  }
+
+  Environment host_env = Host::GetEnvironment();
+  std::string env_debugserver_log_file =
+      host_env.lookup("LLDB_DEBUGSERVER_LOG_FILE");
+  if (!env_debugserver_log_file.empty()) {
+    debugserver_args.AppendArgument(
+        llvm::formatv("--log-file={0}", env_debugserver_log_file).str());
+  }
 
 #if defined(__APPLE__)
-    const char *env_debugserver_log_flags =
-        getenv("LLDB_DEBUGSERVER_LOG_FLAGS");
-    if (env_debugserver_log_flags) {
-      debugserver_args.AppendArgument(
-          llvm::formatv("--log-flags={0}", env_debugserver_log_flags).str());
-    }
+  const char *env_debugserver_log_flags = getenv("LLDB_DEBUGSERVER_LOG_FLAGS");
+  if (env_debugserver_log_flags) {
+    debugserver_args.AppendArgument(
+        llvm::formatv("--log-flags={0}", env_debugserver_log_flags).str());
+  }
 #else
-    std::string env_debugserver_log_channels =
-        host_env.lookup("LLDB_SERVER_LOG_CHANNELS");
-    if (!env_debugserver_log_channels.empty()) {
-      debugserver_args.AppendArgument(
-          llvm::formatv("--log-channels={0}", env_debugserver_log_channels)
-              .str());
-    }
+  std::string env_debugserver_log_channels =
+      host_env.lookup("LLDB_SERVER_LOG_CHANNELS");
+  if (!env_debugserver_log_channels.empty()) {
+    debugserver_args.AppendArgument(
+        llvm::formatv("--log-channels={0}", env_debugserver_log_channels)
+            .str());
+  }
 #endif
 
-    // Add additional args, starting with LLDB_DEBUGSERVER_EXTRA_ARG_1 until an
-    // env var doesn't come back.
-    uint32_t env_var_index = 1;
-    bool has_env_var;
-    do {
-      char env_var_name[64];
-      snprintf(env_var_name, sizeof(env_var_name),
-               "LLDB_DEBUGSERVER_EXTRA_ARG_%" PRIu32, env_var_index++);
-      std::string extra_arg = host_env.lookup(env_var_name);
-      has_env_var = !extra_arg.empty();
-
-      if (has_env_var) {
-        debugserver_args.AppendArgument(llvm::StringRef(extra_arg));
-        LLDB_LOGF(log,
-                  "GDBRemoteCommunication::%s adding env var %s contents "
-                  "to stub command line (%s)",
-                  __FUNCTION__, env_var_name, extra_arg.c_str());
-      }
-    } while (has_env_var);
-
-    if (inferior_args && inferior_args->GetArgumentCount() > 0) {
-      debugserver_args.AppendArgument(llvm::StringRef("--"));
-      debugserver_args.AppendArguments(*inferior_args);
+  // Add additional args, starting with LLDB_DEBUGSERVER_EXTRA_ARG_1 until an
+  // env var doesn't come back.
+  uint32_t env_var_index = 1;
+  bool has_env_var;
+  do {
+    char env_var_name[64];
+    snprintf(env_var_name, sizeof(env_var_name),
+             "LLDB_DEBUGSERVER_EXTRA_ARG_%" PRIu32, env_var_index++);
+    std::string extra_arg = host_env.lookup(env_var_name);
+    has_env_var = !extra_arg.empty();
+
+    if (has_env_var) {
+      debugserver_args.AppendArgument(llvm::StringRef(extra_arg));
+      LLDB_LOGF(log,
+                "GDBRemoteCommunication::%s adding env var %s contents "
+                "to stub command line (%s)",
+                __FUNCTION__, env_var_name, extra_arg.c_str());
     }
+  } while (has_env_var);
 
-    // Copy the current environment to the gdbserver/debugserver instance
-    launch_info.GetEnvironment() = host_env;
+  if (inferior_args && inferior_args->GetArgumentCount() > 0) {
+    debugserver_args.AppendArgument(llvm::StringRef("--"));
+    debugserver_args.AppendArguments(*inferior_args);
+  }
 
-    // Close STDIN, STDOUT and STDERR.
-    launch_info.AppendCloseFileAction(STDIN_FILENO);
-    launch_info.AppendCloseFileAction(STDOUT_FILENO);
-    launch_info.AppendCloseFileAction(STDERR_FILENO);
+  // Copy the current environment to the gdbserver/debugserver instance
+  launch_info.GetEnvironment() = host_env;
+
+  // Close STDIN, STDOUT and STDERR.
+  launch_info.AppendCloseFileAction(STDIN_FILENO);
+  launch_info.AppendCloseFileAction(STDOUT_FILENO);
+  launch_info.AppendCloseFileAction(STDERR_FILENO);
+
+  // Redirect STDIN, STDOUT and STDERR to "/dev/null".
+  launch_info.AppendSuppressFileAction(STDIN_FILENO, true, false);
+  launch_info.AppendSuppressFileAction(STDOUT_FILENO, false, true);
+  launch_info.AppendSuppressFileAction(STDERR_FILENO, false, true);
+
+  if (log) {
+    StreamString string_stream;
+    Platform *const platform = nullptr;
+    launch_info.Dump(string_stream, platform);
+    LLDB_LOG(log, "launch info for gdb-remote stub:\n{0}",
+             string_stream.GetData());
+  }
+  if (Status error = Host::LaunchProcess(launch_info); error.Fail()) {
+    LLDB_LOG(log, "launch failed: {0}", error);
+    return error;
+  }
 
-    // Redirect STDIN, STDOUT and STDERR to "/dev/null".
-    launch_info.AppendSuppressFileAction(STDIN_FILENO, true, false);
-    launch_info.AppendSuppressFileAction(STDOUT_FILENO, false, true);
-    launch_info.AppendSuppressFileAction(STDERR_FILENO, false, true);
+  if (pass_comm_fd != SharedSocket::kInvalidFD)
+    return Status();
 
-    if (log) {
-      StreamString string_stream;
-      Platform *const platform = nullptr;
-      launch_info.Dump(string_stream, platform);
-      LLDB_LOGF(log, "launch info for gdb-remote stub:\n%s",
-                string_stream.GetData());
+  Status error;
+  if (named_pipe_path.size() > 0) {
+    error = socket_pipe.OpenAsReader(named_pipe_path, false);
+    if (error.Fail()) {
+      LLDB_LOG(log, "failed to open named pipe {0} for reading: {1}",
+               named_pipe_path, error);
     }
-    error = Host::LaunchProcess(launch_info);
-
-    if (error.Success() &&
-        (launch_info.GetProcessID() != LLDB_INVALID_PROCESS_ID) &&
-        pass_comm_fd == SharedSocket::kInvalidFD) {
-      if (named_pipe_path.size() > 0) {
-        error = socket_pipe.OpenAsReader(named_pipe_path, false);
-        if (error.Fail())
-          LLDB_LOGF(log,
-                    "GDBRemoteCommunication::%s() "
-                    "failed to open named pipe %s for reading: %s",
-                    __FUNCTION__, named_pipe_path.c_str(), error.AsCString());
-      }
+  }
 
-      if (socket_pipe.CanWrite())
-        socket_pipe.CloseWriteFileDescriptor();
-      if (socket_pipe.CanRead()) {
-        // Read port from pipe with 10 second timeout.
-        std::string port_str;
-        while (error.Success()) {
-          char buf[10];
-          if (llvm::Expected<size_t> num_bytes = socket_pipe.Read(
-                  buf, std::size(buf), std::chrono::seconds(10))) {
-            if (*num_bytes == 0)
-              break;
-            port_str.append(buf, *num_bytes);
-          } else {
-            error = Status::FromError(num_bytes.takeError());
-          }
-        }
-        if (error.Success() && (port != nullptr)) {
-          // NB: Deliberately using .c_str() to stop at embedded '\0's
-          llvm::StringRef port_ref = port_str.c_str();
-          uint1...
[truncated]

[slydiman]

LGTM, thanks.
Currently GDBRemoteCommunication::ListenThread() to get the bound port is weird.