[lldb][NFC] Separated GDBRemoteCommunication::GetDebugserverPath()

[slydiman]

This is the prerequisite for #104238.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

This is the prerequisite for #104238.

---

Full diff: https://github.com/llvm/llvm-project/pull/107388.diff

2 Files Affected:

• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+17-11)
• (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h (+3)

diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index 50fa11e916c5f5..3ac58859cecafb 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -879,19 +879,11 @@ lldb::thread_result_t GDBRemoteCommunication::ListenThread() {
   return {};
 }
 
-Status GDBRemoteCommunication::StartDebugserverProcess(
-    const char *url, Platform *platform, ProcessLaunchInfo &launch_info,
-    uint16_t *port, const Args *inferior_args, int pass_comm_fd) {
+FileSpec GDBRemoteCommunication::GetDebugserverPath(Platform *platform) {
   Log *log = GetLog(GDBRLog::Process);
-  LLDB_LOGF(log, "GDBRemoteCommunication::%s(url=%s, port=%" PRIu16 ")",
-            __FUNCTION__, url ? url : "<empty>", port ? *port : uint16_t(0));
-
-  Status error;
   // If we locate debugserver, keep that located version around
   static FileSpec g_debugserver_file_spec;
-
-  char debugserver_path[PATH_MAX];
-  FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
+  FileSpec debugserver_file_spec;
 
   Environment host_env = Host::GetEnvironment();
 
@@ -943,8 +935,20 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
       }
     }
   }
+  return debugserver_file_spec;
+}
 
-  if (debugserver_exists) {
+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));
+
+  Status error;
+  FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
+  if (debugserver_file_spec = GetDebugserverPath(platform)) {
+    char debugserver_path[PATH_MAX];
     debugserver_file_spec.GetPath(debugserver_path, sizeof(debugserver_path));
 
     Args &debugserver_args = launch_info.GetArguments();
@@ -1059,6 +1063,8 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
         }
       }
     }
+
+    Environment host_env = Host::GetEnvironment();
     std::string env_debugserver_log_file =
         host_env.lookup("LLDB_DEBUGSERVER_LOG_FILE");
     if (!env_debugserver_log_file.empty()) {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
index 4e59bd5ec8be6b..a182291f6c8594 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h
@@ -146,6 +146,9 @@ class GDBRemoteCommunication : public Communication {
 
   std::chrono::seconds GetPacketTimeout() const { return m_packet_timeout; }
 
+  // Get the debugserver path and check that it exist.
+  FileSpec GetDebugserverPath(Platform *platform);
+
   // Start a debugserver instance on the current host using the
   // supplied connection URL.
   Status StartDebugserverProcess(

[JDevlieghere]

LGTM modulo one nit.

[JDevlieghere]

Looks like we're immediately converting this to a StringRef. Since we're already touching this line, could we make this std::string debugserver_path = debugserver_file_spec.GetPath()?

[jasonmolenda]

Agree with this comment, otherwise this change looks fine to me.

[labath]

+1

[labath]

+1

[slydiman]

Oops, I copied shared_fd_t from #104238 by mistake

Status GDBRemoteCommunication::StartDebugserverProcess(... , shared_fd_t pass_comm_fd) {

It will break the Windows build.
I can replace it with int or we can apply #107553.
Please advise.

[mstorsjo]

Oops, I copied shared_fd_t from #104238 by mistake

Status GDBRemoteCommunication::StartDebugserverProcess(... , shared_fd_t pass_comm_fd) {

It will break the Windows build. I can replace it with int or we can apply #107553. Please advise.

As someone who just ran into this build failure - I would prefer to first push a minimal fixup commit to change it to int, to unbreak the build, and then rebase #107553 on top of that. Even if #107553 is meant to be NFC, it's a much larger change.

[slydiman]

Updated. Thanks.

[mstorsjo]

Thanks for the quick fix!