[llvm] [lldb] Added settings for DEBUGINFOD cache location and timeout (PR #78605)

Kevin Frei via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 19 06:53:18 PST 2024


https://github.com/kevinfrei updated https://github.com/llvm/llvm-project/pull/78605

>From 773740afcf9c54ef45a1178ee681ee46c29c9759 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 09:09:50 -0800
Subject: [PATCH 1/8] Added settings for cache location and timeout

---
 .../Debuginfod/SymbolLocatorDebuginfod.cpp    | 82 +++++++++++++++----
 .../SymbolLocatorDebuginfodProperties.td      |  8 +-
 llvm/include/llvm/Debuginfod/Debuginfod.h     | 13 +++
 llvm/lib/Debuginfod/Debuginfod.cpp            | 31 +++++--
 4 files changed, 108 insertions(+), 26 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 111be6be365240..a20437c256eb43 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
@@ -112,31 +142,49 @@ 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 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 1c668b001a1655..7f17faa8241aa7 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 251fd7005305e7..719bbf9cd0ccca 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 9df30ab55cbad4..5cabb40e3551ed 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) {

>From 410317c02caffbfc5daf47a8b7d8a9346e1d16ea Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 10:32:03 -0800
Subject: [PATCH 2/8] Typo

---
 .../SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp   | 7 +++----
 llvm/lib/Debuginfod/Debuginfod.cpp                         | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index a20437c256eb43..afbc7d8b12febf 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -142,10 +142,9 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-+ static std::optional<FileSpec> +
-    GetFileForModule(
-        const ModuleSpec &module_spec,
-        std::function<std::string(llvm::object::BuildID)> UrlBuilder) {
+static std::optional<FileSpec>
+GetFileForModule(const ModuleSpec &module_spec,
+                 std::function<std::string(llvm::object::BuildID)> UrlBuilder) {
   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
diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp
index 5cabb40e3551ed..bdba384815ebf6 100644
--- a/llvm/lib/Debuginfod/Debuginfod.cpp
+++ b/llvm/lib/Debuginfod/Debuginfod.cpp
@@ -158,6 +158,7 @@ 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) {

>From 6ec2a3cf3722e4a85fb071eb73f0a7972512cab1 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 12:46:02 -0800
Subject: [PATCH 3/8] Comment cleanup

---
 .../Debuginfod/SymbolLocatorDebuginfod.cpp            | 11 ++++++-----
 llvm/lib/Debuginfod/Debuginfod.cpp                    |  4 ++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index afbc7d8b12febf..45e2f5acb11e96 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -147,16 +147,17 @@ GetFileForModule(const ModuleSpec &module_spec,
                  std::function<std::string(llvm::object::BuildID)> UrlBuilder) {
   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
+  // 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
+  // Grab LLDB's Debuginfod overrides from the
+  // plugin.symbol-locator.debuginfod.* settings.
   PluginProperties &plugin_props = GetGlobalPluginProperties();
   llvm::Expected<llvm::StringRef> CacheDirectoryPathOrErr =
       plugin_props.GetCachePath();
-  // A cache location is *required*
+  // A cache location is *required*.
   if (!CacheDirectoryPathOrErr)
     return {};
   llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
@@ -164,7 +165,7 @@ GetFileForModule(const ModuleSpec &module_spec,
       llvm::getDefaultDebuginfodUrls();
   std::chrono::milliseconds Timeout = plugin_props.GetTimeout();
 
-  // We're ready to ask the Debuginfod library to find our file
+  // 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);
@@ -173,7 +174,7 @@ GetFileForModule(const ModuleSpec &module_spec,
   if (result)
     return FileSpec(*result);
   // An error here should be logged as a failure in the Debuginfod library,
-  // just consume it here
+  // just consume it here.
   consumeError(result.takeError());
   return {};
 }
diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp
index bdba384815ebf6..4928fcb3b3f879 100644
--- a/llvm/lib/Debuginfod/Debuginfod.cpp
+++ b/llvm/lib/Debuginfod/Debuginfod.cpp
@@ -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;

>From a3ca5abebdad1f551a0eedd022e5920af833b26b Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 14:09:47 -0800
Subject: [PATCH 4/8] Switch locals from PascalCase to snake_case

---
 .../Debuginfod/SymbolLocatorDebuginfod.cpp       | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 45e2f5acb11e96..680167ebecd951 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -155,22 +155,22 @@ GetFileForModule(const ModuleSpec &module_spec,
   // Grab LLDB's Debuginfod overrides from the
   // plugin.symbol-locator.debuginfod.* settings.
   PluginProperties &plugin_props = GetGlobalPluginProperties();
-  llvm::Expected<llvm::StringRef> CacheDirectoryPathOrErr =
+  llvm::Expected<llvm::StringRef> cache_path_or_err =
       plugin_props.GetCachePath();
   // A cache location is *required*.
-  if (!CacheDirectoryPathOrErr)
+  if (!cache_path_or_err)
     return {};
-  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
-  llvm::SmallVector<llvm::StringRef> DebuginfodUrls =
+  llvm::StringRef cache_path = *cache_path_or_err;
+  llvm::SmallVector<llvm::StringRef> debuginfod_urls =
       llvm::getDefaultDebuginfodUrls();
-  std::chrono::milliseconds Timeout = plugin_props.GetTimeout();
+  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);
+  std::string url_path = UrlBuilder(build_id);
+  std::string cache_key = llvm::getDebuginfodCacheKey(url_path);
   llvm::Expected<std::string> result = llvm::getCachedOrDownloadArtifact(
-      CacheKey, UrlPath, CacheDirectoryPath, DebuginfodUrls, Timeout);
+      cache_key, url_path, cache_path, debuginfod_urls, timeout);
   if (result)
     return FileSpec(*result);
   // An error here should be logged as a failure in the Debuginfod library,

>From 0b3b0f8caa0ba3b262a09e38dffbeefd0a272f5d Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 15:02:25 -0800
Subject: [PATCH 5/8] Log Debuginfod server failures in verbose mode

---
 .../Debuginfod/SymbolLocatorDebuginfod.cpp           | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 680167ebecd951..71503790fbb087 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -11,6 +11,8 @@
 #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"
@@ -173,9 +175,13 @@ GetFileForModule(const ModuleSpec &module_spec,
       cache_key, url_path, cache_path, debuginfod_urls, 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());
+
+  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 {};
 }
 

>From a7d3b1033237e7a097dc94a4ef78088ecfa8be7a Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 16:19:07 -0800
Subject: [PATCH 6/8] Switch from StringRef to std::string in
 PluginProps::GetCachePath

---
 .../Debuginfod/SymbolLocatorDebuginfod.cpp       | 16 ++++++----------
 llvm/include/llvm/Debuginfod/Debuginfod.h        |  9 +++++----
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 71503790fbb087..65bf3c531378f8 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -57,7 +57,7 @@ class PluginProperties : public Properties {
     return urls;
   }
 
-  llvm::Expected<llvm::StringRef> GetCachePath() {
+  llvm::Expected<std::string> GetCachePath() {
     OptionValueString *s =
         m_collection_sp->GetPropertyAtIndexAsOptionValueString(
             ePropertySymbolCachePath);
@@ -65,13 +65,11 @@ class PluginProperties : public Properties {
     if (!s || !s->GetCurrentValueAsRef().size()) {
       llvm::Expected<std::string> maybeCachePath =
           llvm::getDefaultDebuginfodCacheDirectory();
-      if (!maybeCachePath) {
+      if (!maybeCachePath)
         return maybeCachePath;
-      }
-      m_cache_path = *maybeCachePath;
-      return llvm::StringRef(m_cache_path);
+      return *maybeCachePath;
     }
-    return s->GetCurrentValueAsRef();
+    return s->GetCurrentValue();
   }
 
   std::chrono::milliseconds GetTimeout() const {
@@ -96,7 +94,6 @@ class PluginProperties : public Properties {
   }
   // Storage for the StringRef's used within the Debuginfod library.
   Args m_server_urls;
-  std::string m_cache_path;
 };
 
 } // namespace
@@ -157,12 +154,11 @@ GetFileForModule(const ModuleSpec &module_spec,
   // Grab LLDB's Debuginfod overrides from the
   // plugin.symbol-locator.debuginfod.* settings.
   PluginProperties &plugin_props = GetGlobalPluginProperties();
-  llvm::Expected<llvm::StringRef> cache_path_or_err =
-      plugin_props.GetCachePath();
+  llvm::Expected<std::string> cache_path_or_err = plugin_props.GetCachePath();
   // A cache location is *required*.
   if (!cache_path_or_err)
     return {};
-  llvm::StringRef cache_path = *cache_path_or_err;
+  std::string cache_path = *cache_path_or_err;
   llvm::SmallVector<llvm::StringRef> debuginfod_urls =
       llvm::getDefaultDebuginfodUrls();
   std::chrono::milliseconds timeout = plugin_props.GetTimeout();
diff --git a/llvm/include/llvm/Debuginfod/Debuginfod.h b/llvm/include/llvm/Debuginfod/Debuginfod.h
index 719bbf9cd0ccca..ef03948a706c06 100644
--- a/llvm/include/llvm/Debuginfod/Debuginfod.h
+++ b/llvm/include/llvm/Debuginfod/Debuginfod.h
@@ -46,7 +46,7 @@ bool canUseDebuginfod();
 /// environment variable.
 SmallVector<StringRef> getDefaultDebuginfodUrls();
 
-/// Creates the cache-key for a given Debuginfod UrlPath
+/// 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
@@ -61,7 +61,8 @@ 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.
+/// Get the full URL path for a source request of a given BuildID and file
+/// path.
 std::string getDebuginfodSourceUrlPath(object::BuildIDRef ID,
                                        StringRef SourceFilePath);
 
@@ -70,14 +71,14 @@ std::string getDebuginfodSourceUrlPath(object::BuildIDRef ID,
 Expected<std::string> getCachedOrDownloadSource(object::BuildIDRef ID,
                                                 StringRef SourceFilePath);
 
-/// Get the full UrlPath for an Executable request of a given BuildID.
+/// 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 UrlPath for a debug binary request of a given BuildID.
+/// 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

>From 45ebfe1ea58b2764ac12c0e22319f306b5f46275 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 16:50:09 -0800
Subject: [PATCH 7/8] Fixed log message

---
 .../SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp       | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 65bf3c531378f8..2cd7bbbb244902 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -175,8 +175,7 @@ GetFileForModule(const ModuleSpec &module_spec,
   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}",
+            "Debuginfod failed to download symbol artifact {0} with error {1}",
             url_path, err_message);
   return {};
 }

>From de0c17d22c9f03b89675c0cddaa7941ca45a4a98 Mon Sep 17 00:00:00 2001
From: Kevin Frei <freik at meta.com>
Date: Thu, 18 Jan 2024 16:56:48 -0800
Subject: [PATCH 8/8] Took JDevlieghere's wording for the timeout setting

---
 .../Debuginfod/SymbolLocatorDebuginfodProperties.td             | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
index 7f17faa8241aa7..0ff02674b8ea31 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
@@ -9,5 +9,5 @@ let Definition = "symbollocatordebuginfod" in {
     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).">;
+    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.">;
 }



More information about the llvm-commits mailing list