llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Pavel Labath (labath) <details> <summary>Changes</summary> .. from the guts of GDBRemoteCommunication to ~top level. This is motivated by #<!-- -->131519 and by the fact that's impossible to guess whether the author of a symlink intended it to be a "convenience shortcut" -- meaning it should be resolved before looking for related files; or an "implementation detail" -- meaning the related files should be located near the symlink itself. This debate is particularly ridiculous when it comes to lldb-server running in platform mode, because it also functions as a debug server, so what we really just need to do is to pass /proc/self/exe in a platform-independent manner. Moving the location logic higher up achieves that as lldb-platform (on non-macos) can pass `HostInfo::GetProgramFileSpec`, while liblldb can use the existing complex logic (which only worked on liblldb anyway as lldb-platform doesn't have a lldb_private::Platform instance). Another benefit of this patch is a reduction in dependency from GDBRemoteCommunication to the rest of liblldb (achieved by avoiding the Platform dependency). --- Patch is 23.19 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/145706.diff 8 Files Affected: - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp (+1-76) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h (+4-8) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp (+11-11) - (modified) lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h (+3-1) - (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+58-4) - (modified) lldb/test/API/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.py (+46-23) - (modified) lldb/test/API/tools/lldb-dap/console/TestDAP_console.py (+2-2) - (modified) lldb/tools/lldb-server/lldb-platform.cpp (+35-8) ``````````diff diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp index bea2faff2330e..cea553e0ac9a2 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp @@ -11,7 +11,6 @@ #include "lldb/Host/Config.h" #include "lldb/Host/FileSystem.h" #include "lldb/Host/Host.h" -#include "lldb/Host/HostInfo.h" #include "lldb/Host/Pipe.h" #include "lldb/Host/ProcessLaunchInfo.h" #include "lldb/Host/Socket.h" @@ -33,14 +32,6 @@ #include <sys/stat.h> #include <variant> -#if defined(__APPLE__) -#define DEBUGSERVER_BASENAME "debugserver" -#elif defined(_WIN32) -#define DEBUGSERVER_BASENAME "lldb-server.exe" -#else -#define DEBUGSERVER_BASENAME "lldb-server" -#endif - #if HAVE_LIBCOMPRESSION #include <compression.h> #endif @@ -835,77 +826,11 @@ GDBRemoteCommunication::CheckForPacket(const uint8_t *src, size_t src_len, return GDBRemoteCommunication::PacketType::Invalid; } -FileSpec GDBRemoteCommunication::GetDebugserverPath(Platform *platform) { - Log *log = GetLog(GDBRLog::Process); - // If we locate debugserver, keep that located version around - static FileSpec g_debugserver_file_spec; - FileSpec debugserver_file_spec; - - Environment host_env = Host::GetEnvironment(); - - // Always check to see if we have an environment override for the path to the - // debugserver to use and use it if we do. - std::string env_debugserver_path = host_env.lookup("LLDB_DEBUGSERVER_PATH"); - if (!env_debugserver_path.empty()) { - debugserver_file_spec.SetFile(env_debugserver_path, - FileSpec::Style::native); - LLDB_LOGF(log, - "GDBRemoteCommunication::%s() gdb-remote stub exe path set " - "from environment variable: %s", - __FUNCTION__, env_debugserver_path.c_str()); - } else - debugserver_file_spec = g_debugserver_file_spec; - bool debugserver_exists = - FileSystem::Instance().Exists(debugserver_file_spec); - if (!debugserver_exists) { - // The debugserver binary is in the LLDB.framework/Resources directory. - debugserver_file_spec = HostInfo::GetSupportExeDir(); - if (debugserver_file_spec) { - debugserver_file_spec.AppendPathComponent(DEBUGSERVER_BASENAME); - debugserver_exists = FileSystem::Instance().Exists(debugserver_file_spec); - if (debugserver_exists) { - LLDB_LOGF(log, - "GDBRemoteCommunication::%s() found gdb-remote stub exe '%s'", - __FUNCTION__, debugserver_file_spec.GetPath().c_str()); - - g_debugserver_file_spec = debugserver_file_spec; - } else { - if (platform) - debugserver_file_spec = - platform->LocateExecutable(DEBUGSERVER_BASENAME); - else - debugserver_file_spec.Clear(); - if (debugserver_file_spec) { - // Platform::LocateExecutable() wouldn't return a path if it doesn't - // exist - debugserver_exists = true; - } else { - LLDB_LOGF(log, - "GDBRemoteCommunication::%s() could not find " - "gdb-remote stub exe '%s'", - __FUNCTION__, debugserver_file_spec.GetPath().c_str()); - } - // Don't cache the platform specific GDB server binary as it could - // change from platform to platform - g_debugserver_file_spec.Clear(); - } - } - } - return debugserver_file_spec; -} - Status GDBRemoteCommunication::StartDebugserverProcess( - std::variant<llvm::StringRef, shared_fd_t> comm, Platform *platform, + std::variant<llvm::StringRef, shared_fd_t> comm, ProcessLaunchInfo &launch_info, const Args *inferior_args) { Log *log = GetLog(GDBRLog::Process); - FileSpec debugserver_file_spec = GetDebugserverPath(platform); - if (!debugserver_file_spec) - return Status::FromErrorString("unable to locate " DEBUGSERVER_BASENAME); - - launch_info.SetExecutableFile(debugserver_file_spec, - /*add_exe_file_as_first_arg=*/true); - Args &debugserver_args = launch_info.GetArguments(); #if !defined(__APPLE__) diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h index 31f8edf715a3a..35bf5eb2e3f0d 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.h @@ -134,16 +134,12 @@ class GDBRemoteCommunication : public Communication { std::chrono::seconds GetPacketTimeout() const { return m_packet_timeout; } - // Get the debugserver path and check that it exist. - static FileSpec GetDebugserverPath(Platform *platform); - // Start a debugserver instance on the current host using the // supplied connection 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, const Args *inferior_args); + static Status + StartDebugserverProcess(std::variant<llvm::StringRef, shared_fd_t> comm, + 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 7506cf64def38..5876c3a9434a1 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp @@ -46,9 +46,10 @@ using namespace lldb_private; // GDBRemoteCommunicationServerPlatform constructor GDBRemoteCommunicationServerPlatform::GDBRemoteCommunicationServerPlatform( - const Socket::SocketProtocol socket_protocol, uint16_t gdbserver_port) - : GDBRemoteCommunicationServerCommon(), m_socket_protocol(socket_protocol), - m_gdbserver_port(gdbserver_port) { + FileSpec debugserver_path, const Socket::SocketProtocol socket_protocol, + uint16_t gdbserver_port) + : m_debugserver_path(std::move(debugserver_path)), + m_socket_protocol(socket_protocol), m_gdbserver_port(gdbserver_port) { RegisterMemberFunctionHandler( StringExtractorGDBRemote::eServerPacketType_qC, @@ -102,14 +103,15 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( debugserver_launch_info.SetLaunchInSeparateProcessGroup(false); debugserver_launch_info.SetMonitorProcessCallback( [](lldb::pid_t, int, int) {}); + if (!FileSystem::Instance().Exists(m_debugserver_path)) + return Status::FromErrorString("debugserver does not exist"); + debugserver_launch_info.SetExecutableFile(m_debugserver_path, + /*add_exe_file_as_first_arg=*/true); Status error; if (fd == SharedSocket::kInvalidFD) { if (m_socket_protocol == Socket::ProtocolTcp) { - // Just check that GDBServer exists. GDBServer must be launched after - // accepting the connection. - if (!GetDebugserverPath(nullptr)) - return Status::FromErrorString("unable to locate debugserver"); + // The server will be launched after accepting the connection. return Status(); } @@ -120,13 +122,11 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer( #endif socket_name = GetDomainSocketPath("gdbserver").GetPath(); url << socket_name; - error = StartDebugserverProcess(url.str(), nullptr, debugserver_launch_info, - &args); + error = StartDebugserverProcess(url.str(), 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); + error = StartDebugserverProcess(fd, debugserver_launch_info, &args); } if (error.Success()) { diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h index beee61ea8048e..bcfb6d3f3ed02 100644 --- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h +++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h @@ -26,7 +26,8 @@ class GDBRemoteCommunicationServerPlatform : public GDBRemoteCommunicationServerCommon { public: GDBRemoteCommunicationServerPlatform( - const Socket::SocketProtocol socket_protocol, uint16_t gdbserver_port); + FileSpec debugserver_path, const Socket::SocketProtocol socket_protocol, + uint16_t gdbserver_port); ~GDBRemoteCommunicationServerPlatform() override; @@ -40,6 +41,7 @@ class GDBRemoteCommunicationServerPlatform void SetPendingGdbServer(const std::string &socket_name); protected: + const FileSpec m_debugserver_path; const Socket::SocketProtocol m_socket_protocol; std::recursive_mutex m_spawned_pids_mutex; std::set<lldb::pid_t> m_spawned_pids; diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp index 3f9c4ddc60a25..a2c34ddfc252e 100644 --- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp +++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp @@ -34,6 +34,7 @@ #include "lldb/DataFormatters/FormatManager.h" #include "lldb/Host/ConnectionFileDescriptor.h" #include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" #include "lldb/Host/HostThread.h" #include "lldb/Host/PosixApi.h" #include "lldb/Host/PseudoTerminal.h" @@ -92,7 +93,14 @@ #include "llvm/Support/Threading.h" #include "llvm/Support/raw_ostream.h" +#if defined(__APPLE__) #define DEBUGSERVER_BASENAME "debugserver" +#elif defined(_WIN32) +#define DEBUGSERVER_BASENAME "lldb-server.exe" +#else +#define DEBUGSERVER_BASENAME "lldb-server" +#endif + using namespace lldb; using namespace lldb_private; using namespace lldb_private::process_gdb_remote; @@ -3448,6 +3456,51 @@ ProcessGDBRemote::EstablishConnectionIfNeeded(const ProcessInfo &process_info) { return error; } +static FileSpec GetDebugserverPath(Platform &platform) { + Log *log = GetLog(GDBRLog::Process); + // If we locate debugserver, keep that located version around + static FileSpec g_debugserver_file_spec; + FileSpec debugserver_file_spec; + + Environment host_env = Host::GetEnvironment(); + + // Always check to see if we have an environment override for the path to the + // debugserver to use and use it if we do. + std::string env_debugserver_path = host_env.lookup("LLDB_DEBUGSERVER_PATH"); + if (!env_debugserver_path.empty()) { + debugserver_file_spec.SetFile(env_debugserver_path, + FileSpec::Style::native); + LLDB_LOG(log, "gdb-remote stub exe path set from environment variable: {0}", + env_debugserver_path); + } else + debugserver_file_spec = g_debugserver_file_spec; + if (FileSystem::Instance().Exists(debugserver_file_spec)) + return debugserver_file_spec; + + // The debugserver binary is in the LLDB.framework/Resources directory. + debugserver_file_spec = HostInfo::GetSupportExeDir(); + if (debugserver_file_spec) { + debugserver_file_spec.AppendPathComponent(DEBUGSERVER_BASENAME); + if (FileSystem::Instance().Exists(debugserver_file_spec)) { + LLDB_LOG(log, "found gdb-remote stub exe '{0}'", debugserver_file_spec); + + g_debugserver_file_spec = debugserver_file_spec; + } else { + debugserver_file_spec = platform.LocateExecutable(DEBUGSERVER_BASENAME); + if (!debugserver_file_spec) { + // Platform::LocateExecutable() wouldn't return a path if it doesn't + // exist + LLDB_LOG(log, "could not find gdb-remote stub exe '{0}'", + debugserver_file_spec); + } + // Don't cache the platform specific GDB server binary as it could + // change from platform to platform + g_debugserver_file_spec.Clear(); + } + } + return debugserver_file_spec; +} + Status ProcessGDBRemote::LaunchAndConnectToDebugserver( const ProcessInfo &process_info) { using namespace std::placeholders; // For _1, _2, etc. @@ -3466,6 +3519,8 @@ Status ProcessGDBRemote::LaunchAndConnectToDebugserver( std::bind(MonitorDebugserverProcess, this_wp, _1, _2, _3)); debugserver_launch_info.SetUserID(process_info.GetUserID()); + FileSpec debugserver_path = GetDebugserverPath(*GetTarget().GetPlatform()); + #if defined(__APPLE__) // On macOS 11, we need to support x86_64 applications translated to // arm64. We check whether a binary is translated and spawn the correct @@ -3478,12 +3533,12 @@ Status ProcessGDBRemote::LaunchAndConnectToDebugserver( NULL, 0) == 0 && bufsize > 0) { if (processInfo.kp_proc.p_flag & P_TRANSLATED) { - FileSpec rosetta_debugserver( - "/Library/Apple/usr/libexec/oah/debugserver"); - debugserver_launch_info.SetExecutableFile(rosetta_debugserver, false); + debugserver_path = FileSpec("/Library/Apple/usr/libexec/oah/debugserver"); } } #endif + debugserver_launch_info.SetExecutableFile(debugserver_path, + /*add_exe_file_as_first_arg=*/true); llvm::Expected<Socket::Pair> socket_pair = Socket::CreatePair(); if (!socket_pair) @@ -3495,7 +3550,6 @@ Status ProcessGDBRemote::LaunchAndConnectToDebugserver( return error; error = m_gdb_comm.StartDebugserverProcess(shared_socket.GetSendableFD(), - GetTarget().GetPlatform().get(), debugserver_launch_info, nullptr); if (error.Fail()) { diff --git a/lldb/test/API/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.py b/lldb/test/API/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.py index c365bc993e338..45d855f5c1938 100644 --- a/lldb/test/API/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.py +++ b/lldb/test/API/commands/platform/launchgdbserver/TestPlatformLaunchGDBServer.py @@ -1,8 +1,3 @@ -""" Check that errors while handling qLaunchGDBServer are reported to the user. - Though this isn't a platform command in itself, the best way to test it is - from Python because we can juggle multiple processes more easily. -""" - import os import socket import shutil @@ -15,14 +10,7 @@ class TestPlatformProcessLaunchGDBServer(TestBase): NO_DEBUG_INFO_TESTCASE = True - @skipIfRemote - # Windows cannot delete the executable while it is running. - # On Darwin we may be using debugserver. - @skipUnlessPlatform(["linux"]) - @add_test_categories(["lldb-server"]) - def test_platform_process_launch_gdb_server(self): - self.build() - + def _launch_and_connect(self, exe): hostname = socket.getaddrinfo("localhost", 0, proto=socket.IPPROTO_TCP)[0][4][0] listen_url = "[%s]:0" % hostname @@ -33,16 +21,9 @@ def test_platform_process_launch_gdb_server(self): listen_url, "--socket-file", port_file, - "--", - self.getBuildArtifact("a.out"), - "foo", ] - # Run lldb-server from a new location. - new_lldb_server = self.getBuildArtifact("lldb-server") - shutil.copy(lldbgdbserverutils.get_lldb_server_exe(), new_lldb_server) - - self.spawnSubprocess(new_lldb_server, commandline_args) + self.spawnSubprocess(exe, commandline_args) socket_id = lldbutil.wait_for_file_on_target(self, port_file) new_platform = lldb.SBPlatform("remote-" + self.getPlatform()) @@ -51,10 +32,52 @@ def test_platform_process_launch_gdb_server(self): connect_url = "connect://[%s]:%s" % (hostname, socket_id) self.runCmd("platform connect %s" % connect_url) - # First connect to lldb-server which spawn a process to handle the connection. - # Then remove our new lldb-server so that when it tries to invoke itself as a + wd = self.getBuildArtifact("wd") + os.mkdir(wd) + new_platform.SetWorkingDirectory(wd) + + @skipIfRemote + # Windows cannot delete the executable while it is running. + # On Darwin we may be using debugserver. + @skipUnlessPlatform(["linux"]) + @add_test_categories(["lldb-server"]) + def test_launch_error(self): + """ + Check that errors while handling qLaunchGDBServer are reported to the + user. Though this isn't a platform command in itself, the best way to + test it is from Python because we can juggle multiple processes more + easily. + """ + + self.build() + + # Run lldb-server from a new location. + new_lldb_server = self.getBuildArtifact("lldb-server") + shutil.copy(lldbgdbserverutils.get_lldb_server_exe(), new_lldb_server) + self._launch_and_connect(new_lldb_server) + + # Now, remove our new lldb-server so that when it tries to invoke itself as a # gdbserver, it fails. os.remove(new_lldb_server) self.runCmd("target create {}".format(self.getBuildArtifact("a.out"))) self.expect("run", substrs=["unable to launch a GDB server on"], error=True) + + @skipIfRemote + @skipIfDarwin # Uses debugserver for debugging + @add_test_categories(["lldb-server"]) + def test_launch_with_unusual_process_name(self): + """ + Test that lldb-server can launch a debug session when running under an + unusual name (or under a symlink which resolves to an unusal name). + """ + + self.build() + + # Run lldb-server from a new location. + new_lldb_server = self.getBuildArtifact("obfuscated-server") + shutil.copy(lldbgdbserverutils.get_lldb_server_exe(), new_lldb_server) + self._launch_and_connect(new_lldb_server) + + self.runCmd("target create {}".format(self.getBuildArtifact("a.out"))) + self.expect("run", substrs=["exited with status = 0"]) diff --git a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py index 7b4d1adbb2071..811843dfdf7af 100644 --- a/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py +++ b/lldb/test/API/tools/lldb-dap/console/TestDAP_console.py @@ -144,9 +144,9 @@ def test_exit_status_message_sigterm(self): ) # Verify the exit status message is printed. - self.assertIn( - "exited with status = -1 (0xffffffff) debugserver died with signal SIGTERM", + self.assertRegex( console_output, + ".*exited with status = -1 .* died with signal SIGTERM.*", "Exit status does not contain message 'exited with status'", ) diff --git a/lldb/tools/lldb-server/lldb-platform.cpp b/lldb/tools/lldb-server/lldb-platform.cpp index 3c79dc001f65e..0bd928507ba89 100644 --- a/lldb/tools/lldb-server/lldb-platform.cpp +++ b/lldb/tools/lldb-server/lldb-platform.cpp @@ -30,6 +30,7 @@ #include "Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.h" #include "Plugins/Process/gdb-remote/ProcessGDBRemoteLog.h" #include "lldb/Host/ConnectionFileDescriptor.h" +#include "lldb/Host/FileSystem.h" #include "lldb/Host/HostGetOpt.h" #include "lldb/Host/HostInfo.h" #include "lldb/Host/MainLoop.h" @@ -192,14 +193,15 @@ static Status ListenGdbConnectionsIfNeeded( } static llvm::Expected<std::vector<MainLoopBase::ReadHandleUP>> -AcceptGdbConnectionsIfNeeded(const Socket::SocketProtocol protocol, +AcceptGdbConnectionsIfNeeded(const FileSpec &debugserver_path, + const Socket::SocketProtocol protocol, std::unique_ptr<TCPSocket> &gdb_sock, MainLoop &main_loop, const uint16_t gdbserver_port, const lldb_private::Args &args) { if (protocol != Socket::ProtocolTcp) return std::vector<MainLoopBase::ReadHandleUP>(); - return gdb_sock->Accept(main_loop, [gdbserve... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/145706 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
