Author: Jonas Devlieghere Date: 2026-08-20T11:21:11-07:00 New Revision: da92ff6a449ffd56e46781c7b893a639296686eb
URL: https://github.com/llvm/llvm-project/commit/da92ff6a449ffd56e46781c7b893a639296686eb DIFF: https://github.com/llvm/llvm-project/commit/da92ff6a449ffd56e46781c7b893a639296686eb.diff LOG: [lldb] Only report a symbol server error when the download failed (#217669) Every symbol locator plugin writes into the same Status, and a plugin that delivers a symbol file is under no obligation to clear what an earlier step recorded there, so a search that ended in a download could still report an error at the user. That contradicts what symbol_error promises its readers, which is an explanation for a search that came up short. Take the download's own answer as the authority on whether anything was delivered, and consult the Status only when nothing was. Reported by Coverity (CID 1685292). Assisted-by: Claude Added: Modified: lldb/source/Symbol/SymbolLocator.cpp lldb/unittests/Symbol/SymbolLocatorTest.cpp Removed: ################################################################################ diff --git a/lldb/source/Symbol/SymbolLocator.cpp b/lldb/source/Symbol/SymbolLocator.cpp index 58304aad24100..effce99d3bdaa 100644 --- a/lldb/source/Symbol/SymbolLocator.cpp +++ b/lldb/source/Symbol/SymbolLocator.cpp @@ -52,9 +52,11 @@ SymbolLocator::LocateWithPlugins(const Request &request, 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) + const bool downloaded = PluginManager::DownloadObjectAndSymbolFile( + module_spec, error, request.external_lookup); + // The plugins share one Status, so a failure recorded along the way says + // nothing about a search that ended in a download. + if (!downloaded && error.Fail() && request.external_lookup) result.symbol_error.emplace(error.takeError()); } diff --git a/lldb/unittests/Symbol/SymbolLocatorTest.cpp b/lldb/unittests/Symbol/SymbolLocatorTest.cpp index cc7e8854cf1b7..e5d27cd722f7a 100644 --- a/lldb/unittests/Symbol/SymbolLocatorTest.cpp +++ b/lldb/unittests/Symbol/SymbolLocatorTest.cpp @@ -53,6 +53,9 @@ std::optional<FileSpec> g_object_file; /// What the symbol file locator claims to have found, if anything. std::optional<FileSpec> g_symbol_file; +/// What the symbol server claims to have downloaded, if anything. +std::optional<FileSpec> g_downloaded_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; @@ -83,8 +86,8 @@ std::optional<ModuleSpec> LocateExecutableObjectFile(const ModuleSpec &spec) { return located; } -bool DownloadObjectAndSymbolFile(ModuleSpec &, Status &error, bool force_lookup, - bool) { +bool DownloadObjectAndSymbolFile(ModuleSpec &module_spec, Status &error, + bool force_lookup, bool) { g_calls.downloaded = true; if (!force_lookup) return false; @@ -92,7 +95,11 @@ bool DownloadObjectAndSymbolFile(ModuleSpec &, Status &error, bool force_lookup, error = Status(std::make_error_code(std::errc::too_many_files_open)); else error = Status::FromErrorString("the symbol server said no"); - return false; + // A server that delivers is under no obligation to leave the Status alone. + if (!g_downloaded_symbol_file) + return false; + module_spec.GetSymbolFileSpec() = *g_downloaded_symbol_file; + return true; } SymbolLocator *CreateSymbolLocator() { return nullptr; } @@ -154,6 +161,7 @@ class SymbolLocatorTest : public testing::Test { g_calls.Clear(); g_object_file = std::nullopt; g_symbol_file = std::nullopt; + g_downloaded_symbol_file = std::nullopt; g_symbol_server_errno = false; g_only_with_uuid = false; g_barrier = nullptr; @@ -246,6 +254,24 @@ TEST_F(SymbolLocatorTest, BinaryWithoutSymbolsIsASuccess) { llvm::toString(std::move(*result->symbol_error))); } +TEST_F(SymbolLocatorTest, ASuccessfulDownloadIsNotASymbolError) { + // A symbol server that delivered has nothing left to explain. + g_object_file = m_binary; + g_downloaded_symbol_file = m_symbols; + 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_symbols, result->module_spec.GetSymbolFileSpec()); + // Consumed in the failure message, because an unchecked Error aborts when it + // goes out of scope. + EXPECT_FALSE(result->symbol_error) + << 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. _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
