Skip to content
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.

Sign up for GitHub

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

Add option to pass thread ID to thread select command #73596

Merged
merged 7 commits into from
Dec 14, 2023
Merged

Add option to pass thread ID to thread select command #73596

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97a6e23
Add option to pass thread ID to thread select command
mdko Nov 27, 2023
34045b9
Code review changes -- update thread-id argument name, separate into …
mdko Nov 29, 2023
3ef88cd
Fix formatting
mdko Nov 29, 2023
e30a155
Code review changes -- update help text and usage text, fix format sp…
mdko Nov 29, 2023
e109a2e
Empty commit to retrigger CI
mdko Nov 29, 2023
2db54f6
Code review changes -- put eThreadIDCompletion after eCustomCompletio…
mdko Dec 5, 2023
e1f1f0e
Code review changes -- update eTerminatorCompletion value
mdko Dec 5, 2023
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/Interpreter/CommandCompletions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ class CommandCompletions {
CompletionRequest &request,
SearchFilter *searcher);

static void ThreadIDs(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
1 change: 1 addition & 0 deletions lldb/include/lldb/Interpreter/CommandOptionArgumentTable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ static constexpr OptionEnumValueElement g_completion_type[] = {
"Completes to a remote disk directory."},
{lldb::eTypeCategoryNameCompletion, "type-category-name",
"Completes to a type category name."},
{lldb::eThreadIDCompletion, "thread-id", "Completes to a thread ID."},
{lldb::eCustomCompletion, "custom", "Custom completion."},
};

Expand Down
3 changes: 2 additions & 1 deletion lldb/include/lldb/lldb-enumerations.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1296,10 +1296,11 @@ enum CompletionType {
eRemoteDiskFileCompletion = (1u << 22),
eRemoteDiskDirectoryCompletion = (1u << 23),
eTypeCategoryNameCompletion = (1u << 24),
eThreadIDCompletion = (1u << 25),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Public enums are part of our public API, we can't add new values in the middle, we must always add them to the end. If someone compiled against an older LLDB, they would have 25 meaning eCustomCompletion, but if the re-link allowing the above change it would now mean eThreadIDCompletion

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayborg Is there anything special about eCustomCompletion being last? The comment in 1300-1302 seems to allude to this, but I don't see anything in the code base requiring this nor any other custom completions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayborg Is there anything special about eCustomCompletion being last? The comment in 1300-1302 seems to allude to this, but I don't see anything in the code base requiring this nor any other custom completions.

Yes, this is a problem. If there ever is a magic bit it should be something like:

eCustomCompletion = (1u << 63) // Pick the last bit in a 64 bit value

But this is public API now which causes a problem for reasons I mentioned before. @jimingham any ideas on if we care about adding the thread ID before the eCustomCompletion from an API standpoint? lldb-rpc-server is the main thing I worry about here since it sends enums as integers, not as strings which are then converted back into integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jimingham What are your thoughts on putting eThreadIDCompletion before eCustomCompletion? (See @clayborg 's comments above)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent of the API standpoint, Greg is right. This is ABI-breaking, something we try hard to avoid in LLDB. Even though programs linked against older versions of LLDB will still be able to run against newer versions of LLDB, the semantics of these values have changed and the program may do the wrong thing. Even though I don't think it's likely that this will affect many folks, you can never be too sure.

https://lldb.llvm.org/resources/sbapi.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though this was a public enum, there was never anything you could do with it from the SB API's. The only place you could set completions at all from the outside was in command script add and that didn't use the enum values, it used string equivalents.
So I think at this point we can change it as we need.
We also don't add completions that extend this table anywhere in lldb that I can see, we use custom completion functions that we use by hand in the HandleCompletion's instead. And anyway, all that is internal to lldb_private, that's not exposed.

So I think none of this is really designed or used at this point, and we should think of what we want to do with this.

I think eCustomCompletion should mean "this object implements a custom completion, use that instead of any of the built-in ones." So overloading it to be the enum terminator is wrong, since its value cannot float, it has to have a definite meaning.

The terminator is useful for input validation. The "parsed command" patch I'm working on WILL allow users to specify a completion for the arguments, and it would be useful to be able to say "that enum value you just passed me was out of the range of valid completions." But I think that's all it's needed for.

So I'd suggest leaving eCustomCompletion where it is, adding the Thread ID one after and then adding a terminator for input validation.

// This item serves two purposes. It is the last element in the enum, so
// you can add custom enums starting from here in your Option class. Also
// if you & in this bit the base code will not process the option.
eCustomCompletion = (1u << 25)
eCustomCompletion = (1u << 26)
};

} // namespace lldb
Expand Down
18 changes: 18 additions & 0 deletions lldb/source/Commands/CommandCompletions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ bool CommandCompletions::InvokeCommonCompletionCallbacks(
CommandCompletions::RemoteDiskDirectories},
{lldb::eTypeCategoryNameCompletion,
CommandCompletions::TypeCategoryNames},
{lldb::eThreadIDCompletion, CommandCompletions::ThreadIDs},
{lldb::CompletionType::eNoCompletion,
nullptr} // This one has to be last in the list.
};
Expand Down Expand Up @@ -807,6 +808,23 @@ void CommandCompletions::TypeCategoryNames(CommandInterpreter &interpreter,
});
}

void CommandCompletions::ThreadIDs(CommandInterpreter &interpreter,
CompletionRequest &request,
SearchFilter *searcher) {
const ExecutionContext &exe_ctx = interpreter.GetExecutionContext();
if (!exe_ctx.HasProcessScope())
return;

ThreadList &threads = exe_ctx.GetProcessPtr()->GetThreadList();
lldb::ThreadSP thread_sp;
for (uint32_t idx = 0; (thread_sp = threads.GetThreadAtIndex(idx)); ++idx) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit, don't block the review on this, but why do you have ()s here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to avoid this warning:

/home/michristensen/llvm/llvm-project/lldb/source/Commands/CommandCompletions.cpp:820:36: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
  for (uint32_t idx = 0; thread_sp = threads.GetThreadAtIndex(idx); ++idx) {
                         ~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/michristensen/llvm/llvm-project/lldb/source/Commands/CommandCompletions.cpp:820:36: note: place parentheses around the assignment to silence this warning
  for (uint32_t idx = 0; thread_sp = threads.GetThreadAtIndex(idx); ++idx) {

It's also consistent to how it was done in the ThreadIndex completer code:

llvm-project/lldb/source/Commands/CommandCompletions.cpp

Line 776 in e109a2e

for (uint32_t idx = 0; (thread_sp = threads.GetThreadAtIndex(idx)); ++idx) {

Copy link
Contributor

@felipepiovezan felipepiovezan Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, I had not noticed what we were doing here, we usually see that pattern in ifs, but not so much in fors

So that means that if one of the thread pointers in the middle of the list is null, we stop processing other threads? This seems counter intuitive, I would have expected the code to be:

for (auto idx : llvm::seq<uint32_t>(threads.GetSize()) {
   const auto thread_sp = threads.GetThreadAtIndex(idx);
   if (!thread_sp) continue;
   ...
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(please don't resolve conversations until the author has had a chance to look at the answer)

StreamString strm;
thread_sp->GetStatus(strm, 0, 1, 1, true);
request.TryCompleteCurrentArg(std::to_string(thread_sp->GetID()),
strm.GetString());
}
}

void CommandCompletions::CompleteModifiableCmdPathArgs(
CommandInterpreter &interpreter, CompletionRequest &request,
OptionElementVector &opt_element_vector) {
Expand Down
96 changes: 81 additions & 15 deletions lldb/source/Commands/CommandObjectThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,11 +1129,51 @@ class CommandObjectThreadUntil : public CommandObjectParsed {

// CommandObjectThreadSelect

#define LLDB_OPTIONS_thread_select
#include "CommandOptions.inc"

class CommandObjectThreadSelect : public CommandObjectParsed {
public:
class OptionGroupThreadSelect : public OptionGroup {
public:
OptionGroupThreadSelect() { OptionParsingStarting(nullptr); }

~OptionGroupThreadSelect() override = default;

void OptionParsingStarting(ExecutionContext *execution_context) override {
m_thread_id = LLDB_INVALID_THREAD_ID;
}

Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
ExecutionContext *execution_context) override {
const int short_option = g_thread_select_options[option_idx].short_option;
switch (short_option) {
case 't': {
if (option_arg.getAsInteger(0, m_thread_id)) {
m_thread_id = LLDB_INVALID_THREAD_ID;
return Status("Invalid thread ID: '%s'.", option_arg.str().c_str());
}
break;
}

default:
llvm_unreachable("Unimplemented option");
}

return {};
}

llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
return llvm::ArrayRef(g_thread_select_options);
}

lldb::tid_t m_thread_id;
};

CommandObjectThreadSelect(CommandInterpreter &interpreter)
: CommandObjectParsed(interpreter, "thread select",
"Change the currently selected thread.", nullptr,
"Change the currently selected thread.",
"thread select <thread-index> (or -t <thread-id>)",
eCommandRequiresProcess | eCommandTryTargetAPILock |
eCommandProcessMustBeLaunched |
eCommandProcessMustBePaused) {
Expand All @@ -1143,13 +1183,17 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
// Define the first (and only) variant of this arg.
thread_idx_arg.arg_type = eArgTypeThreadIndex;
thread_idx_arg.arg_repetition = eArgRepeatPlain;
thread_idx_arg.arg_opt_set_association = LLDB_OPT_SET_1;

// There is only one variant this argument could be; put it into the
// argument entry.
arg.push_back(thread_idx_arg);

// Push the data for the first argument into the m_arguments vector.
m_arguments.push_back(arg);

m_option_group.Append(&m_options, LLDB_OPT_SET_ALL, LLDB_OPT_SET_2);
m_option_group.Finalize();
}

~CommandObjectThreadSelect() override = default;
Expand All @@ -1165,37 +1209,59 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
nullptr);
}

Options *GetOptions() override { return &m_option_group; }

protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
Process *process = m_exe_ctx.GetProcessPtr();
if (process == nullptr) {
result.AppendError("no process");
return;
} else if (command.GetArgumentCount() != 1) {
} else if (m_options.m_thread_id == LLDB_INVALID_THREAD_ID &&
command.GetArgumentCount() != 1) {
result.AppendErrorWithFormat(
"'%s' takes exactly one thread index argument:\nUsage: %s\n",
"'%s' takes exactly one thread index argument, or a thread ID "
"option:\nUsage: %s\n",
m_cmd_name.c_str(), m_cmd_syntax.c_str());
return;
}

uint32_t index_id;
if (!llvm::to_integer(command.GetArgumentAtIndex(0), index_id)) {
result.AppendErrorWithFormat("Invalid thread index '%s'",
command.GetArgumentAtIndex(0));
} else if (m_options.m_thread_id != LLDB_INVALID_THREAD_ID &&
command.GetArgumentCount() != 0) {
result.AppendErrorWithFormat("'%s' cannot take both a thread ID option "
"and a thread index argument:\nUsage: %s\n",
m_cmd_name.c_str(), m_cmd_syntax.c_str());
return;
}

Thread *new_thread =
process->GetThreadList().FindThreadByIndexID(index_id).get();
if (new_thread == nullptr) {
result.AppendErrorWithFormat("invalid thread #%s.\n",
command.GetArgumentAtIndex(0));
return;
Thread *new_thread = nullptr;
if (command.GetArgumentCount() == 1) {
uint32_t index_id;
if (!llvm::to_integer(command.GetArgumentAtIndex(0), index_id)) {
result.AppendErrorWithFormat("Invalid thread index '%s'",
command.GetArgumentAtIndex(0));
return;
}
new_thread = process->GetThreadList().FindThreadByIndexID(index_id).get();
if (new_thread == nullptr) {
result.AppendErrorWithFormat("Invalid thread index #%s.\n",
command.GetArgumentAtIndex(0));
return;
}
} else {
new_thread =
process->GetThreadList().FindThreadByID(m_options.m_thread_id).get();
if (new_thread == nullptr) {
result.AppendErrorWithFormat("Invalid thread ID %" PRIu64 ".\n",
m_options.m_thread_id);
return;
}
}

process->GetThreadList().SetSelectedThreadByID(new_thread->GetID(), true);
result.SetStatus(eReturnStatusSuccessFinishNoResult);
}

OptionGroupThreadSelect m_options;
OptionGroupOptions m_option_group;
};

// CommandObjectThreadList
Expand Down
6 changes: 6 additions & 0 deletions lldb/source/Commands/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,6 +1117,12 @@ let Command = "thread plan list" in {
Desc<"Display thread plans for unreported threads">;
}

let Command = "thread select" in {
def thread_select_thread_id : Option<"thread-id", "t">, Group<2>,
Arg<"ThreadID">, Completion<"ThreadID">,
Desc<"Provide a thread ID instead of a thread index.">;
}

let Command = "thread trace dump function calls" in {
def thread_trace_dump_function_calls_file : Option<"file", "F">, Group<1>,
Arg<"Filename">,
Expand Down
35 changes: 31 additions & 4 deletions lldb/test/API/commands/thread/select/TestThreadSelect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,44 @@ def test_invalid_arg(self):
self, "// break here", lldb.SBFileSpec("main.cpp")
)

self.expect(
"thread select -1", error=True, startstr="error: Invalid thread index '-1'"
)
self.expect(
"thread select 0x1ffffffff",
error=True,
startstr="error: Invalid thread index '0x1ffffffff'",
)
self.expect(
"thread select -t 0x1ffffffff",
error=True,
startstr="error: Invalid thread ID",
)
self.expect(
"thread select 1 2 3",
error=True,
startstr="error: 'thread select' takes exactly one thread index argument, or a thread ID option:",
)
self.expect(
"thread select -t 1234 1",
error=True,
startstr="error: 'thread select' cannot take both a thread ID option and a thread index argument:",
)
# Parses but not a valid thread id.
self.expect(
"thread select 0xffffffff",
error=True,
startstr="error: invalid thread #0xffffffff.",
startstr="error: Invalid thread index #0xffffffff.",
)
self.expect(
"thread select -t 0xffffffff",
error=True,
startstr="error: Invalid thread ID",
)

def test_thread_select_tid(self):
self.build()

lldbutil.run_to_source_breakpoint(
self, "// break here", lldb.SBFileSpec("main.cpp")
)
self.runCmd(
"thread select -t %d" % self.thread().GetThreadID(),
)