Author: Jonas Devlieghere Date: 2026-08-13T13:56:17-07:00 New Revision: 8728bc61b4e39eed747b227e8b9c3b5d7a0ce2cd
URL: https://github.com/llvm/llvm-project/commit/8728bc61b4e39eed747b227e8b9c3b5d7a0ce2cd DIFF: https://github.com/llvm/llvm-project/commit/8728bc61b4e39eed747b227e8b9c3b5d7a0ce2cd.diff LOG: [lldb] Add a unified entry point for locating a binary and its symbols (#215391) The three-plugin composition that finds a binary and its symbol file is open coded in several places. Give it one home, as a pure function of a module spec, so that a caller holding several binaries can search for all of them before creating any. Nothing on this path takes a lock, where ModuleList::GetSharedModule holds the shared module list's lock across the plugin search. That is what will make searching for several binaries at once worth doing. A miss that nothing could explain gets its own error type rather than an error code, because a Status carrying an errno converts to the same llvm::ECError, and a caller composing its own message for a plain miss must not swallow a failure to reach a symbol server. DynamicLoader's search no longer runs twice for a binary that is not already known, and a module created from a located binary is now registered in the shared module list, so a second Target asking for the same binary reuses it. Assisted-by: Claude Added: lldb/unittests/Symbol/SymbolLocatorTest.cpp Modified: lldb/include/lldb/Symbol/SymbolLocator.h lldb/source/Core/DynamicLoader.cpp lldb/source/Symbol/SymbolLocator.cpp lldb/unittests/Symbol/CMakeLists.txt Removed: ################################################################################ diff --git a/lldb/include/lldb/Symbol/SymbolLocator.h b/lldb/include/lldb/Symbol/SymbolLocator.h index 400b4adfd73c4..b1e01710cb1c3 100644 --- a/lldb/include/lldb/Symbol/SymbolLocator.h +++ b/lldb/include/lldb/Symbol/SymbolLocator.h @@ -9,15 +9,75 @@ #ifndef LLDB_SYMBOL_SYMBOLLOCATOR_H #define LLDB_SYMBOL_SYMBOLLOCATOR_H +#include "lldb/Core/ModuleSpec.h" #include "lldb/Core/PluginInterface.h" +#include "lldb/Target/Statistics.h" +#include "lldb/Utility/Status.h" #include "lldb/Utility/UUID.h" +#include "llvm/Support/Error.h" + +#include <optional> +#include <system_error> + namespace lldb_private { class SymbolLocator : public PluginInterface { public: SymbolLocator() = default; + /// A binary was not found and nothing could say why. Distinct from every + /// other error so that a caller composing its own message for a plain miss + /// cannot mistake a real failure for one. + class NotFound : public llvm::ErrorInfo<NotFound> { + public: + static char ID; + + void log(llvm::raw_ostream &os) const override; + std::error_code convertToErrorCode() const override; + }; + + /// One binary to search for. + struct Request { + /// What to look for. + ModuleSpec module_spec; + + /// Allow contacting an external symbol server when the local searches come + /// up empty. + bool external_lookup = false; + }; + + /// What a search found. + struct Result { + /// The binary, and its symbol file if there is one to be had. + ModuleSpec module_spec; + + /// What an external symbol server had to say about the symbols, even + /// though the binary itself was found. Recorded rather than reported, so + /// that it reaches the user in the caller's order, and so has to be + /// consumed even by a caller that does not report it. + std::optional<llvm::Error> symbol_error; + + /// Where the time went, to be merged into the Module once it exists. + StatisticsMap statistics; + }; + + /// Find a binary and, if possible, its symbols. + /// + /// Mutates no Target and no Process, which is what lets a caller holding + /// several binaries search for all of them before installing any. + /// + /// \return + /// Where the binary is, or an error if it could not be found at all. + /// Finding the binary but not its symbols is a success, with + /// Result::symbol_error carrying whatever a symbol server had to say + /// about them. + /// + /// A miss carries an external symbol server's explanation if it gave + /// one, and is a NotFound when nothing could say why. + static llvm::Expected<Result> Locate(const Request &request, + const FileSpecList &search_paths); + /// Locate the symbol file for the given UUID on a background thread. This /// function returns immediately. Under the hood it uses the debugger's /// thread pool to call DownloadObjectAndSymbolFile. If a symbol file is diff --git a/lldb/source/Core/DynamicLoader.cpp b/lldb/source/Core/DynamicLoader.cpp index 1f85b655ba116..4f9023976b95a 100644 --- a/lldb/source/Core/DynamicLoader.cpp +++ b/lldb/source/Core/DynamicLoader.cpp @@ -16,6 +16,7 @@ #include "lldb/Core/Progress.h" #include "lldb/Core/Section.h" #include "lldb/Symbol/ObjectFile.h" +#include "lldb/Symbol/SymbolLocator.h" #include "lldb/Target/MemoryRegionInfo.h" #include "lldb/Target/Platform.h" #include "lldb/Target/Process.h" @@ -258,47 +259,55 @@ static void SearchForBinary(Target &target, DynamicLoader::BinarySpec &bin_spec, if (FileSystem::Instance().Exists(name_filespec)) module_spec.GetFileSpec() = name_filespec; - // Has lldb already seen a module with this UUID? - // Or have external lookup enabled in DebugSymbols on macOS. - Status error = ModuleList::GetSharedModule(module_spec, bin_spec.module_sp, - nullptr, nullptr); - - // Can lldb's symbol/executable location schemes find an executable and - // symbol file. - if (!bin_spec.module_sp) { - StatisticsMap symbol_locator_map; - module_spec.GetSymbolFileSpec() = PluginManager::LocateExecutableSymbolFile( - module_spec, search_paths, symbol_locator_map); - ModuleSpec objfile_module_spec = PluginManager::LocateExecutableObjectFile( - module_spec, symbol_locator_map); - module_spec.GetFileSpec() = objfile_module_spec.GetFileSpec(); - if (FileSystem::Instance().Exists(module_spec.GetFileSpec()) && - FileSystem::Instance().Exists(module_spec.GetSymbolFileSpec())) { - bin_spec.module_sp = std::make_shared<Module>(module_spec); - } - - if (bin_spec.module_sp) { - bin_spec.module_sp->GetSymbolLocatorStatistics().merge( - symbol_locator_map); - } - } - - // If we haven't found a binary, or we don't have a SymbolFile, see - // if there is an external search tool that can find it. - if (!bin_spec.module_sp || !bin_spec.module_sp->GetSymbolFileFileSpec()) { - PluginManager::DownloadObjectAndSymbolFile(module_spec, error, - bin_spec.force_symbol_search); - if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) { - bin_spec.module_sp = std::make_shared<Module>(module_spec); - } else if (bin_spec.force_symbol_search && error.Fail()) { - bin_spec.error = std::move(error); - } + // Has lldb already seen a module with this UUID? A module whose symbols are + // already in hand is the answer, and searching would only find them again. + // Without them the search still has something to add. + ModuleList::GetSharedModule(module_spec, bin_spec.module_sp, nullptr, nullptr, + /*invoke_locate_callback=*/true, + /*invoke_symbol_locators=*/false); + if (bin_spec.module_sp && bin_spec.module_sp->GetSymbolFileFileSpec()) + return; + + // Search for the binary and its symbols. + SymbolLocator::Request request; + request.module_spec = module_spec; + request.external_lookup = bin_spec.force_symbol_search; + + llvm::Expected<SymbolLocator::Result> located = + SymbolLocator::Locate(request, search_paths); + if (!located) { + // This function's caller names the binary it could not find, so a plain + // miss needs nothing added to it. An explanation from a symbol server does. + llvm::Error error = located.takeError(); + if (error.isA<SymbolLocator::NotFound>()) + llvm::consumeError(std::move(error)); + else + bin_spec.error = Status::FromError(std::move(error)); + return; } - // If we only found the executable, create a Module based on that. - if (!bin_spec.module_sp && - FileSystem::Instance().Exists(module_spec.GetFileSpec())) - bin_spec.module_sp = std::make_shared<Module>(module_spec); + // A binary was found. Its symbols are another matter, and the caller reports + // that in its own order. + if (located->symbol_error) + bin_spec.error = Status::FromError(std::move(*located->symbol_error)); + + // Create a module for what was found, sharing it with any other Target that + // asks for the same binary. The module is not registered with this Target + // until LoadBinaryInTarget. The locators have run, so don't run them again. + ModuleSP located_module_sp; + ModuleList::GetSharedModule(located->module_spec, located_module_sp, nullptr, + nullptr, /*invoke_locate_callback=*/false, + /*invoke_symbol_locators=*/false); + + // A located binary always yields a module, whatever ObjectFile makes of the + // file, because the caller has nowhere else to record what the search found. + if (!located_module_sp) + located_module_sp = std::make_shared<Module>(located->module_spec); + + // Published only now, so that a search that came up empty leaves whatever the + // shared module list had in hand. + bin_spec.module_sp = std::move(located_module_sp); + bin_spec.module_sp->GetSymbolLocatorStatistics().merge(located->statistics); } static void FindBinaryUUIDInMemory(Process *process, diff --git a/lldb/source/Symbol/SymbolLocator.cpp b/lldb/source/Symbol/SymbolLocator.cpp index 93a5bc428b614..3b3e531e14521 100644 --- a/lldb/source/Symbol/SymbolLocator.cpp +++ b/lldb/source/Symbol/SymbolLocator.cpp @@ -10,6 +10,7 @@ #include "lldb/Core/Debugger.h" #include "lldb/Core/PluginManager.h" +#include "lldb/Host/FileSystem.h" #include "lldb/Host/Host.h" #include "llvm/ADT/SmallSet.h" @@ -18,6 +19,54 @@ using namespace lldb; using namespace lldb_private; +char SymbolLocator::NotFound::ID = 0; + +void SymbolLocator::NotFound::log(llvm::raw_ostream &os) const { + os << "binary not found"; +} + +std::error_code SymbolLocator::NotFound::convertToErrorCode() const { + return std::make_error_code(std::errc::no_such_file_or_directory); +} + +llvm::Expected<SymbolLocator::Result> +SymbolLocator::Locate(const Request &request, + const FileSpecList &search_paths) { + FileSystem &fs = FileSystem::Instance(); + Result result; + ModuleSpec &module_spec = result.module_spec; + module_spec = request.module_spec; + + // Can lldb's symbol and executable location schemes find them locally? + module_spec.GetSymbolFileSpec() = PluginManager::LocateExecutableSymbolFile( + module_spec, search_paths, result.statistics); + module_spec.GetFileSpec() = + PluginManager::LocateExecutableObjectFile(module_spec, result.statistics) + .GetFileSpec(); + + // If we're still missing either file, see if an external symbol server can + // provide it. + if (!fs.Exists(module_spec.GetFileSpec()) || + !fs.Exists(module_spec.GetSymbolFileSpec())) { + Status error; + PluginManager::DownloadObjectAndSymbolFile(module_spec, error, + request.external_lookup); + if (error.Fail() && request.external_lookup) + result.symbol_error.emplace(error.takeError()); + } + + // Not finding the binary is the one hard failure. Whatever a symbol server + // had to say is the best explanation available for it. Without one there is + // nothing to add beyond the fact of the miss. + if (!fs.Exists(module_spec.GetFileSpec())) { + if (result.symbol_error) + return std::move(*result.symbol_error); + return llvm::make_error<NotFound>(); + } + + return result; +} + void SymbolLocator::DownloadSymbolFileAsync(const UUID &uuid) { static llvm::SmallSet<UUID, 8> g_seen_uuids; static std::mutex g_mutex; diff --git a/lldb/unittests/Symbol/CMakeLists.txt b/lldb/unittests/Symbol/CMakeLists.txt index f9794369a89f4..0fdbc9d72445e 100644 --- a/lldb/unittests/Symbol/CMakeLists.txt +++ b/lldb/unittests/Symbol/CMakeLists.txt @@ -4,6 +4,7 @@ add_lldb_unittest(SymbolTests LocateSymbolFileTest.cpp MangledTest.cpp PostfixExpressionTest.cpp + SymbolLocatorTest.cpp SymbolTest.cpp SymtabTest.cpp SymStoreTest.cpp diff --git a/lldb/unittests/Symbol/SymbolLocatorTest.cpp b/lldb/unittests/Symbol/SymbolLocatorTest.cpp new file mode 100644 index 0000000000000..58a84b30fadbf --- /dev/null +++ b/lldb/unittests/Symbol/SymbolLocatorTest.cpp @@ -0,0 +1,201 @@ +//===----------------------------------------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "lldb/Symbol/SymbolLocator.h" +#include "lldb/Core/PluginManager.h" +#include "lldb/Host/FileSystem.h" +#include "lldb/Host/HostInfo.h" +#include "lldb/Utility/FileSpecList.h" + +#include "llvm/Support/VirtualFileSystem.h" +#include "llvm/Testing/Support/Error.h" + +#include "gtest/gtest.h" + +using namespace lldb; +using namespace lldb_private; + +namespace { + +/// Which steps of the search ran, so a test can tell where an answer came from. +struct LocatorCalls { + bool located_symbol_file = false; + bool located_object_file = false; + bool downloaded = false; +}; + +LocatorCalls g_calls; + +/// What the object file locator claims to have found, if anything. +std::optional<FileSpec> g_object_file; + +/// What the symbol file locator claims to have found, if anything. +std::optional<FileSpec> g_symbol_file; + +/// When set, the symbol server fails the way a failure to launch it does, with +/// an errno rather than a message. +bool g_symbol_server_errno = false; + +std::optional<FileSpec> LocateExecutableSymbolFile(const ModuleSpec &, + const FileSpecList &) { + g_calls.located_symbol_file = true; + return g_symbol_file; +} + +std::optional<ModuleSpec> LocateExecutableObjectFile(const ModuleSpec &spec) { + g_calls.located_object_file = true; + if (!g_object_file) + return {}; + ModuleSpec located(spec); + located.GetFileSpec() = *g_object_file; + return located; +} + +bool DownloadObjectAndSymbolFile(ModuleSpec &, Status &error, bool force_lookup, + bool) { + g_calls.downloaded = true; + if (!force_lookup) + return false; + if (g_symbol_server_errno) + error = Status(std::make_error_code(std::errc::too_many_files_open)); + else + error = Status::FromErrorString("the symbol server said no"); + return false; +} + +SymbolLocator *CreateSymbolLocator() { return nullptr; } + +class SymbolLocatorTest : public testing::Test { +public: + SymbolLocatorTest() + : m_empty_buffer("", "<empty buffer>"), + m_fs(new llvm::vfs::InMemoryFileSystem()) {} + + void SetUp() override { + // Locate reports a binary it cannot find as an error, so a test that wants + // a hit has to point the fake locator at a file that exists. + FileSystem::Initialize(m_fs); + HostInfo::Initialize(); + m_fs->addFileNoOwn(m_binary.GetPath(), 0, m_empty_buffer); + m_fs->addFileNoOwn(m_symbols.GetPath(), 0, m_empty_buffer); + + g_calls = LocatorCalls(); + g_object_file = std::nullopt; + g_symbol_file = std::nullopt; + g_symbol_server_errno = false; + ASSERT_TRUE(PluginManager::RegisterPlugin( + "test", "test symbol locator", CreateSymbolLocator, + LocateExecutableObjectFile, LocateExecutableSymbolFile, + DownloadObjectAndSymbolFile)); + } + + void TearDown() override { + PluginManager::UnregisterPlugin(CreateSymbolLocator); + HostInfo::Terminate(); + FileSystem::Terminate(); + } + + llvm::MemoryBufferRef m_empty_buffer; + llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> m_fs; + + /// Files that exist, for the cases that want a hit. + FileSpec m_binary = FileSpec("/binary", FileSpec::Style::posix); + FileSpec m_symbols = FileSpec("/binary.dSYM", FileSpec::Style::posix); +}; + +} // namespace + +TEST_F(SymbolLocatorTest, MissRunsEveryStep) { + llvm::Expected<SymbolLocator::Result> result = + SymbolLocator::Locate(SymbolLocator::Request(), FileSpecList()); + + EXPECT_TRUE(g_calls.located_symbol_file); + EXPECT_TRUE(g_calls.located_object_file); + // Neither file was found, so the symbol server is the last resort. + EXPECT_TRUE(g_calls.downloaded); + + // Nothing was found and no symbol server was asked, so there is nothing to + // say beyond the fact of the miss. The caller composes its own message. + ASSERT_FALSE(result); + llvm::Error error = result.takeError(); + EXPECT_TRUE(error.isA<SymbolLocator::NotFound>()); + llvm::consumeError(std::move(error)); +} + +TEST_F(SymbolLocatorTest, ReportsWhatThePluginsFound) { + g_object_file = m_binary; + g_symbol_file = m_symbols; + + llvm::Expected<SymbolLocator::Result> result = + SymbolLocator::Locate(SymbolLocator::Request(), FileSpecList()); + + ASSERT_THAT_EXPECTED(result, llvm::Succeeded()); + EXPECT_EQ(m_binary, result->module_spec.GetFileSpec()); + EXPECT_EQ(m_symbols, result->module_spec.GetSymbolFileSpec()); + // Both files were in hand, so there was nothing to ask a symbol server for. + EXPECT_FALSE(g_calls.downloaded); +} + +TEST_F(SymbolLocatorTest, MissCarriesSymbolServerError) { + SymbolLocator::Request request; + request.external_lookup = true; + + llvm::Expected<SymbolLocator::Result> result = + SymbolLocator::Locate(request, FileSpecList()); + + ASSERT_FALSE(result); + llvm::Error error = result.takeError(); + EXPECT_FALSE(error.isA<SymbolLocator::NotFound>()); + EXPECT_EQ("the symbol server said no", llvm::toString(std::move(error))); +} + +TEST_F(SymbolLocatorTest, BinaryWithoutSymbolsIsASuccess) { + // A binary without symbols is still worth loading, and the caller still gets + // to tell the user why the symbols are missing. + g_object_file = m_binary; + SymbolLocator::Request request; + request.external_lookup = true; + + llvm::Expected<SymbolLocator::Result> result = + SymbolLocator::Locate(request, FileSpecList()); + + ASSERT_THAT_EXPECTED(result, llvm::Succeeded()); + EXPECT_EQ(m_binary, result->module_spec.GetFileSpec()); + EXPECT_FALSE(result->module_spec.GetSymbolFileSpec()); + ASSERT_TRUE(result->symbol_error); + EXPECT_EQ("the symbol server said no", + llvm::toString(std::move(*result->symbol_error))); +} + +TEST_F(SymbolLocatorTest, SymbolFileWithoutBinaryIsAMiss) { + // Symbols with no binary to apply them to are of no use, so this is the hard + // failure and not a success carrying half an answer. + g_symbol_file = m_symbols; + + llvm::Expected<SymbolLocator::Result> result = + SymbolLocator::Locate(SymbolLocator::Request(), FileSpecList()); + + EXPECT_THAT_EXPECTED(result, llvm::Failed()); +} + +TEST_F(SymbolLocatorTest, AnErrnoFromTheSymbolServerIsNotAPlainMiss) { + // A Status carrying an errno converts to an llvm::ECError, so an error code + // is not enough to tell a real failure from the miss sentinel. Failing to + // even launch a symbol server has to reach the user. + SymbolLocator::Request request; + request.external_lookup = true; + g_symbol_server_errno = true; + + llvm::Expected<SymbolLocator::Result> result = + SymbolLocator::Locate(request, FileSpecList()); + + ASSERT_FALSE(result); + llvm::Error error = result.takeError(); + EXPECT_FALSE(error.isA<SymbolLocator::NotFound>()); + llvm::consumeError(std::move(error)); +} _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
