llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) <details> <summary>Changes</summary> Removed fork(). Used threads and the common thread-safe port map for all platform connections. Updated lldb::FileSystem to use llvm::vfs::createPhysicalFileSystem() with an own virtual working directory per thread. This patch depends on #<!-- -->100659, #<!-- -->100666. This patch fixes #<!-- -->97537, #<!-- -->90923, #<!-- -->56346. lldb-server has been tested on Windows with 50 connections and 100 processes launched simultaneously. Tested also the cross build with Linux x86_64 host and Linux Aarch64 target. --- Patch is 33.09 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100670.diff 8 Files Affected: - (modified) lldb/include/lldb/Host/FileSystem.h (+7) - (modified) lldb/source/Host/common/FileSystem.cpp (+8) - (modified) lldb/source/Host/posix/PipePosix.cpp (+12) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp (+13-2) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp (+217-92) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h (+19-12) - (modified) lldb/tools/lldb-server/LLDBServerUtilities.cpp (+2) - (modified) lldb/tools/lldb-server/lldb-platform.cpp (+22-77) ``````````diff diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h index 640f3846e448c..5e25414a894d3 100644 --- a/lldb/include/lldb/Host/FileSystem.h +++ b/lldb/include/lldb/Host/FileSystem.h @@ -47,6 +47,12 @@ class FileSystem { static FileSystem &Instance(); + static void InitializePerThread() { + lldbassert(!InstancePerThread() && "Already initialized."); + InstancePerThread().emplace(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem>( + llvm::vfs::createPhysicalFileSystem().release())); + } + template <class... T> static void Initialize(T &&...t) { lldbassert(!InstanceImpl() && "Already initialized."); InstanceImpl().emplace(std::forward<T>(t)...); @@ -206,6 +212,7 @@ class FileSystem { private: static std::optional<FileSystem> &InstanceImpl(); + static std::optional<FileSystem> &InstancePerThread(); llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> m_fs; std::unique_ptr<TildeExpressionResolver> m_tilde_resolver; std::string m_home_directory; diff --git a/lldb/source/Host/common/FileSystem.cpp b/lldb/source/Host/common/FileSystem.cpp index 5153a0a9ec513..cb76086616d6b 100644 --- a/lldb/source/Host/common/FileSystem.cpp +++ b/lldb/source/Host/common/FileSystem.cpp @@ -49,7 +49,15 @@ void FileSystem::Terminate() { InstanceImpl().reset(); } +std::optional<FileSystem> &FileSystem::InstancePerThread() { + static thread_local std::optional<FileSystem> t_fs; + return t_fs; +} + std::optional<FileSystem> &FileSystem::InstanceImpl() { + std::optional<FileSystem> &fs = InstancePerThread(); + if (fs) + return fs; static std::optional<FileSystem> g_fs; return g_fs; } diff --git a/lldb/source/Host/posix/PipePosix.cpp b/lldb/source/Host/posix/PipePosix.cpp index f35c348990df6..1aa02efe86610 100644 --- a/lldb/source/Host/posix/PipePosix.cpp +++ b/lldb/source/Host/posix/PipePosix.cpp @@ -324,6 +324,18 @@ Status PipePosix::ReadWithTimeout(void *buf, size_t size, bytes_read += result; if (bytes_read == size || result == 0) break; + + // This is the workaround for the following bug in Linux multithreading + // select() https://bugzilla.kernel.org/show_bug.cgi?id=546 + // ReadWithTimeout() with a non-zero timeout is used only to + // read the port number from the gdbserver pipe + // in GDBRemoteCommunication::StartDebugserverProcess(). + // The port number may be "1024\0".."65535\0". + if (timeout.count() > 0 && size == 6 && bytes_read == 5 && + static_cast<char *>(buf)[4] == '\0') { + break; + } + } else if (errno == EINTR) { continue; } else { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp index f9d37490e16ae..cef836e001adf 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp @@ -646,7 +646,9 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_Size( packet.GetHexByteString(path); if (!path.empty()) { uint64_t Size; - if (llvm::sys::fs::file_size(path, Size)) + FileSpec file_spec(path); + FileSystem::Instance().Resolve(file_spec); + if (llvm::sys::fs::file_size(file_spec.GetPath(), Size)) return SendErrorResponse(5); StreamString response; response.PutChar('F'); @@ -725,7 +727,9 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_unlink( packet.SetFilePos(::strlen("vFile:unlink:")); std::string path; packet.GetHexByteString(path); - Status error(llvm::sys::fs::remove(path)); + FileSpec file_spec(path); + FileSystem::Instance().Resolve(file_spec); + Status error(llvm::sys::fs::remove(file_spec.GetPath())); StreamString response; response.Printf("F%x,%x", error.GetError(), error.GetError()); return SendPacketNoLock(response.GetString()); @@ -744,6 +748,13 @@ GDBRemoteCommunicationServerCommon::Handle_qPlatform_shell( // uint32_t timeout = packet.GetHexMaxU32(false, 32); if (packet.GetChar() == ',') packet.GetHexByteString(working_dir); + else { + auto cwd = FileSystem::Instance() + .GetVirtualFileSystem() + ->getCurrentWorkingDirectory(); + if (cwd) + working_dir = *cwd; + } int status, signo; std::string output; FileSpec working_spec(working_dir); diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp index 65f1cc12ba307..6e3b7b4a351e0 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp @@ -18,13 +18,13 @@ #include <sstream> #include <thread> -#include "llvm/Support/FileSystem.h" #include "llvm/Support/JSON.h" #include "llvm/Support/Threading.h" #include "lldb/Host/Config.h" #include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/FileAction.h" +#include "lldb/Host/FileSystem.h" #include "lldb/Host/Host.h" #include "lldb/Host/HostInfo.h" #include "lldb/Interpreter/CommandCompletions.h" @@ -44,8 +44,17 @@ using namespace lldb; using namespace lldb_private::process_gdb_remote; using namespace lldb_private; +// Copy assignment operator to avoid copying m_mutex +GDBRemoteCommunicationServerPlatform::PortMap & +GDBRemoteCommunicationServerPlatform::PortMap::operator=( + const GDBRemoteCommunicationServerPlatform::PortMap &o) { + m_port_map = std::move(o.m_port_map); + return *this; +} + GDBRemoteCommunicationServerPlatform::PortMap::PortMap(uint16_t min_port, - uint16_t max_port) { + uint16_t max_port) + : m_mutex() { assert(min_port); for (; min_port < max_port; ++min_port) m_port_map[min_port] = LLDB_INVALID_PROCESS_ID; @@ -54,11 +63,13 @@ GDBRemoteCommunicationServerPlatform::PortMap::PortMap(uint16_t min_port, void GDBRemoteCommunicationServerPlatform::PortMap::AllowPort(uint16_t port) { assert(port); // Do not modify existing mappings + std::lock_guard<std::mutex> guard(m_mutex); m_port_map.insert({port, LLDB_INVALID_PROCESS_ID}); } llvm::Expected<uint16_t> GDBRemoteCommunicationServerPlatform::PortMap::GetNextAvailablePort() { + std::lock_guard<std::mutex> guard(m_mutex); if (m_port_map.empty()) return 0; // Bind to port zero and get a port, we didn't have any // limitations @@ -75,6 +86,7 @@ GDBRemoteCommunicationServerPlatform::PortMap::GetNextAvailablePort() { bool GDBRemoteCommunicationServerPlatform::PortMap::AssociatePortWithProcess( uint16_t port, lldb::pid_t pid) { + std::lock_guard<std::mutex> guard(m_mutex); auto pos = m_port_map.find(port); if (pos != m_port_map.end()) { pos->second = pid; @@ -84,6 +96,7 @@ bool GDBRemoteCommunicationServerPlatform::PortMap::AssociatePortWithProcess( } bool GDBRemoteCommunicationServerPlatform::PortMap::FreePort(uint16_t port) { + std::lock_guard<std::mutex> guard(m_mutex); 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; @@ -94,6 +107,7 @@ bool GDBRemoteCommunicationServerPlatform::PortMap::FreePort(uint16_t port) { bool GDBRemoteCommunicationServerPlatform::PortMap::FreePortForProcess( lldb::pid_t pid) { + std::lock_guard<std::mutex> guard(m_mutex); if (!m_port_map.empty()) { for (auto &pair : m_port_map) { if (pair.second == pid) { @@ -106,15 +120,22 @@ bool GDBRemoteCommunicationServerPlatform::PortMap::FreePortForProcess( } bool GDBRemoteCommunicationServerPlatform::PortMap::empty() const { + std::lock_guard<std::mutex> guard(m_mutex); return m_port_map.empty(); } +GDBRemoteCommunicationServerPlatform::PortMap + GDBRemoteCommunicationServerPlatform::g_port_map; +std::set<lldb::pid_t> GDBRemoteCommunicationServerPlatform::g_spawned_pids; +std::mutex GDBRemoteCommunicationServerPlatform::g_spawned_pids_mutex; + // 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) { + const Socket::SocketProtocol socket_protocol, const char *socket_scheme, + const lldb_private::Args &args, uint16_t port_offset) + : GDBRemoteCommunicationServerCommon(), m_socket_protocol(socket_protocol), + m_socket_scheme(socket_scheme), m_inferior_arguments(args), + m_port_offset(port_offset) { m_pending_gdb_server.pid = LLDB_INVALID_PROCESS_ID; m_pending_gdb_server.port = 0; @@ -159,11 +180,72 @@ GDBRemoteCommunicationServerPlatform::GDBRemoteCommunicationServerPlatform( GDBRemoteCommunicationServerPlatform::~GDBRemoteCommunicationServerPlatform() = default; +lldb::thread_result_t GDBRemoteCommunicationServerPlatform::ThreadProc() { + // We need a virtual working directory per thread. + FileSystem::InitializePerThread(); + + Log *log = GetLog(LLDBLog::Platform); + + if (IsConnected()) { + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "Thread started...", + __FUNCTION__); + + if (m_inferior_arguments.GetArgumentCount() > 0) { + lldb::pid_t pid = LLDB_INVALID_PROCESS_ID; + std::optional<uint16_t> port; + std::string socket_name; + Status error = LaunchGDBServer(m_inferior_arguments, + "", // hostname + pid, port, socket_name); + if (error.Success()) + SetPendingGdbServer(pid, *port, socket_name); + } + + bool interrupt = false; + bool done = false; + Status error; + while (!interrupt && !done) { + if (GetPacketAndSendResponse(std::nullopt, error, interrupt, done) != + GDBRemoteCommunication::PacketResult::Success) + break; + } + + if (error.Fail()) { + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "GetPacketAndSendResponse: %s", + __FUNCTION__, error.AsCString()); + } + } + + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "Disconnected. Killing child processes...", + __FUNCTION__); + for (lldb::pid_t pid : m_spawned_pids) + KillSpawnedProcess(pid); + + // Do do not wait for child processes. See comments in + // DebugserverProcessReaped() for details. + + FileSystem::Terminate(); + + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "Thread exited.", + __FUNCTION__); + + delete this; + return {}; +} + 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(); + llvm::Expected<uint16_t> available_port = g_port_map.GetNextAvailablePort(); if (available_port) port = *available_port; else @@ -181,23 +263,25 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( if (hostname.empty()) hostname = "127.0.0.1"; - Log *log = GetLog(LLDBLog::Platform); - LLDB_LOGF(log, "Launching debugserver with: %s:%u...", hostname.c_str(), - *port); + auto cwd = FileSystem::Instance() + .GetVirtualFileSystem() + ->getCurrentWorkingDirectory(); + if (cwd) + debugserver_launch_info.SetWorkingDirectory(FileSpec(*cwd)); // 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( - std::bind(&GDBRemoteCommunicationServerPlatform::DebugserverProcessReaped, - this, std::placeholders::_1)); + &GDBRemoteCommunicationServerPlatform::DebugserverProcessReaped); std::ostringstream url; // debugserver does not accept the URL scheme prefix. #if !defined(__APPLE__) url << m_socket_scheme << "://"; #endif - uint16_t *port_ptr = &*port; + uint16_t child_port = *port; + uint16_t *port_ptr = &child_port; if (m_socket_protocol == Socket::ProtocolTcp) { std::string platform_uri = GetConnection()->GetURI(); std::optional<URI> parsed_uri = URI::Parse(platform_uri); @@ -208,19 +292,44 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( port_ptr = nullptr; } + Log *log = GetLog(LLDBLog::Platform); + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "Host %s launching debugserver with: %s...", + __FUNCTION__, hostname.c_str(), url.str().c_str()); + Status error = StartDebugserverProcess( url.str().c_str(), nullptr, debugserver_launch_info, port_ptr, &args, -1); pid = debugserver_launch_info.GetProcessID(); + + if (error.Success()) { + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "debugserver launched successfully as pid %" PRIu64, + __FUNCTION__, pid); + } else { + LLDB_LOGF(log, + "GDBRemoteCommunicationServerPlatform::%s() " + "debugserver launch failed: %s", + __FUNCTION__, error.AsCString()); + } + + // TODO: Be sure gdbserver uses the requested port. + // assert(!port_ptr || *port == 0 || *port == child_port) + // Use only the original *port returned by GetNextAvailablePort() + // for AssociatePortWithProcess() or FreePort() below. + if (pid != LLDB_INVALID_PROCESS_ID) { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - m_spawned_pids.insert(pid); + AddSpawnedProcess(pid); if (*port > 0) - m_port_map.AssociatePortWithProcess(*port, pid); + g_port_map.AssociatePortWithProcess(*port, pid); } else { if (*port > 0) - m_port_map.FreePort(*port); + g_port_map.FreePort(*port); } + if (port_ptr) + *port = child_port; return error; } @@ -230,10 +339,6 @@ GDBRemoteCommunicationServerPlatform::Handle_qLaunchGDBServer( // Spawn a local debugserver as a platform so we can then attach or launch a // process... - Log *log = GetLog(LLDBLog::Platform); - LLDB_LOGF(log, "GDBRemoteCommunicationServerPlatform::%s() called", - __FUNCTION__); - ConnectionFileDescriptor file_conn; std::string hostname; packet.SetFilePos(::strlen("qLaunchGDBServer;")); @@ -255,18 +360,9 @@ GDBRemoteCommunicationServerPlatform::Handle_qLaunchGDBServer( Status error = LaunchGDBServer(Args(), hostname, debugserver_pid, port, socket_name); if (error.Fail()) { - LLDB_LOGF(log, - "GDBRemoteCommunicationServerPlatform::%s() debugserver " - "launch failed: %s", - __FUNCTION__, error.AsCString()); return SendErrorResponse(9); } - LLDB_LOGF(log, - "GDBRemoteCommunicationServerPlatform::%s() debugserver " - "launched successfully as pid %" PRIu64, - __FUNCTION__, debugserver_pid); - StreamGDBRemote response; assert(port); response.Printf("pid:%" PRIu64 ";port:%u;", debugserver_pid, @@ -317,28 +413,45 @@ GDBRemoteCommunicationServerPlatform::Handle_qKillSpawnedProcess( lldb::pid_t pid = packet.GetU64(LLDB_INVALID_PROCESS_ID); + if (SpawnedProcessFinished(pid)) + m_spawned_pids.erase(pid); + // verify that we know anything about this pid. Scope for locker - { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - if (m_spawned_pids.find(pid) == m_spawned_pids.end()) { - // not a pid we know about - return SendErrorResponse(10); - } + if ((m_spawned_pids.find(pid) == m_spawned_pids.end())) { + // not a pid we know about + return SendErrorResponse(10); // ECHILD } // go ahead and attempt to kill the spawned process - if (KillSpawnedProcess(pid)) + if (KillSpawnedProcess(pid)) { + m_spawned_pids.erase(pid); return SendOKResponse(); - else - return SendErrorResponse(11); + } else + return SendErrorResponse(11); // EDEADLK +} + +void GDBRemoteCommunicationServerPlatform::AddSpawnedProcess(lldb::pid_t pid) { + std::lock_guard<std::mutex> guard(g_spawned_pids_mutex); + + // If MonitorChildProcessThreadFunction() failed hope the system will not + // reuse pid of zombie processes. + // assert(g_spawned_pids.find(pid) == g_spawned_pids.end()); + + g_spawned_pids.insert(pid); + m_spawned_pids.insert(pid); +} + +bool GDBRemoteCommunicationServerPlatform::SpawnedProcessFinished( + lldb::pid_t pid) { + std::lock_guard<std::mutex> guard(g_spawned_pids_mutex); + return (g_spawned_pids.find(pid) == g_spawned_pids.end()); } bool GDBRemoteCommunicationServerPlatform::KillSpawnedProcess(lldb::pid_t pid) { // make sure we know about this process - { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - if (m_spawned_pids.find(pid) == m_spawned_pids.end()) - return false; + if (SpawnedProcessFinished(pid)) { + // it seems the process has been finished recently + return true; } // first try a SIGTERM (standard kill) @@ -346,46 +459,30 @@ bool GDBRemoteCommunicationServerPlatform::KillSpawnedProcess(lldb::pid_t pid) { // check if that worked for (size_t i = 0; i < 10; ++i) { - { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - if (m_spawned_pids.find(pid) == m_spawned_pids.end()) { - // it is now killed - return true; - } + if (SpawnedProcessFinished(pid)) { + // it is now killed + return true; } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } - { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - if (m_spawned_pids.find(pid) == m_spawned_pids.end()) - return true; - } + if (SpawnedProcessFinished(pid)) + return true; // the launched process still lives. Now try killing it again, this time // with an unblockable signal. Host::Kill(pid, SIGKILL); for (size_t i = 0; i < 10; ++i) { - { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - if (m_spawned_pids.find(pid) == m_spawned_pids.end()) { - // it is now killed - return true; - } + if (SpawnedProcessFinished(pid)) { + // it is now killed + return true; } std::this_thread::sleep_for(std::chrono::milliseconds(10)); } // check one more time after the final sleep - { - std::lock_guard<std::recursive_mutex> guard(m_spawned_pids_mutex); - if (m_spawned_pids.find(pid) == m_spawned_pids.end()) - return true; - } - - // no luck - the process still lives - return false; + return SpawnedProcessFinished(pid); } GDBRemoteCommunication::PacketResult @@ -442,12 +539,14 @@ GDBRemoteCommunication::PacketResult GDBRemoteCommunicationServerPlatform::Handle_qGetWorkingDir( StringExtractorGDBRemote &packet) { - llvm::SmallString<64> cwd; - if (std::error_code ec = llvm::sys::fs::current_path(cwd)) - return SendErrorResponse(ec.value()); + auto cwd = FileSystem::Instance() + .GetVirtualFileSystem() + ->getCurrentWorkingDirectory(); + if (!cwd) + return SendErrorResponse(cwd.getError()); StreamString response; - response.PutBytesAsRawHex8(cwd.data(), cwd.size()); + response.PutBytesAsRawHex8(cwd->data(), cwd->size()); return SendPacketNoLock(response.GetString()); } @@ -458,7 +557,9 @@ GDBRemoteCommunicationServerPlatform::Handle_QSetWorkingDir( std::string path; packet.GetHexByteString(path); - if (std::error_code ec = llvm::sys::fs::set_current_path(path)) + if (std::error_code ec = FileSystem::Instance() + .GetVirtualFileSystem() + ... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/100670 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
