Added settings for DEBUGINFOD cache location and timeout

[kevinfrei]

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).

[llvmbot]

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-debuginfo

Author: Kevin Frei (kevinfrei)

Changes

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).

---

Full diff: https://github.com/llvm/llvm-project/pull/78605.diff

4 Files Affected:

• (modified) lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp (+62-15)
• (modified) lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td (+7-1)
• (modified) llvm/include/llvm/Debuginfod/Debuginfod.h (+13)
• (modified) llvm/lib/Debuginfod/Debuginfod.cpp (+23-8)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 111be6be365240e..2a618271624a1b6 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -9,6 +9,7 @@
 #include "SymbolLocatorDebuginfod.h"
 
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Interpreter/OptionValueString.h"
 #include "lldb/Utility/Args.h"
 
 #include "llvm/Debuginfod/Debuginfod.h"
@@ -54,6 +55,34 @@ class PluginProperties : public Properties {
     return urls;
   }
 
+  llvm::Expected<llvm::StringRef> 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;
+      }
+      m_cache_path = *maybeCachePath;
+      return llvm::StringRef(m_cache_path);
+    }
+    return s->GetCurrentValueAsRef();
+  }
+
+  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();
@@ -65,6 +94,7 @@ class PluginProperties : public Properties {
   }
   // Storage for the StringRef's used within the Debuginfod library.
   Args m_server_urls;
+  std::string m_cache_path;
 };
 
 } // namespace
@@ -114,29 +144,46 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
 
 static std::optional<FileSpec> GetFileForModule(
     const ModuleSpec &module_spec,
-    std::function<llvm::Expected<std::string>(llvm::object::BuildIDRef)>
-        PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-    return {};
+                 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 the settings values we need
+  PluginProperties &plugin_props = GetGlobalPluginProperties();
+  llvm::Expected<llvm::StringRef> CacheDirectoryPathOrErr =
+      plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+    return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector<llvm::StringRef> DebuginfodUrls =
+      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 UrlPath = UrlBuilder(build_id);
+  std::string CacheKey = llvm::getDebuginfodCacheKey(UrlPath);
+  llvm::Expected<std::string> result = llvm::getCachedOrDownloadArtifact(
+      CacheKey, UrlPath, CacheDirectoryPath, DebuginfodUrls, Timeout);
+  if (result)
+    return FileSpec(*result);
+  // An error here should be logged as a failure in the Debuginfod library,
+  // just consume it here
+  consumeError(result.takeError());
   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);
 }
diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
index 1c668b001a16557..7f17faa8241aa78 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
+++ b/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 defaults to DEBUGINFOD_TIMEOUT environment variable (or 90 seconds).">;
 }
diff --git a/llvm/include/llvm/Debuginfod/Debuginfod.h b/llvm/include/llvm/Debuginfod/Debuginfod.h
index 251fd7005305e74..719bbf9cd0ccca2 100644
--- a/llvm/include/llvm/Debuginfod/Debuginfod.h
+++ b/llvm/include/llvm/Debuginfod/Debuginfod.h
@@ -46,6 +46,9 @@ bool canUseDebuginfod();
 /// environment variable.
 SmallVector<StringRef> getDefaultDebuginfodUrls();
 
+/// Creates the cache-key for a given Debuginfod UrlPath
+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,25 @@ Expected<std::string> getDefaultDebuginfodCacheDirectory();
 /// DEBUGINFOD_TIMEOUT environment variable, default is 90 seconds (90000 ms).
 std::chrono::milliseconds getDefaultDebuginfodTimeout();
 
+/// Get the full UrlPath 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 UrlPath 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 UrlPath 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);
diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp
index 9df30ab55cbad48..5cabb40e3551ed5 100644
--- a/llvm/lib/Debuginfod/Debuginfod.cpp
+++ b/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));
 }
 
@@ -120,29 +120,44 @@ 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.
 Expected<std::string> getCachedOrDownloadArtifact(StringRef UniqueKey,
                                                   StringRef UrlPath) {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[mysterymath]

LGTM for debuginfod

[kevinfrei]

LGTM for debuginfod

I've considered moving that entire thing into a namespace. Would you be amenable to that (at a later date...)?

Unimportant: The OMF2097 github pic is pretty excellent, but clearly you're almost as old as me...

[JDevlieghere]

Thanks! This LGTM.

[clayborg]

Looks good to me on the LLDB side, but you should probably get the ok from the maintainers of the debug infod library for those changes.

[kevinfrei]

Looks good to me on the LLDB side, but you should probably get the ok from the maintainers of the debug infod library for those changes.

I think that was @mysterymath