Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lldb/include/lldb/Core/PluginManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions lldb/include/lldb/Interpreter/CommandCompletions.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion lldb/include/lldb/lldb-enumerations.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +1324 to +1328
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

};

/// Specifies if children need to be re-computed
Expand Down
15 changes: 11 additions & 4 deletions lldb/packages/Python/lldbsuite/test/lldbtest.py
Original file line number Diff line number Diff line change
Expand Up @@ -2268,17 +2268,24 @@ 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()
match_strings = lldb.SBStringList()
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
Expand Down
8 changes: 8 additions & 0 deletions lldb/source/Commands/CommandCompletions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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.
};
Expand Down Expand Up @@ -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) {
Expand Down
24 changes: 24 additions & 0 deletions lldb/source/Commands/CommandObjectPlugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down
32 changes: 32 additions & 0 deletions lldb/source/Core/PluginManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -2473,3 +2474,34 @@ 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));
Comment on lines +2494 to +2497
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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).

Copy link
Member

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.

}
} else if (plugin_ns.name.starts_with(name) &&
!plugin_ns.get_info().empty()) {
// 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.
request.AddCompletion(plugin_ns.name, "", CompletionMode::Partial);
}
}
}
46 changes: 46 additions & 0 deletions lldb/test/API/commands/plugin/TestPlugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Loading