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

2024-01-21 Thread Kevin Frei via lldb-commits 

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

>From bf4fd0e1ce2236d94d15046c344d90c472368c98 Mon Sep 17 00:00:00 2001
From: Kevin Frei 
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 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 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 seconds =
+m_collection_sp->GetPropertyAtIndexAs(ePropertyTimeout);
+if (seconds && *seconds != 0) {
+  return std::chrono::duration_cast(
+  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 GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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 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 SymbolLocatorDebuginfod::LocateExecutableObjectFile(
 const ModuleSpec _spec) {
-  return GetFileForModule(module_spec, llvm::getCachedOrDownloadExecutable);
+  return GetFileForModule(module_spec,

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

2024-01-19 Thread Kevin Frei via lldb-commits 

kevinfrei wrote:

> 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

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-19 Thread Greg Clayton via lldb-commits 

https://github.com/clayborg commented:

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.

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Jonas Devlieghere via lldb-commits 


@@ -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).">;

JDevlieghere wrote:

I think this is rather weird: why not make 90 the default? If I specify zero 
I'd expect no timeout. 

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 


@@ -54,6 +55,34 @@ class PluginProperties : public Properties {
 return urls;
   }
 
+  llvm::Expected GetCachePath() {

kevinfrei wrote:

D'oh. You're right. I'll hoist that StringRef up and switch the function to 
return a string. Good catch!

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 


@@ -112,31 +142,49 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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 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());

kevinfrei wrote:

LLDB_LOGV (LLDBLog::Symbols context) seems like the right choice, given how 
noisy it will be.

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 


@@ -112,31 +142,49 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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

kevinfrei wrote:

Modified to describe the 'interesting part'.

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Jonas Devlieghere via lldb-commits 


@@ -112,31 +142,49 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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);

JDevlieghere wrote:

Yeah, LLDB diverges from LLVM's coding style in this: 
https://lldb.llvm.org/resources/contributing.html#coding-style. it should use 
snake_case everywhere. 

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 


@@ -112,31 +142,49 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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);

kevinfrei wrote:

ditto from above.

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 


@@ -112,31 +142,49 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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);

kevinfrei wrote:

What's the style guidance for locals? The locals above this are filled with 
PascalCaseOrErr everywhere. I'm happy to change the entire function, but right 
now it's really inconsistent (and kinda ugly). I'd like to make it consistent, 
either snake or pascal, based on your guidance, here. Should I just snake them 
all (cache_directory_path_or_err?) or Pascal them all (BuildId), or is there 
some rule I'm missing that makes the mixture sensible?

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 


@@ -54,6 +55,34 @@ class PluginProperties : public Properties {
 return urls;
   }
 
+  llvm::Expected GetCachePath() {

kevinfrei wrote:

String storage lives 40 lines lower (line 97) because, yeah, this bit me. The 
Debuginfod library takes a StringRef, and I found this was the most consistent 
way to deal with it. If you'd prefer me to switch this and cons up a StringRef 
before the API, I'm okay with that (just let me know in here)

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Jonas Devlieghere via lldb-commits 


@@ -112,31 +142,49 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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

JDevlieghere wrote:

LLVM's code style requires a period at the end of comments. Same across the 
patch. 

https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread Kevin Frei via lldb-commits 

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

>From b46553c6fe17a50dc2072544e46b7a1dde127436 Mon Sep 17 00:00:00 2001
From: Kevin Frei 
Date: Thu, 18 Jan 2024 09:09:50 -0800
Subject: [PATCH 1/2] 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 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 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 seconds =
+m_collection_sp->GetPropertyAtIndexAs(ePropertyTimeout);
+if (seconds && *seconds != 0) {
+  return std::chrono::duration_cast(
+  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 GetFileForModule(
-const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
++ static std::optional +
+GetFileForModule(
+const ModuleSpec _spec,
+std::function UrlBuilder) {
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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 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 SymbolLocatorDebuginfod::LocateExecutableObjectFile(
 const ModuleSpec _spec) {
-  return GetFileForModule(module_spec, llvm::getCachedOrDownloadExecutable);
+  return GetFileForModule(module_spec,

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

2024-01-18 Thread via lldb-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff f2b5a314b29275f2092af3ec26f42272daa4312c 
e565965c7395b6431b9442c2e6b5184c60501439 -- 
lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp 
llvm/include/llvm/Debuginfod/Debuginfod.h llvm/lib/Debuginfod/Debuginfod.cpp
``





View the diff from clang-format here.


``diff
diff --git 
a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp 
b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 2a61827162..afbc7d8b12 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -142,8 +142,8 @@ SymbolLocator *SymbolLocatorDebuginfod::CreateInstance() {
   return new SymbolLocatorDebuginfod();
 }
 
-static std::optional GetFileForModule(
-const ModuleSpec _spec,
+static std::optional
+GetFileForModule(const ModuleSpec _spec,
  std::function UrlBuilder) 
{
   const UUID _uuid = module_spec.GetUUID();
   // Don't bother if we don't have a valid UUID, Debuginfod isn't available,

``




https://github.com/llvm/llvm-project/pull/78605
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2024-01-18 Thread via lldb-commits 

llvmbot wrote:



@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
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 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 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 seconds =
+m_collection_sp->GetPropertyAtIndexAs(ePropertyTimeout);
+if (seconds && *seconds != 0) {
+  return std::chrono::duration_cast(
+  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 GetFileForModule(
 const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
+ std::function UrlBuilder) 
{
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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 result = llvm::getCachedOrDownloadArtifact(
+  CacheKey,

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

2024-01-18 Thread Kevin Frei via lldb-commits 

https://github.com/kevinfrei created 
https://github.com/llvm/llvm-project/pull/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).

>From e565965c7395b6431b9442c2e6b5184c60501439 Mon Sep 17 00:00:00 2001
From: Kevin Frei 
Date: Thu, 18 Jan 2024 09:09:50 -0800
Subject: [PATCH] Added settings for cache location and timeout

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

diff --git 
a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp 
b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index 111be6be365240..2a618271624a1b 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 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 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 seconds =
+m_collection_sp->GetPropertyAtIndexAs(ePropertyTimeout);
+if (seconds && *seconds != 0) {
+  return std::chrono::duration_cast(
+  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 GetFileForModule(
 const ModuleSpec _spec,
-std::function(llvm::object::BuildIDRef)>
-PullFromServer) {
-  if (!ModuleList::GetGlobalModuleListProperties().GetEnableExternalLookup())
-return {};
+ std::function UrlBuilder) 
{
   const UUID _uuid = module_spec.GetUUID();
-  if (module_uuid.IsValid() && llvm::canUseDebuginfod()) {
-llvm::object::BuildID build_id(module_uuid.GetBytes());
-llvm::Expected 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 _props = GetGlobalPluginProperties();
+  llvm::Expected CacheDirectoryPathOrErr =
+  plugin_props.GetCachePath();
+  // A cache location is *required*
+  if (!CacheDirectoryPathOrErr)
+return {};
+  llvm::StringRef CacheDirectoryPath = *CacheDirectoryPathOrErr;
+  llvm::SmallVector 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 result =