llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) <details> <summary>Changes</summary> Listen to gdbserver-port, accept the connection and run `lldb-server gdbserver --fd` on all platforms. Added acceptor_gdb and gdb_thread to lldb-platform.cpp SharedSocket has been moved to ConnectionFileDescriptorPosix. Parameters --min-gdbserver-port and --max-gdbserver-port are deprecated now. This is the part 2 of #<!-- -->101283. Fixes #<!-- -->97537. --- Patch is 61.67 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/104238.diff 17 Files Affected: - (modified) lldb/docs/man/lldb-server.rst (+2-9) - (modified) lldb/docs/resources/qemu-testing.rst (+5-14) - (modified) lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h (+26) - (modified) lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp (+109-3) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+24-17) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h (+6-2) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp (+97-193) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h (+13-70) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+1-1) - (removed) lldb/test/Shell/lldb-server/TestGdbserverPort.test (-4) - (modified) lldb/tools/lldb-server/Acceptor.cpp (+4-2) - (modified) lldb/tools/lldb-server/Acceptor.h (+3) - (modified) lldb/tools/lldb-server/lldb-gdbserver.cpp (+16-7) - (modified) lldb/tools/lldb-server/lldb-platform.cpp (+152-174) - (modified) lldb/unittests/Process/gdb-remote/CMakeLists.txt (-1) - (removed) lldb/unittests/Process/gdb-remote/PortMapTest.cpp (-115) - (modified) lldb/unittests/tools/lldb-server/tests/TestClient.h (+4) ``````````diff diff --git a/lldb/docs/man/lldb-server.rst b/lldb/docs/man/lldb-server.rst index a67c00b305f6d2..31f5360df5e23e 100644 --- a/lldb/docs/man/lldb-server.rst +++ b/lldb/docs/man/lldb-server.rst @@ -147,15 +147,8 @@ GDB-SERVER CONNECTIONS .. option:: --gdbserver-port <port> - Define a port to be used for gdb-server connections. Can be specified multiple - times to allow multiple ports. Has no effect if --min-gdbserver-port - and --max-gdbserver-port are specified. - -.. option:: --min-gdbserver-port <port> -.. option:: --max-gdbserver-port <port> - - Specify the range of ports that can be used for gdb-server connections. Both - options need to be specified simultaneously. Overrides --gdbserver-port. + Define a port to be used for gdb-server connections. This port is used for + multiple connections. .. option:: --port-offset <offset> diff --git a/lldb/docs/resources/qemu-testing.rst b/lldb/docs/resources/qemu-testing.rst index 51a30b11717a87..e102f84a1d31f4 100644 --- a/lldb/docs/resources/qemu-testing.rst +++ b/lldb/docs/resources/qemu-testing.rst @@ -149,7 +149,6 @@ to the host (refer to QEMU's manuals for the specific options). * At least one to connect to the intial ``lldb-server``. * One more if you want to use ``lldb-server`` in ``platform mode``, and have it start a ``gdbserver`` instance for you. -* A bunch more if you want to run tests against the ``lldb-server`` platform. If you are doing either of the latter 2 you should also restrict what ports ``lldb-server tries`` to use, otherwise it will randomly pick one that is almost @@ -157,22 +156,14 @@ certainly not forwarded. An example of this is shown below. :: - $ lldb-server plaform --server --listen 0.0.0.0:54321 \ - --min-gdbserver-port 49140 --max-gdbserver-port 49150 + $ lldb-server plaform --server --listen 0.0.0.0:54321 --gdbserver-port 49140 The result of this is that: * ``lldb-server`` platform mode listens externally on port ``54321``. -* When it is asked to start a new gdbserver mode instance, it will use a port - in the range ``49140`` to ``49150``. +* When it is asked to start a new gdbserver mode instance, it will use the port + ``49140``. -Your VM configuration should have ports ``54321``, and ``49140`` to ``49150`` -forwarded for this to work. - -.. note:: - These options are used to create a "port map" within ``lldb-server``. - Unfortunately this map is not cleaned up on Windows on connection close, - and across a few uses you may run out of valid ports. To work around this, - restart the platform every so often, especially after running a set of tests. - This is tracked here: https://github.com/llvm/llvm-project/issues/90923 +Your VM configuration should have ports ``54321`` and ``49140`` forwarded for +this to work. diff --git a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h index 35773d5907e913..08f486b92e0f07 100644 --- a/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h +++ b/lldb/include/lldb/Host/posix/ConnectionFileDescriptorPosix.h @@ -26,6 +26,32 @@ class Status; class Socket; class SocketAddress; +#ifdef _WIN32 +typedef lldb::pipe_t shared_fd_t; +#else +typedef NativeSocket shared_fd_t; +#endif + +class SharedSocket { +public: + static const shared_fd_t kInvalidFD; + + SharedSocket(Connection *conn, Status &error); + + shared_fd_t GetSendableFD() { return m_fd; } + + Status CompleteSending(lldb::pid_t child_pid); + + static Status GetNativeSocket(shared_fd_t fd, NativeSocket &socket); + +private: +#ifdef _WIN32 + Pipe m_socket_pipe; + NativeSocket m_socket; +#endif + shared_fd_t m_fd; +}; + class ConnectionFileDescriptor : public Connection { public: typedef llvm::function_ref<void(llvm::StringRef local_socket_id)> diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp index fceeff08ed9d36..fa7d2982d72238 100644 --- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp +++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp @@ -52,6 +52,94 @@ using namespace lldb; using namespace lldb_private; +#ifdef _WIN32 +const shared_fd_t SharedSocket::kInvalidFD = LLDB_INVALID_PIPE; +#else +const shared_fd_t SharedSocket::kInvalidFD = Socket::kInvalidSocketValue; +#endif + +SharedSocket::SharedSocket(Connection *conn, Status &error) { + m_fd = kInvalidFD; + + const Socket *socket = + static_cast<const Socket *>(conn->GetReadObject().get()); + if (socket == nullptr) { + error = Status("invalid conn socket"); + return; + } + +#ifdef _WIN32 + m_socket = socket->GetNativeSocket(); + + // Create a pipe to transfer WSAPROTOCOL_INFO to the child process. + error = m_socket_pipe.CreateNew(true); + if (error.Fail()) + return; + + m_fd = m_socket_pipe.GetReadPipe(); +#else + m_fd = socket->GetNativeSocket(); + error = Status(); +#endif +} + +Status SharedSocket::CompleteSending(lldb::pid_t child_pid) { +#ifdef _WIN32 + // Transfer WSAPROTOCOL_INFO to the child process. + m_socket_pipe.CloseReadFileDescriptor(); + + WSAPROTOCOL_INFO protocol_info; + if (::WSADuplicateSocket(m_socket, child_pid, &protocol_info) == + SOCKET_ERROR) { + int last_error = ::WSAGetLastError(); + return Status("WSADuplicateSocket() failed, error: %d", last_error); + } + + size_t num_bytes; + Status error = + m_socket_pipe.WriteWithTimeout(&protocol_info, sizeof(protocol_info), + std::chrono::seconds(10), num_bytes); + if (error.Fail()) + return error; + if (num_bytes != sizeof(protocol_info)) + return Status("WriteWithTimeout(WSAPROTOCOL_INFO) failed: %d bytes", + num_bytes); +#endif + return Status(); +} + +Status SharedSocket::GetNativeSocket(shared_fd_t fd, NativeSocket &socket) { +#ifdef _WIN32 + socket = Socket::kInvalidSocketValue; + // Read WSAPROTOCOL_INFO from the parent process and create NativeSocket. + WSAPROTOCOL_INFO protocol_info; + { + Pipe socket_pipe(fd, LLDB_INVALID_PIPE); + size_t num_bytes; + Status error = + socket_pipe.ReadWithTimeout(&protocol_info, sizeof(protocol_info), + std::chrono::seconds(10), num_bytes); + if (error.Fail()) + return error; + if (num_bytes != sizeof(protocol_info)) { + return Status( + "socket_pipe.ReadWithTimeout(WSAPROTOCOL_INFO) failed: % d bytes", + num_bytes); + } + } + socket = ::WSASocket(FROM_PROTOCOL_INFO, FROM_PROTOCOL_INFO, + FROM_PROTOCOL_INFO, &protocol_info, 0, 0); + if (socket == INVALID_SOCKET) { + return Status("WSASocket(FROM_PROTOCOL_INFO) failed: error %d", + ::WSAGetLastError()); + } + return Status(); +#else + socket = fd; + return Status(); +#endif +} + ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit) : Connection(), m_pipe(), m_mutex(), m_shutting_down(false), @@ -149,7 +237,7 @@ ConnectionFileDescriptor::Connect(llvm::StringRef path, if (!path.empty()) { auto method = - llvm::StringSwitch<ConnectionStatus (ConnectionFileDescriptor::*)( + llvm::StringSwitch<ConnectionStatus (ConnectionFileDescriptor:: *)( llvm::StringRef, socket_id_callback_type, Status *)>(scheme) .Case("listen", &ConnectionFileDescriptor::AcceptTCP) .Cases("accept", "unix-accept", @@ -162,8 +250,10 @@ ConnectionFileDescriptor::Connect(llvm::StringRef path, .Case("unix-connect", &ConnectionFileDescriptor::ConnectNamedSocket) .Case("unix-abstract-connect", &ConnectionFileDescriptor::ConnectAbstractSocket) -#if LLDB_ENABLE_POSIX +#if LLDB_ENABLE_POSIX || defined(_WIN32) .Case("fd", &ConnectionFileDescriptor::ConnectFD) +#endif +#if LLDB_ENABLE_POSIX .Case("file", &ConnectionFileDescriptor::ConnectFile) .Case("serial", &ConnectionFileDescriptor::ConnectSerialPort) #endif @@ -666,7 +756,23 @@ ConnectionStatus ConnectionFileDescriptor::ConnectFD(llvm::StringRef s, socket_id_callback_type socket_id_callback, Status *error_ptr) { -#if LLDB_ENABLE_POSIX +#ifdef _WIN32 + int64_t fd = -1; + if (!s.getAsInteger(0, fd)) { + // Assume we own fd. + std::unique_ptr<TCPSocket> tcp_socket = + std::make_unique<TCPSocket>((NativeSocket)fd, true, false); + m_io_sp = std::move(tcp_socket); + m_uri = s.str(); + return eConnectionStatusSuccess; + } + + if (error_ptr) + error_ptr->SetErrorStringWithFormat("invalid file descriptor: \"%s\"", + s.str().c_str()); + m_io_sp.reset(); + return eConnectionStatusError; +#elif LLDB_ENABLE_POSIX // Just passing a native file descriptor within this current process that // is already opened (possibly from a service or other source). int fd = -1; diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index 5d0a3e31d04374..075312016e0b22 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -879,20 +879,12 @@ 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) { +bool GDBRemoteCommunication::GetDebugserverPath( + Platform *platform, FileSpec &debugserver_file_spec) { 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(); - Environment host_env = Host::GetEnvironment(); // Always check to see if we have an environment override for the path to the @@ -943,8 +935,20 @@ Status GDBRemoteCommunication::StartDebugserverProcess( } } } + return debugserver_exists; +} - 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 (GetDebugserverPath(platform, debugserver_file_spec)) { + char debugserver_path[PATH_MAX]; debugserver_file_spec.GetPath(debugserver_path, sizeof(debugserver_path)); Args &debugserver_args = launch_info.GetArguments(); @@ -962,13 +966,14 @@ Status GDBRemoteCommunication::StartDebugserverProcess( if (url) debugserver_args.AppendArgument(llvm::StringRef(url)); - if (pass_comm_fd >= 0) { + if (pass_comm_fd != SharedSocket::kInvalidFD) { StreamString fd_arg; - fd_arg.Printf("--fd=%i", pass_comm_fd); + 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 - launch_info.AppendDuplicateFileAction(pass_comm_fd, pass_comm_fd); + // communicate back with this process. Ignored on Windows. + launch_info.AppendDuplicateFileAction((int)pass_comm_fd, + (int)pass_comm_fd); } // use native registers, not the GDB registers @@ -988,7 +993,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess( // 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 == -1) { + 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 @@ -1059,6 +1064,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()) { @@ -1132,7 +1139,7 @@ Status GDBRemoteCommunication::StartDebugserverProcess( if (error.Success() && (launch_info.GetProcessID() != LLDB_INVALID_PROCESS_ID) && - pass_comm_fd == -1) { + pass_comm_fd == SharedSocket::kInvalidFD) { if (named_pipe_path.size() > 0) { error = socket_pipe.OpenAsReader(named_pipe_path, false); if (error.Fail()) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index 4e59bd5ec8be6b..bbff31de8aafd6 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -20,6 +20,7 @@ #include "lldb/Core/Communication.h" #include "lldb/Host/Config.h" +#include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/HostThread.h" #include "lldb/Utility/Args.h" #include "lldb/Utility/Listener.h" @@ -146,6 +147,9 @@ class GDBRemoteCommunication : public Communication { std::chrono::seconds GetPacketTimeout() const { return m_packet_timeout; } + // Get the debugserver path and check that it exist. + bool GetDebugserverPath(Platform *platform, FileSpec &debugserver_file_spec); + // Start a debugserver instance on the current host using the // supplied connection URL. Status StartDebugserverProcess( @@ -153,8 +157,8 @@ class GDBRemoteCommunication : public Communication { 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, - int pass_comm_fd); // Communication file descriptor to pass during - // fork/exec to avoid having to connect/accept + shared_fd_t pass_comm_fd); // Communication file descriptor to pass during + // fork/exec to avoid having to connect/accept 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 65f1cc12ba307b..5c51c9afbe76df 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp @@ -40,83 +40,20 @@ #include "lldb/Utility/StringExtractorGDBRemote.h" +#include "../tools/lldb-server/Acceptor.h" + using namespace lldb; using namespace lldb_private::process_gdb_remote; using namespace lldb_private; -GDBRemoteCommunicationServerPlatform::PortMap::PortMap(uint16_t min_port, - uint16_t max_port) { - assert(min_port); - for (; min_port < max_port; ++min_port) - m_port_map[min_port] = LLDB_INVALID_PROCESS_ID; -} - -void GDBRemoteCommunicationServerPlatform::PortMap::AllowPort(uint16_t port) { - assert(port); - // Do not modify existing mappings - m_port_map.insert({port, LLDB_INVALID_PROCESS_ID}); -} - -llvm::Expected<uint16_t> -GDBRemoteCommunicationServerPlatform::PortMap::GetNextAvailablePort() { - if (m_port_map.empty()) - return 0; // Bind to port zero and get a port, we didn't have any - // limitations - - for (auto &pair : m_port_map) { - if (pair.second == LLDB_INVALID_PROCESS_ID) { - pair.second = ~(lldb::pid_t)LLDB_INVALID_PROCESS_ID; - return pair.first; - } - } - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "No free port found in port map"); -} - -bool GDBRemoteCommunicationServerPlatform::PortMap::AssociatePortWithProcess( - uint16_t port, lldb::pid_t pid) { - auto pos = m_port_map.find(port); - if (pos != m_port_map.end()) { - pos->second = pid; - return true; - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::PortMap::FreePort(uint16_t port) { - std::map<uint16_t, lldb::pid_t>::iterator pos = m_port_map.find(port); - if (pos != m_port_map.end()) { - pos->second = LLDB_INVALID_PROCESS_ID; - return true; - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::PortMap::FreePortForProcess( - lldb::pid_t pid) { - if (!m_port_map.empty()) { - for (auto &pair : m_port_map) { - if (pair.second == pid) { - pair.second = LLDB_INVALID_PROCESS_ID; - return true; - } - } - } - return false; -} - -bool GDBRemoteCommunicationServerPlatform::PortMap::empty() const { - return m_port_map.empty(); -} +std::mutex GDBRemoteCommunicationServerPlatform::g_spawned_pids_mutex; +std::set<lldb::pid_t> GDBRemoteCommunicationServerPlatform::g_spawned_pids; // GDBRemoteCommunicationServerPlatform constructor GDBRemoteCommunicationServerPlatform::GDBRemoteCommunicationServerPlatform( - const Socket::SocketProtocol socket_protocol, const char *socket_scheme) - : GDBRemoteCommunicationServerCommon(), - m_socket_protocol(socket_protocol), m_socket_scheme(socket_scheme), - m_spawned_pids_mutex(), m_port_map(), m_port_offset(0) { - m_pending_gdb_server.pid = LLDB_INVALID_PROCESS_ID; - m_pending_gdb_server.port = 0; + const Socket::SocketProtocol socket_protocol, uint16_t gdbserver_port) + : GDBRemoteCommunicationServerCommon(), m_socket_protocol(socket_protocol), + m_gdbserver_port(gdbserver_port), m_port_offset(0) { RegisterMemberFunctionHandler( StringExtractorGDBRemote::eServerPacketType_qC, @@ -160,66 +97,59 @@ GDBRemoteCommunicationServerPlatform::~GDBRemoteCommunicationServerPlatform() = default; Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( - const lldb_private::Args &args, std::string hostname, lldb::pid_t &pid, - std::optional<uint16_t> &port, std::string &socket_name) { - if (!port) { - llvm::Expected<uint16_t> available_port = m_port_map.GetNextAvailablePort(); - if (available_port) - port = *available_port; - else - return Status(available_port.takeError()); - } - - // Spawn a new thread to accept the port that gets bound after binding to - // port 0 (zero). + const lldb_private::Args &args, lldb::pid_t &pid, std::string &socket_name, + shared_fd_t fd) { + std::ostringstream url; + if (fd == SharedSocket::kInvalidFD) { + if (m_socket_protocol == Socket::ProtocolTcp) { + // Just check that GDBServer exists. GDBServer must be launched after + // accepting the connection. + FileSpec debugserver_file_spec; + if (!GetDebugserverPath(nullptr, debugserver_file_spec)) + return Status("unable to locate debugserver"); + return Status(); + } - // ignore the hostname send from the remote end, just use the ip address that - // we're currently communicating with as the hostname + // debugserver does not accept the URL scheme prefix. +#if !defined(__APPLE__) + url << lldb_server::Acceptor::FindSchemeByProtocol(m_socket_protocol) + << "://"; +#endif + socket_name = GetDomainSocketPath("gdbserver").GetPath(); + url << socket_name; + } else { + if (m_socket_protocol != Socket::ProtocolTcp) + return Status("protocol must be tcp"); + } // Spawn a debugserver and try to get the port it listens to. ProcessLaunchInfo debugserver_launch_info; - if (hostname.empty()) - hostname = "127.0.0.1"; - Log *log = GetLog(LLDBLog::Platform); - LLDB_LOGF(log, "Launching debugserver with: %s:%u...", hostname.c_st... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/104238 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
