Add enable/disable api for SystemRuntime plugins #133794
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds support for enabling and disabling plugins by name.
The changes are made generically in the `PluginInstances` class, but
currently we only expose the ability to SystemRuntime plugins. Other
plugins types can be added easily.
We had a few design goals for how disabled plugins should work
1. Plugins that are disabled should still be visible to the system.
This allows us to dynamically enable and disable plugins
and report their state to the user.
2. Plugin order should be stable across disable and enable changes.
We want avoid changing the order of plugin lookup. When a plugin
is re-enabled it should return to its original slot in the
creation order.
3. Disabled plugins should not appear in PluginManager operations.
Clients should be able to assume that only enabled plugins will
be returned from the PluginManager.
For the implementation we modify the plugin instance to maintain a bool
of its enabled state. Existing clients external to the Instances class
expect to iterate over only enabled instance so we skip over disabed
instances in the query and snapshot apis. This way the client does not
have to manually check which instances are enabled.
|
@llvm/pr-subscribers-lldb Author: David Peixotto (dmpots) ChangesThis commit adds support for enabling and disabling plugins by name. The changes are made generically in the We had a few design goals for how disabled plugins should work
For the implementation we modify the plugin instance to maintain a bool of its enabled state. Existing clients external to the Instances class expect to iterate over only enabled instance so we skip over disabed instances in the query and snapshot apis. This way the client does not have to manually check which instances are enabled. Patch is 21.13 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133794.diff 4 Files Affected:
diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h
index e4e0c3eea67f8..a6dab045adf27 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -22,6 +22,7 @@
#include <cstddef>
#include <cstdint>
+#include <vector>
#define LLDB_PLUGIN_DEFINE_ADV(ClassName, PluginName) \
namespace lldb_private { \
@@ -47,6 +48,12 @@ class CommandInterpreter;
class Debugger;
class StringList;
+struct RegisteredPluginInfo {
+ llvm::StringRef name = "";
+ llvm::StringRef description = "";
+ bool enabled = false;
+};
+
class PluginManager {
public:
static void Initialize();
@@ -168,6 +175,12 @@ class PluginManager {
static SystemRuntimeCreateInstance
GetSystemRuntimeCreateCallbackAtIndex(uint32_t idx);
+ static std::vector<RegisteredPluginInfo> GetSystemRuntimePluginInfo();
+
+ // Modify the enabled state of a SystemRuntime plugin.
+ // Returns false if the plugin name is not found.
+ static bool SetSystemRuntimePluginEnabled(llvm::StringRef name, bool enabled);
+
// ObjectFile
static bool
RegisterPlugin(llvm::StringRef name, llvm::StringRef description,
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index 95eb940efcef2..e6cb248ef31ce 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -188,11 +188,13 @@ template <typename Callback> struct PluginInstance {
PluginInstance(llvm::StringRef name, llvm::StringRef description,
Callback create_callback,
DebuggerInitializeCallback debugger_init_callback = nullptr)
- : name(name), description(description), create_callback(create_callback),
+ : name(name), description(description), enabled(true),
+ create_callback(create_callback),
debugger_init_callback(debugger_init_callback) {}
llvm::StringRef name;
llvm::StringRef description;
+ bool enabled;
Callback create_callback;
DebuggerInitializeCallback debugger_init_callback;
};
@@ -250,7 +252,9 @@ template <typename Instance> class PluginInstances {
}
void PerformDebuggerCallback(Debugger &debugger) {
- for (auto &instance : m_instances) {
+ for (const auto &instance : m_instances) {
+ if (!instance.enabled)
+ continue;
if (instance.debugger_init_callback)
instance.debugger_init_callback(debugger);
}
@@ -260,7 +264,14 @@ template <typename Instance> class PluginInstances {
// Note that this is a copy of the internal state so modifications
// to the returned instances will not be reflected back to instances
// stored by the PluginInstances object.
- std::vector<Instance> GetSnapshot() { return m_instances; }
+ std::vector<Instance> GetSnapshot() {
+ std::vector<Instance> enabled_instances;
+ for (const auto &instance : m_instances) {
+ if (instance.enabled)
+ enabled_instances.push_back(instance);
+ }
+ return enabled_instances;
+ }
const Instance *GetInstanceAtIndex(uint32_t idx) {
uint32_t count = 0;
@@ -280,12 +291,41 @@ template <typename Instance> class PluginInstances {
const Instance *
FindEnabledInstance(std::function<bool(const Instance &)> predicate) const {
for (const auto &instance : m_instances) {
+ if (!instance.enabled)
+ continue;
if (predicate(instance))
return &instance;
}
return nullptr;
}
+ // Return a list of all the registered plugin instances. This includes both
+ // enabled and disabled instances. The instances are listed in the order they
+ // were registered which is the order they would be queried if they were all
+ // enabled.
+ std::vector<RegisteredPluginInfo> GetPluginInfoForAllInstances() {
+ // Lookup the plugin info for each instance in the sorted order.
+ std::vector<RegisteredPluginInfo> plugin_infos;
+ plugin_infos.reserve(m_instances.size());
+ for (const Instance &instance : m_instances)
+ plugin_infos.push_back(
+ {instance.name, instance.description, instance.enabled});
+
+ return plugin_infos;
+ }
+
+ bool SetInstanceEnabled(llvm::StringRef name, bool enable) {
+ auto it = std::find_if(
+ m_instances.begin(), m_instances.end(),
+ [&](const Instance &instance) { return instance.name == name; });
+
+ if (it == m_instances.end())
+ return false;
+
+ it->enabled = enable;
+ return true;
+ }
+
private:
std::vector<Instance> m_instances;
};
@@ -627,6 +667,15 @@ PluginManager::GetSystemRuntimeCreateCallbackAtIndex(uint32_t idx) {
return GetSystemRuntimeInstances().GetCallbackAtIndex(idx);
}
+std::vector<RegisteredPluginInfo> PluginManager::GetSystemRuntimePluginInfo() {
+ return GetSystemRuntimeInstances().GetPluginInfoForAllInstances();
+}
+
+bool PluginManager::SetSystemRuntimePluginEnabled(llvm::StringRef name,
+ bool enable) {
+ return GetSystemRuntimeInstances().SetInstanceEnabled(name, enable);
+}
+
#pragma mark ObjectFile
struct ObjectFileInstance : public PluginInstance<ObjectFileCreateInstance> {
diff --git a/lldb/unittests/Core/CMakeLists.txt b/lldb/unittests/Core/CMakeLists.txt
index 60265f794b5e8..8580f5887ea2b 100644
--- a/lldb/unittests/Core/CMakeLists.txt
+++ b/lldb/unittests/Core/CMakeLists.txt
@@ -7,6 +7,7 @@ add_lldb_unittest(LLDBCoreTests
FormatEntityTest.cpp
MangledTest.cpp
ModuleSpecTest.cpp
+ PluginManagerTest.cpp
ProgressReportTest.cpp
RichManglingContextTest.cpp
SourceLocationSpecTest.cpp
diff --git a/lldb/unittests/Core/PluginManagerTest.cpp b/lldb/unittests/Core/PluginManagerTest.cpp
new file mode 100644
index 0000000000000..ca1003ca9a85a
--- /dev/null
+++ b/lldb/unittests/Core/PluginManagerTest.cpp
@@ -0,0 +1,367 @@
+
+#include "lldb/Core/PluginManager.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+// Mock system runtime plugin create functions.
+SystemRuntime *CreateSystemRuntimePluginA(Process *process) { return nullptr; }
+
+SystemRuntime *CreateSystemRuntimePluginB(Process *process) { return nullptr; }
+
+SystemRuntime *CreateSystemRuntimePluginC(Process *process) { return nullptr; }
+
+// Test class for testing the PluginManager.
+// The PluginManager modifies global state when registering new plugins. This
+// class is intended to undo those modifications in the destructor to give each
+// test a clean slate with no registered plugins at the start of a test.
+class PluginManagerTest : public testing::Test {
+public:
+ // Remove any pre-registered plugins so we have a known starting point.
+ static void SetUpTestSuite() { RemoveAllRegisteredSystemRuntimePlugins(); }
+
+ // Add mock system runtime plugins for testing.
+ void RegisterMockSystemRuntimePlugins() {
+ ASSERT_TRUE(PluginManager::RegisterPlugin("a", "test instance A",
+ CreateSystemRuntimePluginA));
+ ASSERT_TRUE(PluginManager::RegisterPlugin("b", "test instance B",
+ CreateSystemRuntimePluginB));
+ ASSERT_TRUE(PluginManager::RegisterPlugin("c", "test instance C",
+ CreateSystemRuntimePluginC));
+ }
+
+ // Remove any plugins added during the tests.
+ virtual ~PluginManagerTest() override {
+ RemoveAllRegisteredSystemRuntimePlugins();
+ }
+
+protected:
+ std::vector<SystemRuntimeCreateInstance> m_system_runtime_plugins;
+
+ static void RemoveAllRegisteredSystemRuntimePlugins() {
+ // Enable all currently registered plugins so we can get a handle to
+ // their create callbacks in the loop below. Only enabled plugins
+ // are returned from the PluginManager Get*CreateCallbackAtIndex apis.
+ for (const RegisteredPluginInfo &PluginInfo :
+ PluginManager::GetSystemRuntimePluginInfo()) {
+ PluginManager::SetSystemRuntimePluginEnabled(PluginInfo.name, true);
+ }
+
+ // Get a handle to the create call backs for all the registered plugins.
+ std::vector<SystemRuntimeCreateInstance> registered_plugin_callbacks;
+ SystemRuntimeCreateInstance create_callback = nullptr;
+ for (uint32_t idx = 0;
+ (create_callback =
+ PluginManager::GetSystemRuntimeCreateCallbackAtIndex(idx)) !=
+ nullptr;
+ ++idx) {
+ registered_plugin_callbacks.push_back((create_callback));
+ }
+
+ // Remove all currently registered plugins.
+ for (SystemRuntimeCreateInstance create_callback :
+ registered_plugin_callbacks) {
+ PluginManager::UnregisterPlugin(create_callback);
+ }
+ }
+};
+
+// Test basic register functionality.
+TEST_F(PluginManagerTest, RegisterSystemRuntimePlugin) {
+ RegisterMockSystemRuntimePlugins();
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginB);
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2),
+ CreateSystemRuntimePluginC);
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(3), nullptr);
+}
+
+// Test basic un-register functionality.
+TEST_F(PluginManagerTest, UnRegisterSystemRuntimePlugin) {
+ RegisterMockSystemRuntimePlugins();
+
+ ASSERT_TRUE(PluginManager::UnregisterPlugin(CreateSystemRuntimePluginB));
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginC);
+
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2), nullptr);
+}
+
+// Test registered plugin info functionality.
+TEST_F(PluginManagerTest, SystemRuntimePluginInfo) {
+ RegisterMockSystemRuntimePlugins();
+
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 3u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[0].description, "test instance A");
+ ASSERT_EQ(plugin_info[0].enabled, true);
+ ASSERT_EQ(plugin_info[1].name, "b");
+ ASSERT_EQ(plugin_info[1].description, "test instance B");
+ ASSERT_EQ(plugin_info[1].enabled, true);
+ ASSERT_EQ(plugin_info[2].name, "c");
+ ASSERT_EQ(plugin_info[2].description, "test instance C");
+ ASSERT_EQ(plugin_info[2].enabled, true);
+}
+
+// Test basic un-register functionality.
+TEST_F(PluginManagerTest, UnRegisterSystemRuntimePluginInfo) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Initial plugin info has all three registered plugins.
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 3u);
+
+ ASSERT_TRUE(PluginManager::UnregisterPlugin(CreateSystemRuntimePluginB));
+
+ // After un-registering a plugin it should be removed from plugin info.
+ plugin_info = PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 2u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[0].enabled, true);
+ ASSERT_EQ(plugin_info[1].name, "c");
+ ASSERT_EQ(plugin_info[1].enabled, true);
+}
+
+// Test plugin disable functionality.
+TEST_F(PluginManagerTest, SystemRuntimePluginDisable) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Disable plugin should succeed.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", false));
+
+ // Disabling a plugin does not remove it from plugin info.
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 3u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[0].enabled, true);
+ ASSERT_EQ(plugin_info[1].name, "b");
+ ASSERT_EQ(plugin_info[1].enabled, false);
+ ASSERT_EQ(plugin_info[2].name, "c");
+ ASSERT_EQ(plugin_info[2].enabled, true);
+
+ // Disabling a plugin does remove it from available plugins.
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginC);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2), nullptr);
+}
+
+// Test plugin disable and enable functionality.
+TEST_F(PluginManagerTest, SystemRuntimePluginDisableThenEnable) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Initially plugin b is available in slot 1.
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginB);
+
+ // Disabling it will remove it from available plugins.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", false));
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginC);
+
+ // We can re-enable the plugin later and it should go back to the original
+ // slot.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", true));
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginB);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2),
+ CreateSystemRuntimePluginC);
+
+ // And show up in the plugin info correctly.
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 3u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[0].enabled, true);
+ ASSERT_EQ(plugin_info[1].name, "b");
+ ASSERT_EQ(plugin_info[1].enabled, true);
+ ASSERT_EQ(plugin_info[2].name, "c");
+ ASSERT_EQ(plugin_info[2].enabled, true);
+}
+
+// Test calling disable on an already disabled plugin is ok.
+TEST_F(PluginManagerTest, SystemRuntimePluginDisableDisabled) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Initial call to disable the plugin should succeed.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", false));
+
+ // The second call should also succeed because the plugin is already disabled.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", false));
+
+ // The call to re-enable the plugin should succeed.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", true));
+
+ // The second call should also succeed since the plugin is already enabled.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", true));
+}
+
+// Test calling disable on an already disabled plugin is ok.
+TEST_F(PluginManagerTest, SystemRuntimePluginDisableNonExistent) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Both enable and disable should return false for a non-existent plugin.
+ ASSERT_FALSE(
+ PluginManager::SetSystemRuntimePluginEnabled("does_not_exist", true));
+ ASSERT_FALSE(
+ PluginManager::SetSystemRuntimePluginEnabled("does_not_exist", false));
+}
+
+// Test disabling all plugins and then re-enabling them in a different
+// order will restore the original plugin order.
+TEST_F(PluginManagerTest, SystemRuntimePluginDisableAll) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Validate initial state of registered plugins.
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginB);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2),
+ CreateSystemRuntimePluginC);
+
+ // Disable all the active plugins.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("a", false));
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", false));
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("c", false));
+
+ // Should have no active plugins.
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0), nullptr);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1), nullptr);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2), nullptr);
+
+ // And show up in the plugin info correctly.
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 3u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[0].enabled, false);
+ ASSERT_EQ(plugin_info[1].name, "b");
+ ASSERT_EQ(plugin_info[1].enabled, false);
+ ASSERT_EQ(plugin_info[2].name, "c");
+ ASSERT_EQ(plugin_info[2].enabled, false);
+
+ // Enable plugins in reverse order and validate expected indicies.
+ // They should show up in the original plugin order.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("c", true));
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginC);
+
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("a", true));
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginC);
+
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", true));
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginB);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2),
+ CreateSystemRuntimePluginC);
+}
+
+// Test un-registering a disabled plugin works.
+TEST_F(PluginManagerTest, UnRegisterDisabledSystemRuntimePlugin) {
+ RegisterMockSystemRuntimePlugins();
+
+ // Initial plugin info has all three registered plugins.
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 3u);
+
+ // First disable a plugin, then unregister it. Both should succeed.
+ ASSERT_TRUE(PluginManager::SetSystemRuntimePluginEnabled("b", false));
+ ASSERT_TRUE(PluginManager::UnregisterPlugin(CreateSystemRuntimePluginB));
+
+ // After un-registering a plugin it should be removed from plugin info.
+ plugin_info = PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(plugin_info.size(), 2u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[0].enabled, true);
+ ASSERT_EQ(plugin_info[1].name, "c");
+ ASSERT_EQ(plugin_info[1].enabled, true);
+}
+
+// Test un-registering and then re-registering a plugin will change the order of
+// loaded plugins.
+TEST_F(PluginManagerTest, UnRegisterSystemRuntimePluginChangesOrder) {
+ RegisterMockSystemRuntimePlugins();
+
+ std::vector<RegisteredPluginInfo> plugin_info =
+ PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginB);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2),
+ CreateSystemRuntimePluginC);
+
+ ASSERT_EQ(plugin_info.size(), 3u);
+ ASSERT_EQ(plugin_info[0].name, "a");
+ ASSERT_EQ(plugin_info[1].name, "b");
+ ASSERT_EQ(plugin_info[2].name, "c");
+
+ // Unregister and then registering a plugin puts it at the end of the order
+ // list.
+ ASSERT_TRUE(PluginManager::UnregisterPlugin(CreateSystemRuntimePluginB));
+ ASSERT_TRUE(PluginManager::RegisterPlugin("b", "New test instance B",
+ CreateSystemRuntimePluginB));
+
+ // Check the callback indices match as expected.
+ plugin_info = PluginManager::GetSystemRuntimePluginInfo();
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(0),
+ CreateSystemRuntimePluginA);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(1),
+ CreateSystemRuntimePluginC);
+ ASSERT_EQ(PluginManager::GetSystemRuntimeCreateCallbackAtIndex(2),
+ CreateSystemRuntimePluginB);
+
+ // And plugin info should match as well.
+ ASSERT_EQ(plugin_info.size(), 3u);
+ ...
[truncated]
|
|
This PR modifies the PluginManager to support enable/disable. A following commit will expose the commands to the user. This is currently only working for the SystemRuntime plugins. Once these first commits land I plan to do the same for the other plugin types. |
JDevlieghere
approved these changes
Apr 2, 2025
There was a problem hiding this comment.
This change LGTM.
Are you able to share the motivation behind wanting to enable/disable plugins at runtime?
I added support for disabling plugins at configuration/build time and at the time @labath had concerns about that as various parts of LLDB depend on certain plugins. Do you plan on making this opt-in for plugins or will users be able to disable arbitrary plugins at runtime?
The motivation is to enable easier testing and experimentation. It came from our attempts to try lazy symbol loading (i.e. We fixed a source of symbol loading in #129593, but found that there were several plugins that would force load all symbols. Some of these plugins (e.g. ASAN) are loaded unconditionally with no way to disable them. This not only makes it hard to test, but also harder to do one-off experiments without modifying the code and rebuilding. After discussing with @clayborg we thought a generic mechanism to disable plugins would be useful and could help with things beyond our original motivation. We also have some internal patches to disable some of these plugins, but if we have a generic way to do it then we can add it to our custom lldb init. This should reduce the number of internal diffs we have. One last idea I should mention is that we thought we could also extend this mechanism to support reordering of plugins. So that, for example, when running on linux we could reorder the ObjectFile plugins to have the Elf plugin come first. Just an idea at this point, but something we might explore in the future.
The plan is to allow disabling arbitrary plugins at runtime. We could have an opt-in or opt-out mechanism, but my feeling is that giving the user full control is not too bad. Most end users will not touch it and lldb developers can manage their self-imposed difficulties. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/15347 Here is the relevant piece of the build log for the reference |
I suspect this is related to #134048, which is also seeing the failure. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/3500 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/7555 Here is the relevant piece of the build log for the reference |
dmpots
added a commit
to dmpots/llvm-project
that referenced
this pull request
Apr 2, 2025
This is an attempt to fix a test failure from llvm#133794 when running on windows builds. I suspect we are running into a case where the [ICF](https://learn.microsoft.com/en-us/cpp/build/reference/opt-optimizations?view=msvc-170) optimization kicks in and combines the CreateSystemRuntimePlugin* functions into a single address. This means that we cannot uniquely unregister the plugin based on its create function address. The fix is have each create function return a different (bogus) value.
|
I have an attempted fix for these failures in #134173. I suspect ICF is combining the create functions into a single address which make us unable to remove them based on a unique address (which the plugin manager assumes). |
dmpots
added a commit
that referenced
this pull request
Apr 3, 2025
This is an attempt to fix a test failure from #133794 when running on windows builds. I suspect we are running into a case where the [ICF](https://learn.microsoft.com/en-us/cpp/build/reference/opt-optimizations?view=msvc-170) optimization kicks in and combines the CreateSystemRuntimePlugin* functions into a single address. This means that we cannot uniquely unregister the plugin based on its create function address. The fix is have each create function return a different (bogus) value.
dmpots
added a commit
to dmpots/llvm-project
that referenced
this pull request
Apr 3, 2025
This reverts commit 2026873.
dmpots
added a commit
to dmpots/llvm-project
that referenced
this pull request
Apr 3, 2025
Revert the below two commits while we investigate the test failures. Revert "[lldb] Fix plugin manager test failure on windows (llvm#134173)" This reverts commit b55bab2. Revert "Add enable/disable api for SystemRuntime plugins (llvm#133794)" This reverts commit 2026873.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit adds support for enabling and disabling plugins by name. The changes are made generically in the
PluginInstancesclass, but currently we only expose the ability to SystemRuntime plugins. Other plugins types can be added easily.We had a few design goals for how disabled plugins should work
For the implementation we modify the plugin instance to maintain a bool of its enabled state. Existing clients external to the Instances class expect to iterate over only enabled instance so we skip over disabed instances in the query and snapshot apis. This way the client does not have to manually check which instances are enabled.