Author: Pavel Labath Date: 2025-06-25T08:09:36+02:00 New Revision: 0512d119fdf019b7c56e58f89b094ee5928b0a07
URL: https://github.com/llvm/llvm-project/commit/0512d119fdf019b7c56e58f89b094ee5928b0a07 DIFF: https://github.com/llvm/llvm-project/commit/0512d119fdf019b7c56e58f89b094ee5928b0a07.diff LOG: [lldb] Clean up GDBRemoteCommunication::StartDebugserverProcess (#145021) The function was extremely messy in that it, depending on the set of arguments, it could either modify the Connection object in `this` or not. It had a lot of arguments, with each call site passing a different combination of null values. This PR: - packs "url" and "comm_fd" arguments into a variant as they are mutually exclusive - removes the (surprising) "null url *and* null comm_fd" code path which is not used as of https://github.com/llvm/llvm-project/pull/145017 - marks the function as `static` to make it clear it (now) does not operate on the `this` object. Depends on #145017 Added: Modified: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp Removed: ################################################################################ diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 0d3ead840b080..bea2faff2330e 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -31,6 +31,7 @@ #include <climits> #include <cstring> #include <sys/stat.h> +#include <variant> #if defined(__APPLE__) #define DEBUGSERVER_BASENAME "debugserver" @@ -894,11 +895,9 @@ FileSpec GDBRemoteCommunication::GetDebugserverPath(Platform *platform) { } Status GDBRemoteCommunication::StartDebugserverProcess( - const char *url, Platform *platform, ProcessLaunchInfo &launch_info, - uint16_t *port, const Args *inferior_args, shared_fd_t pass_comm_fd) { + std::variant<llvm::StringRef, shared_fd_t> comm, Platform *platform, + ProcessLaunchInfo &launch_info, const Args *inferior_args) { Log *log = GetLog(GDBRLog::Process); - LLDB_LOG(log, "Starting debug server: url={0}, port={1}", - url ? url : "<empty>", port ? *port : uint16_t(0)); FileSpec debugserver_file_spec = GetDebugserverPath(platform); if (!debugserver_file_spec) @@ -911,89 +910,58 @@ Status GDBRemoteCommunication::StartDebugserverProcess( #if !defined(__APPLE__) // First argument to lldb-server must be mode in which to run. - debugserver_args.AppendArgument(llvm::StringRef("gdbserver")); + debugserver_args.AppendArgument("gdbserver"); #endif - // 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. - launch_info.AppendDuplicateFileAction((int64_t)pass_comm_fd, - (int64_t)pass_comm_fd); - } - // use native registers, not the GDB registers - debugserver_args.AppendArgument(llvm::StringRef("--native-regs")); + debugserver_args.AppendArgument("--native-regs"); if (launch_info.GetLaunchInSeparateProcessGroup()) - debugserver_args.AppendArgument(llvm::StringRef("--setsid")); + debugserver_args.AppendArgument("--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 - + // TCP port or domain socket name which it listens on. However, we're not + // interested in the actualy value here. + // The only reason for using the pipe is 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; + // If a url is supplied then use it + if (shared_fd_t *comm_fd = std::get_if<shared_fd_t>(&comm)) { + LLDB_LOG(log, "debugserver communicates over fd {0}", comm_fd); + assert(*comm_fd != SharedSocket::kInvalidFD); + debugserver_args.AppendArgument(llvm::formatv("--fd={0}", *comm_fd).str()); + // Send "comm_fd" down to the inferior so it can use it to communicate back + // with this process. + launch_info.AppendDuplicateFileAction((int64_t)*comm_fd, (int64_t)*comm_fd); + } else { + llvm::StringRef url = std::get<llvm::StringRef>(comm); + LLDB_LOG(log, "debugserver listens on: {0}", url); + debugserver_args.AppendArgument(url); - // 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... - 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); + // Using a named pipe as debugserver does not support --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... - 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()); - - 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("127.0.0.1:{0}", port_).str()); - if (port) - *port = port_; + // Using an unnamed pipe as it's simpler. + 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 } Environment host_env = Host::GetEnvironment(); @@ -1070,7 +1038,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess( return error; } - if (pass_comm_fd != SharedSocket::kInvalidFD) + if (std::holds_alternative<shared_fd_t>(comm)) return Status(); Status error; @@ -1084,55 +1052,30 @@ Status GDBRemoteCommunication::StartDebugserverProcess( 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(); - uint16_t child_port = 0; - // FIXME: improve error handling - llvm::to_integer(port_ref, child_port); - if (*port == 0 || *port == child_port) { - *port = child_port; - LLDB_LOG(log, "debugserver listens on port {0}", *port); - } else { - LLDB_LOG(log, - "debugserver listening on port {0} but requested port was {1}", - child_port, (*port)); - } + assert(socket_pipe.CanRead()); + + // Read data from the pipe -- and ignore it (see comment above). + 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; } else { - LLDB_LOG(log, "failed to read a port value from pipe {0}: {1}", - named_pipe_path, error); + error = Status::FromError(num_bytes.takeError()); } - socket_pipe.Close(); } + if (error.Fail()) { + LLDB_LOG(log, "failed to synchronize on pipe {0}: {1}", named_pipe_path, + error); + } + socket_pipe.Close(); if (named_pipe_path.size() > 0) { if (Status err = socket_pipe.Delete(named_pipe_path); err.Fail()) LLDB_LOG(log, "failed to delete pipe {0}: {1}", named_pipe_path, err); } - if (error.Success() && sock_up) { - Socket *accepted_socket = nullptr; - error = sock_up->Accept(/*timeout=*/std::nullopt, accepted_socket); - if (accepted_socket) { - SetConnection(std::make_unique<ConnectionFileDescriptor>( - std::unique_ptr<Socket>(accepted_socket))); - } - } - return error; } diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index fc86f801f0d8a..31f8edf715a3a 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -135,17 +135,15 @@ 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); + static FileSpec GetDebugserverPath(Platform *platform); // Start a debugserver instance on the current host using the // supplied connection URL. - Status StartDebugserverProcess( - const char *url, + static Status StartDebugserverProcess( + std::variant<llvm::StringRef, shared_fd_t> comm, Platform *platform, // If non nullptr, then check with the platform for // the GDB server binary if it can't be located - ProcessLaunchInfo &launch_info, uint16_t *port, const Args *inferior_args, - shared_fd_t pass_comm_fd); // Communication file descriptor to pass during - // fork/exec to avoid having to connect/accept + ProcessLaunchInfo &launch_info, const Args *inferior_args); void DumpHistory(Stream &strm); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp index 89fdfa74bc025..7506cf64def38 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp @@ -94,7 +94,16 @@ GDBRemoteCommunicationServerPlatform::~GDBRemoteCommunicationServerPlatform() = Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( const lldb_private::Args &args, lldb::pid_t &pid, std::string &socket_name, shared_fd_t fd) { - std::ostringstream url; + Log *log = GetLog(LLDBLog::Platform); + + ProcessLaunchInfo debugserver_launch_info; + // Do not run in a new session so that it can not linger after the platform + // closes. + debugserver_launch_info.SetLaunchInSeparateProcessGroup(false); + debugserver_launch_info.SetMonitorProcessCallback( + [](lldb::pid_t, int, int) {}); + + Status error; if (fd == SharedSocket::kInvalidFD) { if (m_socket_protocol == Socket::ProtocolTcp) { // Just check that GDBServer exists. GDBServer must be launched after @@ -104,31 +113,22 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( return Status(); } + std::ostringstream url; // debugserver does not accept the URL scheme prefix. #if !defined(__APPLE__) url << Socket::FindSchemeByProtocol(m_socket_protocol) << "://"; #endif socket_name = GetDomainSocketPath("gdbserver").GetPath(); url << socket_name; + error = StartDebugserverProcess(url.str(), nullptr, debugserver_launch_info, + &args); } else { if (m_socket_protocol != Socket::ProtocolTcp) return Status::FromErrorString("protocol must be tcp"); + error = + StartDebugserverProcess(fd, nullptr, debugserver_launch_info, &args); } - // Spawn a debugserver and try to get the port it listens to. - ProcessLaunchInfo debugserver_launch_info; - Log *log = GetLog(LLDBLog::Platform); - LLDB_LOG(log, "Launching debugserver url='{0}', fd={1}...", url.str(), fd); - - // Do not run in a new session so that it can not linger after the platform - // closes. - debugserver_launch_info.SetLaunchInSeparateProcessGroup(false); - debugserver_launch_info.SetMonitorProcessCallback( - [](lldb::pid_t, int, int) {}); - - Status error = StartDebugserverProcess( - url.str().c_str(), nullptr, debugserver_launch_info, nullptr, &args, fd); - if (error.Success()) { pid = debugserver_launch_info.GetProcessID(); AddSpawnedProcess(pid); diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 4e70fe8ac1595..3f9c4ddc60a25 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -3494,9 +3494,9 @@ Status ProcessGDBRemote::LaunchAndConnectToDebugserver( if (error.Fail()) return error; - error = m_gdb_comm.StartDebugserverProcess( - nullptr, GetTarget().GetPlatform().get(), debugserver_launch_info, - nullptr, nullptr, shared_socket.GetSendableFD()); + error = m_gdb_comm.StartDebugserverProcess(shared_socket.GetSendableFD(), + GetTarget().GetPlatform().get(), + debugserver_launch_info, nullptr); if (error.Fail()) { Log *log = GetLog(GDBRLog::Process); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
