[lldb] Implement priority system for symbol locator plugin #144406
Conversation
|
@llvm/pr-subscribers-lldb Author: None (GeorgeHuyubo) ChangesIn the context if locate_module callback is registered and debuginfod is enabled. To fix this, we want to a way to define the priority of a plugin, so that we can adjust the order of each plugin gets called at runtime. Debuginfod plugin is prioritized Default plugin is prioritized, local debuginfo is used. Full diff: https://github.com/llvm/llvm-project/pull/144406.diff 6 Files Affected:
diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h
index e7b1691031111..021f90f54abff 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -431,7 +431,8 @@ class PluginManager {
SymbolLocatorDownloadObjectAndSymbolFile download_object_symbol_file =
nullptr,
SymbolLocatorFindSymbolFileInBundle find_symbol_file_in_bundle = nullptr,
- DebuggerInitializeCallback debugger_init_callback = nullptr);
+ DebuggerInitializeCallback debugger_init_callback = nullptr,
+ SymbolLocatorGetPriority get_priority_callback = nullptr);
static bool UnregisterPlugin(SymbolLocatorCreateInstance create_callback);
diff --git a/lldb/include/lldb/Symbol/SymbolLocator.h b/lldb/include/lldb/Symbol/SymbolLocator.h
index 8d2f26b3d0cca..0eb9d34cb1cc6 100644
--- a/lldb/include/lldb/Symbol/SymbolLocator.h
+++ b/lldb/include/lldb/Symbol/SymbolLocator.h
@@ -14,6 +14,9 @@
namespace lldb_private {
+// Default priority for symbol locator plugins
+static constexpr uint32_t kDefaultSymbolLocatorPriority = 2;
+
class SymbolLocator : public PluginInterface {
public:
SymbolLocator() = default;
diff --git a/lldb/include/lldb/lldb-private-interfaces.h b/lldb/include/lldb/lldb-private-interfaces.h
index d366dbd1d7832..0fe59e28adaa6 100644
--- a/lldb/include/lldb/lldb-private-interfaces.h
+++ b/lldb/include/lldb/lldb-private-interfaces.h
@@ -100,6 +100,7 @@ typedef std::optional<FileSpec> (*SymbolLocatorLocateExecutableSymbolFile)(
typedef bool (*SymbolLocatorDownloadObjectAndSymbolFile)(
ModuleSpec &module_spec, Status &error, bool force_lookup,
bool copy_executable);
+typedef uint64_t (*SymbolLocatorGetPriority)();
using BreakpointHitCallback =
std::function<bool(void *baton, StoppointCallbackContext *context,
lldb::user_id_t break_id, lldb::user_id_t break_loc_id)>;
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index 5d44434033c55..fedfd3f1e7b6f 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -13,6 +13,7 @@
#include "lldb/Host/HostInfo.h"
#include "lldb/Interpreter/OptionValueProperties.h"
#include "lldb/Symbol/SaveCoreOptions.h"
+#include "lldb/Symbol/SymbolLocator.h"
#include "lldb/Target/Process.h"
#include "lldb/Utility/FileSpec.h"
#include "lldb/Utility/Status.h"
@@ -25,6 +26,7 @@
#include <map>
#include <memory>
#include <mutex>
+#include <queue>
#include <string>
#include <utility>
#include <vector>
@@ -323,6 +325,29 @@ template <typename Instance> class PluginInstances {
return enabled_instances;
}
+ // Return a priority queue of all enabled instances, ordered by priority
+ // (lower priority values = higher priority in queue)
+ template <typename PriorityGetter>
+ std::priority_queue<Instance, std::vector<Instance>,
+ std::function<bool(const Instance &, const Instance &)>>
+ GetPriorityQueue(PriorityGetter get_priority) {
+ // Create comparator that orders by priority (lower values = higher
+ // priority)
+ auto comparator = [get_priority](const Instance &a, const Instance &b) {
+ return get_priority(a) > get_priority(b); // Reverse for min-heap behavior
+ };
+
+ std::priority_queue<Instance, std::vector<Instance>,
+ std::function<bool(const Instance &, const Instance &)>>
+ pq(comparator);
+
+ for (const auto &instance : m_instances) {
+ if (instance.enabled)
+ pq.push(instance);
+ }
+ return pq;
+ }
+
const Instance *GetInstanceAtIndex(uint32_t idx) {
uint32_t count = 0;
@@ -1223,18 +1248,26 @@ struct SymbolLocatorInstance
SymbolLocatorLocateExecutableSymbolFile locate_executable_symbol_file,
SymbolLocatorDownloadObjectAndSymbolFile download_object_symbol_file,
SymbolLocatorFindSymbolFileInBundle find_symbol_file_in_bundle,
- DebuggerInitializeCallback debugger_init_callback)
+ DebuggerInitializeCallback debugger_init_callback,
+ SymbolLocatorGetPriority get_priority_callback)
: PluginInstance<SymbolLocatorCreateInstance>(
name, description, create_callback, debugger_init_callback),
locate_executable_object_file(locate_executable_object_file),
locate_executable_symbol_file(locate_executable_symbol_file),
download_object_symbol_file(download_object_symbol_file),
- find_symbol_file_in_bundle(find_symbol_file_in_bundle) {}
+ find_symbol_file_in_bundle(find_symbol_file_in_bundle),
+ get_priority_callback(get_priority_callback) {}
+
+ uint64_t GetPriority() const {
+ return get_priority_callback ? get_priority_callback()
+ : kDefaultSymbolLocatorPriority;
+ }
SymbolLocatorLocateExecutableObjectFile locate_executable_object_file;
SymbolLocatorLocateExecutableSymbolFile locate_executable_symbol_file;
SymbolLocatorDownloadObjectAndSymbolFile download_object_symbol_file;
SymbolLocatorFindSymbolFileInBundle find_symbol_file_in_bundle;
+ SymbolLocatorGetPriority get_priority_callback;
};
typedef PluginInstances<SymbolLocatorInstance> SymbolLocatorInstances;
@@ -1250,11 +1283,13 @@ bool PluginManager::RegisterPlugin(
SymbolLocatorLocateExecutableSymbolFile locate_executable_symbol_file,
SymbolLocatorDownloadObjectAndSymbolFile download_object_symbol_file,
SymbolLocatorFindSymbolFileInBundle find_symbol_file_in_bundle,
- DebuggerInitializeCallback debugger_init_callback) {
+ DebuggerInitializeCallback debugger_init_callback,
+ SymbolLocatorGetPriority get_priority_callback) {
return GetSymbolLocatorInstances().RegisterPlugin(
name, description, create_callback, locate_executable_object_file,
locate_executable_symbol_file, download_object_symbol_file,
- find_symbol_file_in_bundle, debugger_init_callback);
+ find_symbol_file_in_bundle, debugger_init_callback,
+ get_priority_callback);
}
bool PluginManager::UnregisterPlugin(
@@ -1270,8 +1305,13 @@ PluginManager::GetSymbolLocatorCreateCallbackAtIndex(uint32_t idx) {
ModuleSpec
PluginManager::LocateExecutableObjectFile(const ModuleSpec &module_spec,
StatisticsMap &map) {
- auto instances = GetSymbolLocatorInstances().GetSnapshot();
- for (auto &instance : instances) {
+ auto pq = GetSymbolLocatorInstances().GetPriorityQueue(
+ [](const auto &instance) { return instance.GetPriority(); });
+
+ while (!pq.empty()) {
+ auto instance = pq.top();
+ pq.pop();
+
if (instance.locate_executable_object_file) {
StatsDuration time;
std::optional<ModuleSpec> result;
@@ -1290,8 +1330,12 @@ PluginManager::LocateExecutableObjectFile(const ModuleSpec &module_spec,
FileSpec PluginManager::LocateExecutableSymbolFile(
const ModuleSpec &module_spec, const FileSpecList &default_search_paths,
StatisticsMap &map) {
- auto instances = GetSymbolLocatorInstances().GetSnapshot();
- for (auto &instance : instances) {
+ auto pq = GetSymbolLocatorInstances().GetPriorityQueue(
+ [](const auto &instance) { return instance.GetPriority(); });
+
+ while (!pq.empty()) {
+ auto instance = pq.top();
+ pq.pop();
if (instance.locate_executable_symbol_file) {
StatsDuration time;
std::optional<FileSpec> result;
diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
index f9aa6b1a98765..3e2d863d0d20e 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
@@ -83,6 +83,16 @@ class PluginProperties : public Properties {
}
}
+ uint32_t GetPriority() const {
+ std::optional<uint32_t> priority =
+ m_collection_sp->GetPropertyAtIndexAs<uint64_t>(ePropertyPriority);
+ if (priority && *priority > 0) {
+ return priority.value();
+ } else {
+ return kDefaultSymbolLocatorPriority;
+ }
+ }
+
private:
void ServerURLsChangedCallback() {
m_server_urls = GetDebugInfoDURLs();
@@ -103,6 +113,13 @@ static PluginProperties &GetGlobalPluginProperties() {
return g_settings;
}
+static uint64_t GetDebuginfodPriority() {
+ // Grab LLDB's Debuginfod overrides from the
+ // plugin.symbol-locator.debuginfod.* settings.
+ PluginProperties &plugin_props = GetGlobalPluginProperties();
+ return plugin_props.GetPriority();
+}
+
SymbolLocatorDebuginfod::SymbolLocatorDebuginfod() : SymbolLocator() {}
void SymbolLocatorDebuginfod::Initialize() {
@@ -112,7 +129,8 @@ void SymbolLocatorDebuginfod::Initialize() {
PluginManager::RegisterPlugin(
GetPluginNameStatic(), GetPluginDescriptionStatic(), CreateInstance,
LocateExecutableObjectFile, LocateExecutableSymbolFile, nullptr,
- nullptr, SymbolLocatorDebuginfod::DebuggerInitialize);
+ nullptr, SymbolLocatorDebuginfod::DebuggerInitialize,
+ GetDebuginfodPriority);
llvm::HTTPClient::initialize();
});
}
diff --git a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
index 0ff02674b8ea3..32389f8808b6f 100644
--- a/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
+++ b/lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfodProperties.td
@@ -10,4 +10,7 @@ let Definition = "symbollocatordebuginfod" in {
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.">;
+ def Priority : Property<"priority", "UInt64">,
+ DefaultUnsignedValue<2>,
+ Desc<"The priority order for this symbol locator plugin when multiple symbol locators are available. Lower values indicate higher priority. The default symbol locator priority is 2">;
}
|
lldb/source/Plugins/SymbolLocator/Debuginfod/SymbolLocatorDebuginfod.cpp
Outdated
Show resolved
Hide resolved
|
I don't yet understand how changing the priority of the plugins prevents the "double download" you mention. Your problem statements talks about the callback and the debuginfod plugin, but your solution talks about the debuginfod plugin and the default symbol locator. What confuses me is that In #99362, the order of the plugins was changed to prioritize the debuginfod plugin for stripped binaries. How does this interact with that? Will this go back to doing the wrong thing is the priority is too low? Finally, this PR only supports setting the priority of the debuginfod symbol locator. That only works if you have exactly two plugins. As soon as you have more (as is the case on Darwin, and possibly soon we'll have a fourth one) that mechanism falls short. In hindsight, I don't think the |
|
I like the "Scripted Symbol Locator" idea, it should solve all the problem I have. |
So I believe the locate_module callback is designed for user to do something like: ...after the callback function finish downloading the symbol. This means that we still rely on the Default symbol locator(which should simply look for symbol locally by the dir&filename) to actually locate the symbol file. But if debuginfod is also enabled and called first, debuginfod plugin will ignore the local symbol file(even if it exist), but query the debuginfod server by the gnu_build_id, and do a double downloading. |
In the context if locate_module callback is registered and debuginfod is enabled.
If debuginfod symbol locator is called first then we are double downloading the debuginfo file through locate module callback and debuginfod.
To fix this, we want to a way to define the priority of a plugin, so that we can adjust the order of each plugin gets called at runtime.
Debuginfod plugin is prioritized
Default plugin is prioritized, local debuginfo is used.