Cleaning up changes from a couple months back.

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

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<SymbolLookupMode>(
+        idx, static_cast<SymbolLookupMode>(
+                 g_debuginfod_symbol_lookup_mode[idx].value));
+  }
+
   llvm::Expected<std::string> 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<FileSpec>
 GetFileForModule(const ModuleSpec &module_spec,
-                 std::function<std::string(llvm::object::BuildID)> UrlBuilder) {
+                 std::function<std::string(llvm::object::BuildID)> 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<llvm::StringRef> 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<std::string> result = llvm::getCachedOrDownloadArtifact(
-      cache_key, url_path, cache_path, debuginfod_urls, timeout);
+  bool ask_server = sync_lookup || plugin_props.GetLookupMode() ==
+                                       SymbolLookupMode::eLookupModeAlways;
+  llvm::Expected<std::string> 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<ModuleSpec> SymbolLocatorDebuginfod::LocateExecutableObjectFile(
     const ModuleSpec &module_spec) {
-  return GetFileForModule(module_spec, llvm::getDebuginfodExecutableUrlPath);
+  return GetFileForModule(module_spec, llvm::getDebuginfodExecutableUrlPath,
+                          false);
 }
 
 std::optional<FileSpec> 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<FileSpec> SymbolFile = GetFileForModule(
+        module_spec, llvm::getDebuginfodDebuginfoUrlPath, sync_lookup);
+    if (SymbolFile) {
+      module_spec.GetSymbolFileSpec() = *SymbolFile;
+      found = true;
+    }
+  }
+
+  if (!module_spec.GetFileSpec()) {
+    std::optional<FileSpec> ExecutableFile = GetFileForModule(
+        module_spec, llvm::getDebuginfodExecutableUrlPath, sync_lookup);
+    if (ExecutableFile) {
+      module_spec.GetFileSpec() = *ExecutableFile;
+      found = true;
+    }
+  }
+  return found;
 }

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

@@ -47,6 +47,14 @@ class SymbolLocatorDebuginfod : public SymbolLocator {
   static std::optional<FileSpec>
   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

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

lldb/source/Symbol/SymbolLocator.cpp

@@ -18,9 +18,14 @@
 using namespace lldb;
 using namespace lldb_private;
 
+namespace {
+
+llvm::SmallSet<UUID, 8> g_seen_uuids;
+static std::mutex g_mutex;
+
+} // namespace
+
 void SymbolLocator::DownloadSymbolFileAsync(const UUID &uuid) {
-  static llvm::SmallSet<UUID, 8> 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<std::mutex> guard(g_mutex);
+  g_seen_uuids.clear();
+}

llvm/include/llvm/Debuginfod/Debuginfod.h

@@ -85,6 +85,10 @@ std::string getDebuginfodDebuginfoUrlPath(object::BuildIDRef ID);
 /// server URLs.
 Expected<std::string> getCachedOrDownloadDebuginfo(object::BuildIDRef ID);
 
+/// Fetches any debuginfod artifact from the specified cache directory and key.
+Expected<std::string> getCachedArtifact(StringRef UniqueKey,
+                                        StringRef CacheDirectoryPath);
+
 /// Fetches any debuginfod artifact using the default local cache directory and
 /// server URLs.
 Expected<std::string> getCachedOrDownloadArtifact(StringRef UniqueKey,

llvm/lib/Debuginfod/Debuginfod.cpp

@@ -248,27 +248,54 @@ static SmallVector<std::string, 0> getHeaders() {
   return Headers;
 }
 
-Expected<std::string> getCachedOrDownloadArtifact(
-    StringRef UniqueKey, StringRef UrlPath, StringRef CacheDirectoryPath,
-    ArrayRef<StringRef> 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<AddStreamFn>
+getCachedArtifactHelper(StringRef UniqueKey, StringRef CacheDirectoryPath,
+                        unsigned &Task) {
+  SmallString<64> AbsCachedArtifactPath =
+      getCachedArtifactPath(UniqueKey, CacheDirectoryPath);
 
   Expected<FileCache> CacheOrErr =
       localCache("Debuginfod-client", ".debuginfod-client", CacheDirectoryPath);
   if (!CacheOrErr)
     return CacheOrErr.takeError();
+  return (*CacheOrErr)(Task, UniqueKey, "");
+}
+
+Expected<std::string> getCachedArtifact(StringRef UniqueKey,
+                                        StringRef CacheDirectoryPath) {
+  // We choose an arbitrary Task parameter as we do not make use of it.
+  unsigned Task = 0;
+  Expected<AddStreamFn> 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<std::string> getCachedOrDownloadArtifact(
+    StringRef UniqueKey, StringRef UrlPath, StringRef CacheDirectoryPath,
+    ArrayRef<StringRef> DebuginfodUrls, std::chrono::milliseconds Timeout) {
   // We choose an arbitrary Task parameter as we do not make use of it.
   unsigned Task = 0;
-  Expected<AddStreamFn> CacheAddStreamOrErr = Cache(Task, UniqueKey, "");
+  Expected<AddStreamFn> 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<std::string> 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");