[lldb] Removed gdbserver ports map from lldb-server #104238
Conversation
|
@llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) ChangesListen to gdbserver-port, accept the connection and run 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:
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]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
slydiman
force-pushed
the
lldb-server-remove-gdb-ports-map
branch
from
46808aa to
5732bda
Compare
|
Thanks for putting so much work into this. I will test this out with my VM setup.
Are they just consumed but the port number is not used because it can use the first port for everything? (I do see the logic of leaving the options in at first, so we can tell whether any problems are from the changes in this PR or purely from removing the options) |
| bool GDBRemoteCommunication::GetDebugserverPath( | ||
| Platform *platform, FileSpec &debugserver_file_spec) { |
There was a problem hiding this comment.
| bool GDBRemoteCommunication::GetDebugserverPath( | |
| Platform *platform, FileSpec &debugserver_file_spec) { | |
| FileSpec GDBRemoteCommunication::GetDebugserverPath( | |
| Platform *platform) { |
just return an invalid FileSpec for error.
There was a problem hiding this comment.
I tried to minimize changes. bool debugserver_exists already was here.
Note debugserver_file_spec is initialized in the following code by reference.
Returning FileSpec complicates and slows down the code.
FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
if (GetDebugserverPath(platform, debugserver_file_spec)) {
...
There was a problem hiding this comment.
Minimizing changes is not what I'm concerned with. This change in particular could be easily split off into its own patch.
I don't think
FileSpec &debugserver_file_spec = launch_info.GetExecutableFile();
if (GetDebugserverPath(platform, debugserver_file_spec)) {
is inherently better than
launch_info.GetExecutableFile() = GetDebugserverPath(platform);
if (launch_info.GetExecutableFile()) {
or
if (FileSpec debugserver_file_spec = GetDebugserverPath(platform)) {
launch_info.GetExecutableFile() = debugserver_file_spec;
In fact, I'd say it's the opposite, because that pattern leaves it ambiguous as to what is the value of the by-ref argument in the failure case (is it unchanged, empty/invalid, or undefined)?
| @@ -40,83 +40,20 @@ | |||
|
|
|||
| #include "lldb/Utility/StringExtractorGDBRemote.h" | |||
|
|
|||
| #include "../tools/lldb-server/Acceptor.h" | |||
There was a problem hiding this comment.
We will need to move this someplace else (probably Host)
There was a problem hiding this comment.
I have separated this change to #104439.
| static int g_debug = 0; | ||
| static int g_verbose = 0; | ||
| static int g_server = 0; | ||
| static volatile bool g_listen_gdb = true; |
There was a problem hiding this comment.
I expected to use this for break the gdb port listener thread. But currently there is no public API to close listen socket. See FIXME at the end of lldb-platform.cpp. I think we can just exit the app safely without graceful terminating this thread.
There was a problem hiding this comment.
yeah, but volatile still isn't the right choice for that. It would have to be an atomic var.
| // FIXME: Make TCPSocket::CloseListenSockets() public and implement | ||
| // Acceptor::Close(). | ||
| /* | ||
| if (acceptor_gdb && gdb_thread.IsJoinable()) { | ||
| g_listen_gdb = false; | ||
| static_cast<TCPSocket *>(acceptor_gdb->m_listener_socket_up.get()) | ||
| ->CloseListenSockets(); | ||
| lldb::thread_result_t result; | ||
| gdb_thread.Join(&result); | ||
| } | ||
| */ | ||
|
|
There was a problem hiding this comment.
This won't work. This is exactly the pattern from that kernel bug we were arguing over. It's just not possible to safely close an fd while another thread may be operating on it.
You need to use an out-of-band mechanism (e.g. a pipe) to notify the thread that it's time to terminate. However, since that will involve some form of multiplexing (e.g. select(2)), I would strongly prefer to just multiplex over the two sockets we are accepting (and ditch the extra thread). We should be able to use the MainLoop class for that -- just register the two sockets with the class, and let it call the right callback when needed. Basically, the code should look similar to this, except that it will be listening to two unrelated sockets.
There was a problem hiding this comment.
Oh, Linux... Such pattern works just fine on Windows. I saw MainLoop. But I think it is much safer just exit the app. Terminating the main thread is enough. I will remove this comment in the code and the g_listen_gdb flag.
There was a problem hiding this comment.
<rant>It's not just linux, that kind of pattern is racy on all posix operating systems. It's possible that other OSs will not hang in this situation, but that doesn't mean that usage is safe. Even on windows, that can cause you to call (WSA)accept on a dangling socket descriptor -- since you can never be sure whether the other thread has already called the function, or is it merely about to do that. It's less likely to blow up in your face because windows does not recycle FDs like posix systems do, but I wouldn't recommend that anywhere.</rant>
Exiting the main thread may be convenient, but I definitely wouldn't call it "safe", as that means the other thread may be running, executing random code and memory, while the main thread is shutting everything down and deleting that memory. It may not matter often, as the process is done anyway, but if anyone was observing the exit code, he may be suprised to see a SEGV instead of a clean exit.
Reasoning about a singlethreaded application is much easier than for a multithreaded one. We should already have all of the pieces to do this the right way, so let's just do that.
| #ifdef SendMessage | ||
| #undef SendMessage | ||
| #endif | ||
|
|
There was a problem hiding this comment.
I guess that means we're including a windows header somewhere. Any chance to avoid that? Llvm is pretty strict about that. Lldb is not, though I think it's a good practice.
There was a problem hiding this comment.
Note TestClient.cpp and LLGSTest.cpp already contain this code. Windows headers are necessary for socket definitions. It is impossible to separate them. This fix is necessary because the order of headers has been changed. I have removed the same code in TestClient.cpp and LLGSTest.cpp.
There was a problem hiding this comment.
If it's just due to header moves that's fine.
It's definitely possible to isolate the windows headers. Lllvm does that by making sure it only includes the system headers in implementation files (of the library wrapping them). It'd be nice to have lldb do the same thing, but we're pretty far off.
We need only 1 port defined by 1235 is used for gdb 1236 is used for gdb 1237 is used for gdb |
This is the prerequisite for llvm#104238.
|
Only had time to build so far but: I think this warning was already there, I'm just seeing it now in a clean build. |
I will revert the original definition |
|
I did some testing with my usual setup which is x86 Linux to AArch64 Linux running in a simulator. The SVE tests are the ones that found #97537. And everything works as expected. I don't normally run remote tests with Snapshot of the processes during that, all makes sense to me. Main platform, two connected clients and each one has spawned a gdbserver. |
Note we are testing
Currently we have 2 tests failed on Windows host/local. But these fails are not related to this patch. |
slydiman
force-pushed
the
lldb-server-remove-gdb-ports-map
branch
from
04caaec to
d61a1c2
Compare
slydiman
force-pushed
the
lldb-server-remove-gdb-ports-map
branch
2 times, most recently
from
5e37411 to
b691617
Compare
This is prerequisite for llvm#104238.
…thread This is prerequisite for llvm#104238.
This is the prerequisite for #104238.
slydiman
force-pushed
the
lldb-server-remove-gdb-ports-map
branch
from
b691617 to
1d4949f
Compare
Is this the only argument? I don't like globals too. But no one suggested better solution w/o huge refactoring of MonitorProcessCallback. Updated. |
No, it's not. You can find my second reason in the full text of my previous message. The third reason is that I think this is something that should be handled independenly of this (already larger that I'd like) patch.
You didn't ask for one either. Declaring it's the only way isn't the most traditional way of soliciting feedback.
👍 I'm going to look at the patch now. |
| // Just check that GDBServer exists. GDBServer must be launched after | ||
| // accepting the connection. | ||
| if (!GetDebugserverPath(nullptr)) | ||
| return Status::FromErrorString("unable to locate debugserver"); | ||
| return Status(); |
There was a problem hiding this comment.
I am still looking for an answer to this one. If it's only the Handle_qLaunchGDBServer that needs to not launch an lldb-server then I don't see why the check couldn't/shouldn't be performed there.
| // verify that we know anything about this pid. | ||
| if (SpawnedProcessFinished(pid)) { | ||
| // not a pid we know about | ||
| return SendErrorResponse(10); // ECHILD |
There was a problem hiding this comment.
Let's get rid of these comments. If we really wanted these to have a meaning, they should be one of the GDB_ESOMETHING constants defined in GDBRemoteCommunication.h.
| m_spawned_pids.insert(pid); | ||
| } | ||
|
|
||
| bool GDBRemoteCommunicationServerPlatform::SpawnedProcessFinished( |
There was a problem hiding this comment.
I'd suggest inverting this condition and calling this SpawnedProcessIsRunning (or sth like that). The reason being that "Finished" implies that process was running at some point, but this function does not distinguish a terminated process from one that was never run.
| return Status(); | ||
| } | ||
|
|
||
| static Status AcceptGdbConnectionsIfNeeded( |
There was a problem hiding this comment.
return Expected<handles> here.
I cannot answer in the thread, no idea why. The answer is here LaunchGDBServer() may be called from the child process at the beginnig if inferior_arguments are present. The behavior depends on the protocol. gdbserver will be launched in case of unix sockets. Otherwise we need to check that gdbserver is exists and wait for the tcp gdb connection. This part cannot be optimized/simplified. |
Okay, that makes sense. Let's see how this goes. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/7720 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/4885 Here is the relevant piece of the build log for the reference |
This patch fixes the following problems https://lab.llvm.org/buildbot/#/builders/162/builds/7720
This test is flaky on Arm 32, I'll figure it out. |
…m#104439) This is the prerequisite for llvm#104238. (cherry picked from commit 899a3df)
This is the prerequisite for llvm#104238. (cherry picked from commit 06ccd32)
|
Hi @slydiman, I am looking at commits that warrant a release note for LLDB 20, and I think your work on lldb-server's port map might fit the bill. If you want to do that yourself you can send a PR adding a bullet point to https://github.com/llvm/llvm-project/blob/main/llvm/docs/ReleaseNotes.md#changes-to-lldb, describing the changes. Or you can tell me a 1/2 sentence summary of it and I'll meld it into release notes style. I know that it has helped my work because I only have to open 2 ports now (1 platform and 1 gdb-server), but I'm not 100% sure of the detail. |
lldb-server now listens to a single port for gdbserver connections and provides it to the connection handler processes. This allows only 2 ports to be opened in the firewall. lldb-server now also works on Windows in the server mode. Thanks |
|
Thanks! - cfd7e02 |
Listen to gdbserver-port, accept the connection and run
lldb-server gdbserver --fdon all platforms.Parameters --min-gdbserver-port and --max-gdbserver-port are deprecated now.
This is the part 2 of #101283.
Fixes #97537.