-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[lldb] Add completions for plugin list/enable/disable #147775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit adds completion support for the plugin commands. It will try
to complete partial namespaces to the full namespace string. If the
completion input is already a full namespace string then it will add all
the matching plugins in that namespace as completions.
This lets the user complete to the namespace first and then tab-complete
to the next level if desired.
```
(lldb) plugin list a<tab>
Available completions:
abi
architecture
(lldb) plugin list ab<tab>
(lldb) plugin list abi<tab>
(lldb) plugin list abi.<tab>
Available completions:
abi.SysV-arm64
abi.ABIMacOSX_arm64
abi.SysV-arm
...
```
|
@llvm/pr-subscribers-lldb Author: David Peixotto (dmpots) ChangesThis commit adds completion support for the plugin commands. It will try to complete partial namespaces to the full namespace string. If the completion input is already a full namespace string then it will add all the matching plugins in that namespace as completions. This lets the user complete to the namespace first and then tab-complete to the next level if desired. Full diff: https://github.com/llvm/llvm-project/pull/147775.diff 8 Files Affected:
diff --git a/lldb/include/lldb/Core/PluginManager.h b/lldb/include/lldb/Core/PluginManager.h
index 5499e99025d8a..369785ceea5a5 100644
--- a/lldb/include/lldb/Core/PluginManager.h
+++ b/lldb/include/lldb/Core/PluginManager.h
@@ -787,6 +787,9 @@ class PluginManager {
static std::vector<RegisteredPluginInfo> GetUnwindAssemblyPluginInfo();
static bool SetUnwindAssemblyPluginEnabled(llvm::StringRef name, bool enable);
+
+ static void AutoCompletePluginName(llvm::StringRef partial_name,
+ CompletionRequest &request);
};
} // namespace lldb_private
diff --git a/lldb/include/lldb/Interpreter/CommandCompletions.h b/lldb/include/lldb/Interpreter/CommandCompletions.h
index c7292b3b1471a..0c0424cbac6eb 100644
--- a/lldb/include/lldb/Interpreter/CommandCompletions.h
+++ b/lldb/include/lldb/Interpreter/CommandCompletions.h
@@ -123,6 +123,10 @@ class CommandCompletions {
static void ThreadIDs(CommandInterpreter &interpreter,
CompletionRequest &request, SearchFilter *searcher);
+ static void ManagedPlugins(CommandInterpreter &interpreter,
+ CompletionRequest &request,
+ SearchFilter *searcher);
+
/// This completer works for commands whose only arguments are a command path.
/// It isn't tied to an argument type because it completes not on a single
/// argument but on the sequence of arguments, so you have to invoke it by
diff --git a/lldb/include/lldb/lldb-enumerations.h b/lldb/include/lldb/lldb-enumerations.h
index 69e8671b6e21b..e021c7a926cf1 100644
--- a/lldb/include/lldb/lldb-enumerations.h
+++ b/lldb/include/lldb/lldb-enumerations.h
@@ -1321,10 +1321,11 @@ enum CompletionType {
eTypeCategoryNameCompletion = (1ul << 24),
eCustomCompletion = (1ul << 25),
eThreadIDCompletion = (1ul << 26),
+ eManagedPluginCompletion = (1ul << 27),
// This last enum element is just for input validation.
// Add new completions before this element,
// and then increment eTerminatorCompletion's shift value
- eTerminatorCompletion = (1ul << 27)
+ eTerminatorCompletion = (1ul << 28)
};
/// Specifies if children need to be re-computed
diff --git a/lldb/packages/Python/lldbsuite/test/lldbtest.py b/lldb/packages/Python/lldbsuite/test/lldbtest.py
index a4ff96e4158ce..63fadb59a82a1 100644
--- a/lldb/packages/Python/lldbsuite/test/lldbtest.py
+++ b/lldb/packages/Python/lldbsuite/test/lldbtest.py
@@ -2268,7 +2268,7 @@ def completions_match(self, command, completions, max_completions=-1):
completions, list(match_strings)[1:], "List of returned completion is wrong"
)
- def completions_contain(self, command, completions):
+ def completions_contain(self, command, completions, match=True):
"""Checks that the completions for the given command contain the given
list of completions."""
interp = self.dbg.GetCommandInterpreter()
@@ -2276,9 +2276,16 @@ def completions_contain(self, command, completions):
interp.HandleCompletion(command, len(command), 0, -1, match_strings)
for completion in completions:
# match_strings is a 1-indexed list, so we have to slice...
- self.assertIn(
- completion, list(match_strings)[1:], "Couldn't find expected completion"
- )
+ if match:
+ self.assertIn(
+ completion,
+ list(match_strings)[1:],
+ "Couldn't find expected completion",
+ )
+ else:
+ self.assertNotIn(
+ completion, list(match_strings)[1:], "Found unexpected completion"
+ )
def filecheck(
self, command, check_file, filecheck_options="", expect_cmd_failure=False
diff --git a/lldb/source/Commands/CommandCompletions.cpp b/lldb/source/Commands/CommandCompletions.cpp
index 38231a8e993c7..3e223090c4286 100644
--- a/lldb/source/Commands/CommandCompletions.cpp
+++ b/lldb/source/Commands/CommandCompletions.cpp
@@ -87,6 +87,7 @@ bool CommandCompletions::InvokeCommonCompletionCallbacks(
{lldb::eTypeCategoryNameCompletion,
CommandCompletions::TypeCategoryNames},
{lldb::eThreadIDCompletion, CommandCompletions::ThreadIDs},
+ {lldb::eManagedPluginCompletion, CommandCompletions::ManagedPlugins},
{lldb::eTerminatorCompletion,
nullptr} // This one has to be last in the list.
};
@@ -850,6 +851,13 @@ void CommandCompletions::ThreadIDs(CommandInterpreter &interpreter,
}
}
+void CommandCompletions::ManagedPlugins(CommandInterpreter &interpreter,
+ CompletionRequest &request,
+ SearchFilter *searcher) {
+ PluginManager::AutoCompletePluginName(request.GetCursorArgumentPrefix(),
+ request);
+}
+
void CommandCompletions::CompleteModifiableCmdPathArgs(
CommandInterpreter &interpreter, CompletionRequest &request,
OptionElementVector &opt_element_vector) {
diff --git a/lldb/source/Commands/CommandObjectPlugin.cpp b/lldb/source/Commands/CommandObjectPlugin.cpp
index cdc9006bf5261..093ebde5f5f2c 100644
--- a/lldb/source/Commands/CommandObjectPlugin.cpp
+++ b/lldb/source/Commands/CommandObjectPlugin.cpp
@@ -194,6 +194,14 @@ List only the plugin 'foo' matching a fully qualified name exactly
Options *GetOptions() override { return &m_options; }
+ void
+ HandleArgumentCompletion(CompletionRequest &request,
+ OptionElementVector &opt_element_vector) override {
+ lldb_private::CommandCompletions::InvokeCommonCompletionCallbacks(
+ GetCommandInterpreter(), lldb::eManagedPluginCompletion, request,
+ nullptr);
+ }
+
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
size_t argc = command.GetArgumentCount();
@@ -293,6 +301,14 @@ class CommandObjectPluginEnable : public CommandObjectParsed {
AddSimpleArgumentList(eArgTypeManagedPlugin);
}
+ void
+ HandleArgumentCompletion(CompletionRequest &request,
+ OptionElementVector &opt_element_vector) override {
+ lldb_private::CommandCompletions::InvokeCommonCompletionCallbacks(
+ GetCommandInterpreter(), lldb::eManagedPluginCompletion, request,
+ nullptr);
+ }
+
~CommandObjectPluginEnable() override = default;
protected:
@@ -309,6 +325,14 @@ class CommandObjectPluginDisable : public CommandObjectParsed {
AddSimpleArgumentList(eArgTypeManagedPlugin);
}
+ void
+ HandleArgumentCompletion(CompletionRequest &request,
+ OptionElementVector &opt_element_vector) override {
+ lldb_private::CommandCompletions::InvokeCommonCompletionCallbacks(
+ GetCommandInterpreter(), lldb::eManagedPluginCompletion, request,
+ nullptr);
+ }
+
~CommandObjectPluginDisable() override = default;
protected:
diff --git a/lldb/source/Core/PluginManager.cpp b/lldb/source/Core/PluginManager.cpp
index 3f20a96edc187..59d1dac40a00c 100644
--- a/lldb/source/Core/PluginManager.cpp
+++ b/lldb/source/Core/PluginManager.cpp
@@ -18,6 +18,7 @@
#include "lldb/Utility/Status.h"
#include "lldb/Utility/StringList.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/Support/DynamicLibrary.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/raw_ostream.h"
@@ -2473,3 +2474,35 @@ bool PluginManager::SetUnwindAssemblyPluginEnabled(llvm::StringRef name,
bool enable) {
return GetUnwindAssemblyInstances().SetInstanceEnabled(name, enable);
}
+
+void PluginManager::AutoCompletePluginName(llvm::StringRef name,
+ CompletionRequest &request) {
+ // Split the name into the namespace and the plugin name.
+ // If there is no dot then the ns_name will be equal to name and
+ // plugin_prefix will be empty.
+ llvm::StringRef ns_name, plugin_prefix;
+ std::tie(ns_name, plugin_prefix) = name.split('.');
+
+ for (const PluginNamespace &plugin_ns : GetPluginNamespaces()) {
+ // If the plugin namespace matches exactly then
+ // add all the plugins in this namespace as completions if the
+ // plugin names starts with the plugin_prefix. If the plugin_prefix
+ // is empty then it will match all the plugins (empty string is a
+ // prefix of everything).
+ if (plugin_ns.name == ns_name) {
+ for (const RegisteredPluginInfo &plugin : plugin_ns.get_info()) {
+ llvm::SmallString<128> buf;
+ if (plugin.name.starts_with(plugin_prefix))
+ request.AddCompletion(
+ (plugin_ns.name + "." + plugin.name).toStringRef(buf));
+ }
+ }
+ // Otherwise check if the namespace is a prefix of the full name.
+ // Use a partial completion here so that we can either operate on the full
+ // namespace or tab-complete to the next level.
+ else if (plugin_ns.name.starts_with(name) &&
+ !plugin_ns.get_info().empty()) {
+ request.AddCompletion(plugin_ns.name, "", CompletionMode::Partial);
+ }
+ }
+}
diff --git a/lldb/test/API/commands/plugin/TestPlugin.py b/lldb/test/API/commands/plugin/TestPlugin.py
index fdfb14bfcc24e..e7de7a3f797f1 100644
--- a/lldb/test/API/commands/plugin/TestPlugin.py
+++ b/lldb/test/API/commands/plugin/TestPlugin.py
@@ -60,3 +60,49 @@ def do_list_disable_enable_test(self, plugin_namespace):
self.expect(
f"plugin enable {plugin_namespace}", substrs=[plugin_namespace, "[+]"]
)
+
+ def test_completions(self):
+ # Make sure completions work for the plugin list, enable, and disable commands.
+ # We just check a few of the expected plugins to make sure the completion works.
+ self.completions_contain(
+ "plugin list ", ["abi", "architecture", "disassembler"]
+ )
+ self.completions_contain(
+ "plugin enable ", ["abi", "architecture", "disassembler"]
+ )
+ self.completions_contain(
+ "plugin disable ", ["abi", "architecture", "disassembler"]
+ )
+
+ # A completion for a partial namespace should be the full namespace.
+ # This allows the user to run the command on the full namespace.
+ self.completions_match("plugin list ab", ["abi"])
+ self.completions_contain(
+ "plugin list object", ["object-container", "object-file"]
+ )
+
+ # A completion for a full namespace should contain the plugins in that namespace.
+ self.completions_contain("plugin list abi", ["abi.sysv-x86_64"])
+ self.completions_contain("plugin list abi.", ["abi.sysv-x86_64"])
+ self.completions_contain("plugin list abi.s", ["abi.sysv-x86_64"])
+ self.completions_contain("plugin list abi.sysv-x", ["abi.sysv-x86_64"])
+
+ # Check for a completion that is a both a complete namespace and a prefix of
+ # another namespace. It should return the completions for the plugins in the completed
+ # namespace as well as the completion for the partial namespace.
+ self.completions_contain(
+ "plugin list language", ["language.cplusplus", "language-runtime"]
+ )
+
+ # When the namespace is a prefix of another namespace and the user types a dot, the
+ # completion should not include the match for the partial namespace.
+ self.completions_contain(
+ "plugin list language.", ["language.cplusplus"], match=True
+ )
+ self.completions_contain(
+ "plugin list language.", ["language-runtime"], match=False
+ )
+
+ # Check for an empty completion list when the names is invalid.
+ # See docs for `complete_from_to` for how this checks for an empty list.
+ self.complete_from_to("plugin list abi.foo", ["plugin list abi.foo"])
|
| llvm::SmallString<128> buf; | ||
| if (plugin.name.starts_with(plugin_prefix)) | ||
| request.AddCompletion( | ||
| (plugin_ns.name + "." + plugin.name).toStringRef(buf)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This StringRef will immediately point to deallocated stack memory after the lexical block ends. I took a cursory look at the APIs and it doesn't look like any of them copy the string. Is this a use-after-free or what makes this safe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation of AddCompletion it says it will store a copy of the completion so it can be freed after.
Checking the code, AddCompletion calls CompletionResult::AddResult which constructs a new Completion object
| Completion r(completion, description, mode); |
And the Completion object stores the value as a string and initializes it with a copy of the input StringRef.
| : m_completion(completion.rtrim().str()), |
I believe the StringRef will point to valid memory for the lifetime of the call to AddCompletion and since it makes a copy of the data inside that function we should be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AddCompletion will copy the StringRef into a std::string (and/or an llvm::StringSet, I didn't look in great detail).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now that I see the code I remember I touched those lines not too long ago. Thanks for confirming.
| eManagedPluginCompletion = (1ul << 27), | ||
| // This last enum element is just for input validation. | ||
| // Add new completions before this element, | ||
| // and then increment eTerminatorCompletion's shift value | ||
| eTerminatorCompletion = (1ul << 27) | ||
| eTerminatorCompletion = (1ul << 28) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This eTerminatorCompletion is not good as it will change and any older binaries that linked against a previous version of LLDB could now get eManagedPluginCompletion instead of eTerminatorCompletion. Can we remove the eTerminatorCompletion? It can cause API issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will follow up with a separate PR to remove it.
|
Follow up PR to fix a buildbot failure: #148956 |
This commit adds completion support for the plugin commands. It will try to complete partial namespaces to the full namespace string. If the completion input is already a full namespace string then it will add all the matching plugins in that namespace as completions.
This lets the user complete to the namespace first and then tab-complete to the next level if desired.