Added settings for DEBUGINFOD cache location and timeout (llvm#78605)

I've been working on more/better configuration for improving DEBUGINFOD
support. This is the first (and easiest) slice of the work.

I've added `timeout` and `cache-path` settings that can override the
DEBUGINFOD library defaults (and environment variables.) I also renamed
the `plugin.symbol-locator.debuginfod.server_urls` setting to
`server-urls` to be more consistent with the rest of LLDB's settings
(the underscore switch is switched to a hyphen)

I've got a few tests that validate the cache-path setting (as a
side-effect), but they've exposed a few bugs that I'll be putting up a
separate PR for (which will include the tests).

---------

Co-authored-by: Kevin Frei <[email protected]>

Author: kevinfrei

lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp

@@ -9,7 +9,10 @@
 #include "SymbolLocatorDebuginfod.h"
 
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Interpreter/OptionValueString.h"
 #include "lldb/Utility/Args.h"
+#include "lldb/Utility/LLDBLog.h"
+#include "lldb/Utility/Log.h"
 
 #include "llvm/Debuginfod/Debuginfod.h"
 #include "llvm/Debuginfod/HTTPClient.h"
@@ -54,6 +57,32 @@ class PluginProperties : public Properties {
     return urls;
   }
 
+  llvm::Expected<std::string> GetCachePath() {
+    OptionValueString *s =
+        m_collection_sp->GetPropertyAtIndexAsOptionValueString(
+            ePropertySymbolCachePath);
+    // If we don't have a valid cache location, use the default one.
+    if (!s || !s->GetCurrentValueAsRef().size()) {
+      llvm::Expected<std::string> maybeCachePath =
+          llvm::getDefaultDebuginfodCacheDirectory();
+      if (!maybeCachePath)
+        return maybeCachePath;
+      return *maybeCachePath;
+    }
+    return s->GetCurrentValue();
+  }
+
+  std::chrono::milliseconds GetTimeout() const {
+    std::optional<uint64_t> seconds =
+        m_collection_sp->GetPropertyAtIndexAs<uint64_t>(ePropertyTimeout);
+    if (seconds && *seconds != 0) {
+      return std::chrono::duration_cast<std::chrono::milliseconds>(
+          std::chrono::seconds(*seconds));
+    } else {
+      return llvm::getDefaultDebuginfodTimeout();
+    }
+  }
+
 private:
   void ServerURLsChangedCallback() {
     m_server_urls = GetDebugInfoDURLs();
@@ -112,31 +141,51 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional<FileSpec> GetFileForModule(
-    const ModuleSpec &module_spec,
-    std::function<llvm::Expected<std::string>(llvm::object::BuildIDRef)>
-        PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-    return {};
+static std::optional<FileSpec>
+GetFileForModule(const ModuleSpec &module_spec,
+                 std::function<std::string(llvm::object::BuildID)> UrlBuilder) {
   const UUID &module_uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-    llvm::object::BuildID build_id(module_uuid.GetBytes());
-    llvm::Expected<std::string> result = PullFromServer(build_id);
-    if (result)
-      return FileSpec(*result);
-    // An error here should be logged as a failure in the Debuginfod library,
-    // so just consume it here
-    consumeError(result.takeError());
-  }
+  // 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())
+    return {};
+
+  // Grab LLDB's Debuginfod overrides from the
+  // plugin.symbol-locator.debuginfod.* settings.
+  PluginProperties &plugin_props = GetGlobalPluginProperties();
+  llvm::Expected<std::string> cache_path_or_err = plugin_props.GetCachePath();
+  // A cache location is *required*.
+  if (!cache_path_or_err)
+    return {};
+  std::string cache_path = *cache_path_or_err;
+  llvm::SmallVector<llvm::StringRef> debuginfod_urls =
+      llvm::getDefaultDebuginfodUrls();
+  std::chrono::milliseconds timeout = plugin_props.GetTimeout();
+
+  // 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 cache_key = llvm::getDebuginfodCacheKey(url_path);
+  llvm::Expected<std::string> result = llvm::getCachedOrDownloadArtifact(
+      cache_key, url_path, cache_path, debuginfod_urls, timeout);
+  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);
   return {};
 }
 
 std::optional<ModuleSpec> SymbolLocatorDebuginfod::LocateExecutableObjectFile(
     const ModuleSpec &module_spec) {
-  return GetFileForModule(module_spec, llvm::getCachedOrDownloadExecutable);
+  return GetFileForModule(module_spec, llvm::getDebuginfodExecutableUrlPath);
 }
 
 std::optional<FileSpec> SymbolLocatorDebuginfod::LocateExecutableSymbolFile(
     const ModuleSpec &module_spec, const FileSpecList &default_search_paths) {
-  return GetFileForModule(module_spec, llvm::getCachedOrDownloadDebuginfo);
+  return GetFileForModule(module_spec, llvm::getDebuginfodDebuginfoUrlPath);
 }

lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td

@@ -1,7 +1,13 @@
 include "../../../../include/lldb/Core/PropertiesBase.td"
 
 let Definition = "symbollocatordebuginfod" in {
-  def ServerURLs : Property<"server_urls", "Array">,
+  def ServerURLs : Property<"server-urls", "Array">,
     ElementType<"String">,
     Desc<"An ordered list of Debuginfod server URLs to query for symbols. This defaults to the contents of the DEBUGINFOD_URLS environment variable.">;
+  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 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.">;
 }

llvm/include/llvm/Debuginfod/Debuginfod.h

@@ -46,6 +46,9 @@ bool canUseDebuginfod();
 /// environment variable.
 SmallVector<StringRef> getDefaultDebuginfodUrls();
 
+/// Returns the cache key for a given debuginfod URL path.
+std::string getDebuginfodCacheKey(StringRef UrlPath);
+
 /// Sets the list of debuginfod server URLs to query. This overrides the
 /// environment variable DEBUGINFOD_URLS.
 void setDefaultDebuginfodUrls(const SmallVector<StringRef> &URLs);
@@ -58,15 +61,26 @@ Expected<std::string> getDefaultDebuginfodCacheDirectory();
 /// DEBUGINFOD_TIMEOUT environment variable, default is 90 seconds (90000 ms).
 std::chrono::milliseconds getDefaultDebuginfodTimeout();
 
+/// Get the full URL path for a source request of a given BuildID and file
+/// path.
+std::string getDebuginfodSourceUrlPath(object::BuildIDRef ID,
+                                       StringRef SourceFilePath);
+
 /// Fetches a specified source file by searching the default local cache
 /// directory and server URLs.
 Expected<std::string> getCachedOrDownloadSource(object::BuildIDRef ID,
                                                 StringRef SourceFilePath);
 
+/// Get the full URL path for an executable request of a given BuildID.
+std::string getDebuginfodExecutableUrlPath(object::BuildIDRef ID);
+
 /// Fetches an executable by searching the default local cache directory and
 /// server URLs.
 Expected<std::string> getCachedOrDownloadExecutable(object::BuildIDRef ID);
 
+/// Get the full URL path for a debug binary request of a given BuildID.
+std::string getDebuginfodDebuginfoUrlPath(object::BuildIDRef ID);
+
 /// Fetches a debug binary by searching the default local cache directory and
 /// server URLs.
 Expected<std::string> getCachedOrDownloadDebuginfo(object::BuildIDRef ID);

llvm/lib/Debuginfod/Debuginfod.cpp

@@ -54,7 +54,7 @@ std::optional<SmallVector<StringRef>> DebuginfodUrls;
 llvm::sys::RWMutex UrlsMutex;
 } // namespace
 
-static std::string uniqueKey(llvm::StringRef S) {
+std::string getDebuginfodCacheKey(llvm::StringRef S) {
   return utostr(xxh3_64bits(S));
 }
 
@@ -72,7 +72,7 @@ SmallVector<StringRef> getDefaultDebuginfodUrls() {
   std::shared_lock<llvm::sys::RWMutex> ReadGuard(UrlsMutex);
   if (!DebuginfodUrls) {
     // Only read from the environment variable if the user hasn't already
-    // set the value
+    // set the value.
     ReadGuard.unlock();
     std::unique_lock<llvm::sys::RWMutex> WriteGuard(UrlsMutex);
     DebuginfodUrls = SmallVector<StringRef>();
@@ -86,7 +86,7 @@ SmallVector<StringRef> getDefaultDebuginfodUrls() {
   return DebuginfodUrls.value();
 }
 
-// Set the default debuginfod URL list, override the environment variable
+// Set the default debuginfod URL list, override the environment variable.
 void setDefaultDebuginfodUrls(const SmallVector<StringRef> &URLs) {
   std::unique_lock<llvm::sys::RWMutex> WriteGuard(UrlsMutex);
   DebuginfodUrls = URLs;
@@ -120,27 +120,43 @@ std::chrono::milliseconds getDefaultDebuginfodTimeout() {
 /// cache and return the cached file path. They first search the local cache,
 /// followed by the debuginfod servers.
 
-Expected<std::string> getCachedOrDownloadSource(BuildIDRef ID,
-                                                StringRef SourceFilePath) {
+std::string getDebuginfodSourceUrlPath(BuildIDRef ID,
+                                       StringRef SourceFilePath) {
   SmallString<64> UrlPath;
   sys::path::append(UrlPath, sys::path::Style::posix, "buildid",
                     buildIDToString(ID), "source",
                     sys::path::convert_to_slash(SourceFilePath));
-  return getCachedOrDownloadArtifact(uniqueKey(UrlPath), UrlPath);
+  return std::string(UrlPath);
 }
 
-Expected<std::string> getCachedOrDownloadExecutable(BuildIDRef ID) {
+Expected<std::string> getCachedOrDownloadSource(BuildIDRef ID,
+                                                StringRef SourceFilePath) {
+  std::string UrlPath = getDebuginfodSourceUrlPath(ID, SourceFilePath);
+  return getCachedOrDownloadArtifact(getDebuginfodCacheKey(UrlPath), UrlPath);
+}
+
+std::string getDebuginfodExecutableUrlPath(BuildIDRef ID) {
   SmallString<64> UrlPath;
   sys::path::append(UrlPath, sys::path::Style::posix, "buildid",
                     buildIDToString(ID), "executable");
-  return getCachedOrDownloadArtifact(uniqueKey(UrlPath), UrlPath);
+  return std::string(UrlPath);
 }
 
-Expected<std::string> getCachedOrDownloadDebuginfo(BuildIDRef ID) {
+Expected<std::string> getCachedOrDownloadExecutable(BuildIDRef ID) {
+  std::string UrlPath = getDebuginfodExecutableUrlPath(ID);
+  return getCachedOrDownloadArtifact(getDebuginfodCacheKey(UrlPath), UrlPath);
+}
+
+std::string getDebuginfodDebuginfoUrlPath(BuildIDRef ID) {
   SmallString<64> UrlPath;
   sys::path::append(UrlPath, sys::path::Style::posix, "buildid",
                     buildIDToString(ID), "debuginfo");
-  return getCachedOrDownloadArtifact(uniqueKey(UrlPath), UrlPath);
+  return std::string(UrlPath);
+}
+
+Expected<std::string> getCachedOrDownloadDebuginfo(BuildIDRef ID) {
+  std::string UrlPath = getDebuginfodDebuginfoUrlPath(ID);
+  return getCachedOrDownloadArtifact(getDebuginfodCacheKey(UrlPath), UrlPath);
 }
 
 // General fetching function.