From 0600779b5503564cc8068e43a7dd9f194f76043b Mon Sep 17 00:00:00 2001 From: Kevin Frei Date: Fri, 22 Mar 2024 09:35:47 -0700 Subject: [PATCH] Cleaning up changes from a couple months back. --- lldb/include/lldb/Symbol/SymbolLocator.h | 5 + .../Debuginfod/SymbolLocatorDebuginfod.cpp | 135 +++++++++++++++--- .../Debuginfod/SymbolLocatorDebuginfod.h | 8 ++ .../SymbolLocatorDebuginfodProperties.td | 4 + lldb/source/Symbol/SymbolLocator.cpp | 14 +- llvm/include/llvm/Debuginfod/Debuginfod.h | 4 + llvm/lib/Debuginfod/Debuginfod.cpp | 41 +++++- 7 files changed, 184 insertions(+), 27 deletions(-) diff --git a/lldb/include/lldb/Symbol/SymbolLocator.h b/lldb/include/lldb/Symbol/SymbolLocator.h index 8d2f26b3d0cca3..c3af97c2a189ef 100644 --- a/lldb/include/lldb/Symbol/SymbolLocator.h +++ b/lldb/include/lldb/Symbol/SymbolLocator.h @@ -24,6 +24,11 @@ class SymbolLocator : public PluginInterface { /// found, this will notify all target which contain the module with the /// given UUID. static void DownloadSymbolFileAsync(const UUID &uuid); + /// Normally, DownloadSymbolFileAsync will only try to download a given + /// UUID once. If the configuration for locating symbols has been changed + /// by the user, this function will reset that counter so that we will + /// attempt to download again. + static void ResetDownloadAttempts(); }; } // namespace lldb_private diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp index 2cd7bbbb244902..c8fbf2092e1b69 100644 --- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp +++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp @@ -22,6 +22,39 @@ using namespace lldb_private; LLDB_PLUGIN_DEFINE(SymbolLocatorDebuginfod) +enum SymbolLookupMode { + eLookupModeDisabled, + eLookupModeOnDemand, + eLookupModeAlways, +}; + +static constexpr OptionEnumValueElement g_debuginfod_symbol_lookup_mode[] = { + { + eLookupModeDisabled, + "disabled", + "Do not query DEBUGINFOD servers for symbols. Cached symbols are still " + "used. To fully disable any use of symbols located using DEBUGINFOD, " + "set symbols.enable-external-lookup to false.", + }, + { + eLookupModeOnDemand, + "on-demand", + "Only query DEBUGINFOD servers when they're explicitly requested via " + "commands (such as 'target symbols add' or 'target modules add') or " + "when they're requested asynchronously (if " + "symbols.enable-background-lookup is set). Any cached symbols " + "previously acquired are still used.", + }, + { + eLookupModeAlways, + "always", + "Always try to find debug information for any executable or shared " + "library in any debug session as the shared libraries are loaded. Note " + "that this can cause a lot of debug information to appear in your " + "project and may slow down your debug session.", + }, +}; + namespace { #define LLDB_PROPERTIES_symbollocatordebuginfod @@ -57,6 +90,13 @@ class PluginProperties : public Properties { return urls; } + SymbolLookupMode GetLookupMode() const { + uint32_t idx = ePropertyEnableAutoLookup; + return GetPropertyAtIndexAs( + idx, static_cast( + g_debuginfod_symbol_lookup_mode[idx].value)); + } + llvm::Expected GetCachePath() { OptionValueString *s = m_collection_sp->GetPropertyAtIndexAsOptionValueString( @@ -90,6 +130,8 @@ class PluginProperties : public Properties { llvm::for_each(m_server_urls, [&](const auto &obj) { dbginfod_urls.push_back(obj.ref()); }); + // Something's changed: reset the background attempts counter + SymbolLocator::ResetDownloadAttempts(); llvm::setDefaultDebuginfodUrls(dbginfod_urls); } // Storage for the StringRef's used within the Debuginfod library. @@ -111,8 +153,9 @@ void SymbolLocatorDebuginfod::Initialize() { llvm::call_once(g_once_flag, []() { PluginManager::RegisterPlugin( GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance, - LocateExecutableObjectFile, LocateExecutableSymbolFile, nullptr, - nullptr, SymbolLocatorDebuginfod::DebuggerInitialize); + LocateExecutableObjectFile, LocateExecutableSymbolFile, + DownloadObjectAndSymbolFile, nullptr, + SymbolLocatorDebuginfod::DebuggerInitialize); llvm::HTTPClient::initialize(); }); } @@ -143,12 +186,11 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() { static std::optional GetFileForModule(const ModuleSpec &module_spec, - std::function UrlBuilder) { + std::function url_builder, + bool sync_lookup) { const UUID &module_uuid = module_spec.GetUUID(); - // Don't bother if we don't have a valid UUID, Debuginfod isn't available, - // or if the 'symbols.enable-external-lookup' setting is false. - if (!module_uuid.IsValid() || !llvm::canUseDebuginfod() || - !ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup()) + // Quit early if we don't have a valid UUID or if Debuginfod doesn't work. + if (!module_uuid.IsValid() || !llvm::canUseDebuginfod()) return {}; // Grab LLDB's Debuginfod overrides from the @@ -162,30 +204,87 @@ GetFileForModule(const ModuleSpec &module_spec, llvm::SmallVector debuginfod_urls = llvm::getDefaultDebuginfodUrls(); std::chrono::milliseconds timeout = plugin_props.GetTimeout(); + // sync_lookup is also 'force_lookup' which overrides the global setting + if (!sync_lookup && + !ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup()) + return {}; // We're ready to ask the Debuginfod library to find our file. llvm::object::BuildID build_id(module_uuid.GetBytes()); - std::string url_path = UrlBuilder(build_id); + std::string url_path = url_builder(build_id); std::string cache_key = llvm::getDebuginfodCacheKey(url_path); - llvm::Expected result = llvm::getCachedOrDownloadArtifact( - cache_key, url_path, cache_path, debuginfod_urls, timeout); + bool ask_server = sync_lookup || plugin_props.GetLookupMode() == + SymbolLookupMode::eLookupModeAlways; + llvm::Expected result = + ask_server + ? llvm::getCachedOrDownloadArtifact(cache_key, url_path, cache_path, + debuginfod_urls, timeout) + : llvm::getCachedArtifact(cache_key, cache_path); if (result) return FileSpec(*result); - - Log *log = GetLog(LLDBLog::Symbols); - auto err_message = llvm::toString(result.takeError()); - LLDB_LOGV(log, - "Debuginfod failed to download symbol artifact {0} with error {1}", - url_path, err_message); + if (!ask_server) + // If we only checked the cache & failed, query the server asynchronously. + // This API only requests the symbols if the user has enabled the + // 'symbol.enable-background-lookup' setting. + SymbolLocator::DownloadSymbolFileAsync(module_uuid); + else { + Log *log = GetLog(LLDBLog::Symbols); + auto err_message = llvm::toString(result.takeError()); + LLDB_LOGV( + log, "Debuginfod failed to download symbol artifact {0} with error {1}", + url_path, err_message); + } return {}; } std::optional SymbolLocatorDebuginfod::LocateExecutableObjectFile( const ModuleSpec &module_spec) { - return GetFileForModule(module_spec, llvm::getDebuginfodExecutableUrlPath); + return GetFileForModule(module_spec, llvm::getDebuginfodExecutableUrlPath, + false); } std::optional SymbolLocatorDebuginfod::LocateExecutableSymbolFile( const ModuleSpec &module_spec, const FileSpecList &default_search_paths) { - return GetFileForModule(module_spec, llvm::getDebuginfodDebuginfoUrlPath); + return GetFileForModule(module_spec, llvm::getDebuginfodDebuginfoUrlPath, + false); +} + +// This API is only used asynchronously, or when the user explicitly asks for +// symbols via target symbols add +bool SymbolLocatorDebuginfod::DownloadObjectAndSymbolFile( + ModuleSpec &module_spec, Status &error, bool sync_lookup, + bool copy_executable) { + // copy_executable is only used for macOS kernel debugging stuff involving + // dSYM bundles, so we're not using it here. + const UUID *uuid_ptr = module_spec.GetUUIDPtr(); + const FileSpec *file_spec_ptr = module_spec.GetFileSpecPtr(); + + // We need a UUID or valid existing FileSpec. + if (!uuid_ptr && + (!file_spec_ptr || !FileSystem::Instance().Exists(*file_spec_ptr))) + return false; + + // For DWP files, if you're running a stripped binary, you'll probably want to + // get *both* the symbols and the executable. If your binary isn't stripped, + // then you won't need the executable, but for now, we'll try to download + // both. + bool found = false; + if (!module_spec.GetSymbolFileSpec()) { + std::optional SymbolFile = GetFileForModule( + module_spec, llvm::getDebuginfodDebuginfoUrlPath, sync_lookup); + if (SymbolFile) { + module_spec.GetSymbolFileSpec() = *SymbolFile; + found = true; + } + } + + if (!module_spec.GetFileSpec()) { + std::optional ExecutableFile = GetFileForModule( + module_spec, llvm::getDebuginfodExecutableUrlPath, sync_lookup); + if (ExecutableFile) { + module_spec.GetFileSpec() = *ExecutableFile; + found = true; + } + } + return found; } diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.h b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.h index 0ea79fa1df2a5f..0ebfdbfb84ccd8 100644 --- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.h +++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.h @@ -47,6 +47,14 @@ class SymbolLocatorDebuginfod : public SymbolLocator { static std::optional LocateExecutableSymbolFile(const ModuleSpec &module_spec, const FileSpecList &default_search_paths); + + // Download the object and symbol files given a module specification. + // + // This will be done asynchronously, and is controlled by the + // plugin.symbol-locator.debuginfod.lookup_mode setting. + static bool DownloadObjectAndSymbolFile(ModuleSpec &module_spec, + Status &error, bool sync_lookup, + bool copy_executable); }; } // namespace lldb_private diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td index 0ff02674b8ea31..24f64ee09cb78b 100644 --- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td +++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td @@ -7,6 +7,10 @@ let Definition = "symbollocatordebuginfod" in { def SymbolCachePath: Property<"cache-path", "String">, DefaultStringValue<"">, Desc<"The path where symbol files should be cached. This defaults to LLDB's system cache location.">; + def EnableAutoLookup : Property<"lookup-mode", "Enum">, + DefaultEnumValue<"eLookupModeOnDemand">, + EnumValues<"OptionEnumValues(g_debuginfod_symbol_lookup_mode)">, + Desc<"Control when DEBUGINFOD servers are queried for symbols">; def Timeout : Property<"timeout", "UInt64">, DefaultUnsignedValue<0>, Desc<"Timeout (in seconds) for requests made to a DEBUGINFOD server. A value of zero means we use the debuginfod default timeout: DEBUGINFOD_TIMEOUT if the environment variable is set and 90 seconds otherwise.">; diff --git a/lldb/source/Symbol/SymbolLocator.cpp b/lldb/source/Symbol/SymbolLocator.cpp index 93a5bc428b6140..0e112e90117c39 100644 --- a/lldb/source/Symbol/SymbolLocator.cpp +++ b/lldb/source/Symbol/SymbolLocator.cpp @@ -18,9 +18,14 @@ using namespace lldb; using namespace lldb_private; +namespace { + +llvm::SmallSet g_seen_uuids; +static std::mutex g_mutex; + +} // namespace + void SymbolLocator::DownloadSymbolFileAsync(const UUID &uuid) { - static llvm::SmallSet g_seen_uuids; - static std::mutex g_mutex; auto lookup = [=]() { { @@ -55,3 +60,8 @@ void SymbolLocator::DownloadSymbolFileAsync(const UUID &uuid) { break; }; } + +void SymbolLocator::ResetDownloadAttempts() { + std::lock_guard guard(g_mutex); + g_seen_uuids.clear(); +} diff --git a/llvm/include/llvm/Debuginfod/Debuginfod.h b/llvm/include/llvm/Debuginfod/Debuginfod.h index 99fe15ad859794..d02dbfd28a9e90 100644 --- a/llvm/include/llvm/Debuginfod/Debuginfod.h +++ b/llvm/include/llvm/Debuginfod/Debuginfod.h @@ -85,6 +85,10 @@ std::string getDebuginfodDebuginfoUrlPath(object::BuildIDRef ID); /// server URLs. Expected getCachedOrDownloadDebuginfo(object::BuildIDRef ID); +/// Fetches any debuginfod artifact from the specified cache directory and key. +Expected getCachedArtifact(StringRef UniqueKey, + StringRef CacheDirectoryPath); + /// Fetches any debuginfod artifact using the default local cache directory and /// server URLs. Expected getCachedOrDownloadArtifact(StringRef UniqueKey, diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp index 4c785117ae8ef7..62935da131238a 100644 --- a/llvm/lib/Debuginfod/Debuginfod.cpp +++ b/llvm/lib/Debuginfod/Debuginfod.cpp @@ -248,27 +248,54 @@ static SmallVector getHeaders() { return Headers; } -Expected getCachedOrDownloadArtifact( - StringRef UniqueKey, StringRef UrlPath, StringRef CacheDirectoryPath, - ArrayRef DebuginfodUrls, std::chrono::milliseconds Timeout) { +static SmallString<64> getCachedArtifactPath(StringRef UniqueKey, + StringRef CacheDirectoryPath) { SmallString<64> AbsCachedArtifactPath; sys::path::append(AbsCachedArtifactPath, CacheDirectoryPath, "llvmcache-" + UniqueKey); + return AbsCachedArtifactPath; +} + +static Expected +getCachedArtifactHelper(StringRef UniqueKey, StringRef CacheDirectoryPath, + unsigned &Task) { + SmallString<64> AbsCachedArtifactPath = + getCachedArtifactPath(UniqueKey, CacheDirectoryPath); Expected CacheOrErr = localCache("Debuginfod-client", ".debuginfod-client", CacheDirectoryPath); if (!CacheOrErr) return CacheOrErr.takeError(); + return (*CacheOrErr)(Task, UniqueKey, ""); +} + +Expected getCachedArtifact(StringRef UniqueKey, + StringRef CacheDirectoryPath) { + // We choose an arbitrary Task parameter as we do not make use of it. + unsigned Task = 0; + Expected CacheAddStreamOrErr = + getCachedArtifactHelper(UniqueKey, CacheDirectoryPath, Task); + if (!CacheAddStreamOrErr) + return CacheAddStreamOrErr.takeError(); + if (!*CacheAddStreamOrErr) + return std::string(getCachedArtifactPath(UniqueKey, CacheDirectoryPath)); + return createStringError(errc::argument_out_of_domain, + "build id not found in cache"); +} - FileCache Cache = *CacheOrErr; +Expected getCachedOrDownloadArtifact( + StringRef UniqueKey, StringRef UrlPath, StringRef CacheDirectoryPath, + ArrayRef DebuginfodUrls, std::chrono::milliseconds Timeout) { // We choose an arbitrary Task parameter as we do not make use of it. unsigned Task = 0; - Expected CacheAddStreamOrErr = Cache(Task, UniqueKey, ""); + Expected CacheAddStreamOrErr = + getCachedArtifactHelper(UniqueKey, CacheDirectoryPath, Task); if (!CacheAddStreamOrErr) return CacheAddStreamOrErr.takeError(); AddStreamFn &CacheAddStream = *CacheAddStreamOrErr; if (!CacheAddStream) - return std::string(AbsCachedArtifactPath); + return std::string(getCachedArtifactPath(UniqueKey, CacheDirectoryPath)); + // The artifact was not found in the local cache, query the debuginfod // servers. if (!HTTPClient::isAvailable()) @@ -311,7 +338,7 @@ Expected getCachedOrDownloadArtifact( pruneCache(CacheDirectoryPath, *PruningPolicyOrErr); // Return the path to the artifact on disk. - return std::string(AbsCachedArtifactPath); + return std::string(getCachedArtifactPath(UniqueKey, CacheDirectoryPath)); } return createStringError(errc::argument_out_of_domain, "build id not found");