Add option to pass thread ID to thread select command #73596
Conversation
|
@llvm/pr-subscribers-lldb Author: Michael Christensen (mdko) ChangesWe'd like a way to select the current thread by its thread ID (rather than its internal LLDB thread index). This PR adds a Here's an example of it working: Full diff: https://github.com/llvm/llvm-project/pull/73596.diff 3 Files Affected:
diff --git a/lldb/source/Commands/CommandObjectThread.cpp b/lldb/source/Commands/CommandObjectThread.cpp
index a9f5a4f8a4fbd71c..9384df319cc221dd 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -1129,8 +1129,44 @@ class CommandObjectThreadUntil : public CommandObjectParsed {
// CommandObjectThreadSelect
+#define LLDB_OPTIONS_thread_select
+#include "CommandOptions.inc"
+
class CommandObjectThreadSelect : public CommandObjectParsed {
public:
+ class CommandOptions : public Options {
+ public:
+ CommandOptions() { OptionParsingStarting(nullptr); }
+
+ ~CommandOptions() override = default;
+
+ void OptionParsingStarting(ExecutionContext *execution_context) override {
+ m_thread_id = false;
+ }
+
+ Status SetOptionValue(uint32_t option_idx, llvm::StringRef option_arg,
+ ExecutionContext *execution_context) override {
+ const int short_option = m_getopt_table[option_idx].val;
+ switch (short_option) {
+ case 't': {
+ m_thread_id = true;
+ break;
+ }
+
+ default:
+ llvm_unreachable("Unimplemented option");
+ }
+
+ return {};
+ }
+
+ llvm::ArrayRef<OptionDefinition> GetDefinitions() override {
+ return llvm::ArrayRef(g_thread_select_options);
+ }
+
+ bool m_thread_id;
+ };
+
CommandObjectThreadSelect(CommandInterpreter &interpreter)
: CommandObjectParsed(interpreter, "thread select",
"Change the currently selected thread.", nullptr,
@@ -1165,6 +1201,8 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
nullptr);
}
+ Options *GetOptions() override { return &m_options; }
+
protected:
void DoExecute(Args &command, CommandReturnObject &result) override {
Process *process = m_exe_ctx.GetProcessPtr();
@@ -1173,22 +1211,29 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
return;
} else if (command.GetArgumentCount() != 1) {
result.AppendErrorWithFormat(
- "'%s' takes exactly one thread index argument:\nUsage: %s\n",
- m_cmd_name.c_str(), m_cmd_syntax.c_str());
+ "'%s' takes exactly one thread %s argument:\nUsage: %s\n",
+ m_cmd_name.c_str(), m_options.m_thread_id ? "ID" : "index",
+ 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'",
+ result.AppendErrorWithFormat("Invalid thread %s '%s'",
+ m_options.m_thread_id ? "ID" : "index",
command.GetArgumentAtIndex(0));
return;
}
- Thread *new_thread =
- process->GetThreadList().FindThreadByIndexID(index_id).get();
+ Thread *new_thread = nullptr;
+ if (m_options.m_thread_id) {
+ new_thread = process->GetThreadList().FindThreadByID(index_id).get();
+ } else {
+ new_thread = process->GetThreadList().FindThreadByIndexID(index_id).get();
+ }
if (new_thread == nullptr) {
- result.AppendErrorWithFormat("invalid thread #%s.\n",
+ result.AppendErrorWithFormat("invalid thread %s%s.\n",
+ m_options.m_thread_id ? "ID " : "#",
command.GetArgumentAtIndex(0));
return;
}
@@ -1196,6 +1241,8 @@ class CommandObjectThreadSelect : public CommandObjectParsed {
process->GetThreadList().SetSelectedThreadByID(new_thread->GetID(), true);
result.SetStatus(eReturnStatusSuccessFinishNoResult);
}
+
+ CommandOptions m_options;
};
// CommandObjectThreadList
diff --git a/lldb/source/Commands/Options.td b/lldb/source/Commands/Options.td
index 542c78be5a12dada..23886046df8f673e 100644
--- a/lldb/source/Commands/Options.td
+++ b/lldb/source/Commands/Options.td
@@ -1117,6 +1117,11 @@ let Command = "thread plan list" in {
Desc<"Display thread plans for unreported threads">;
}
+let Command = "thread select" in {
+ def thread_thread_id : Option<"thread_id", "t">,
+ 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">,
diff --git a/lldb/test/API/commands/thread/select/TestThreadSelect.py b/lldb/test/API/commands/thread/select/TestThreadSelect.py
index 91f8909471bf2bbe..4d01b82d9d947e5b 100644
--- a/lldb/test/API/commands/thread/select/TestThreadSelect.py
+++ b/lldb/test/API/commands/thread/select/TestThreadSelect.py
@@ -12,17 +12,34 @@ 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 '0x1ffffffff'",
+ )
# Parses but not a valid thread id.
self.expect(
"thread select 0xffffffff",
error=True,
startstr="error: invalid thread #0xffffffff.",
)
+ self.expect(
+ "thread select -t 0xffffffff",
+ error=True,
+ startstr="error: invalid thread ID 0xffffffff.",
+ )
+
+ 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(),
+ )
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I would prefer you make the |
|
@jimingham So it seems that when Would it make sense then for and (where the
Sounds good, I'll work on adding the complete for the native thread ID option with the above change. |
|
lldb doesn't have a notion of arguments actually being the value of a default option. I'm not sure I like that idea, it seems like it could quickly become confusing.
lldb does have the notion of "groups of options" that work together for a given command. You can specify `break set -n foo` or `break set -a 0x1234` but `-a` and `-n` are in different option groups so the command parser won't allow you to specify them together.
Someone started to do the same thing with arguments, so you can say "if --something is provided, you can have an argument, but if --something-else is provided you can't". But I don't think that is currently working (I noticed that hooking up the parsed scripted commands). For now, you'll have to check the inputs by hand in the DoExecute, but then once we hook up the argument groups properly, you will be able to specify "one option with -t and no arguments" and "one argument" as the two command options, and the parser will handle the checking for you. So that seems the more "lldb natural" way to do this.
It would have been better if we had made `thread select` take two options as you suggested. We could have then fixed the `t` alias to specify the thread index so that we would have fast access to the most common command. Sadly, I think `'thread select" has been around for long enough that changing the way the command line behaves would be disruptive, and I don't think that's a good idea.
JIm
… On Nov 28, 2023, at 9:58 AM, Michael Christensen ***@***.***> wrote:
@jimingham <https://github.com/jimingham> So it seems that when -t <tid> is supplied, the user shouldn't be able to specify a thread index as well, e.g. the following is disallowed:
thread select -t 216051 1
Would it make sense then for thread select to take two possible mutually exclusive options (-t <tid>) and (-i <index>), with the -i option being the default (i.e. doesn't need to be specified)? e.g.
thread select -t 216051
and
thread select -i 1
thread select 1
(where the -i doesn't need to be supplied by default)?
It makes the completer much easier to hook up since the option value can complete against the native thread ID's
Sounds good, I'll work on adding the complete for the native thread ID option with the above change.
—
Reply to this email directly, view it on GitHub <#73596 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUPVW3FLTLLF2QBC733FALYGYQ3PAVCNFSM6AAAAAA744LG7GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZQGM4TKMJRGE>.
You are receiving this because you were mentioned.
|
…separate group, and format
|
Latest changes implement reviewer suggestions:
Example usage: |
…ecifier, add thread ID completion
|
Latest changes update help and usage text, format specifier, add a general thread ID completion mechanism, and use this thread ID completion in the I'm not sure why the Example of changes, including completion. |
|
|
||
| ThreadList &threads = exe_ctx.GetProcessPtr()->GetThreadList(); | ||
| lldb::ThreadSP thread_sp; | ||
| for (uint32_t idx = 0; (thread_sp = threads.GetThreadAtIndex(idx)); ++idx) { |
There was a problem hiding this comment.
minor nit, don't block the review on this, but why do you have ()s here?
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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;
...
}
There was a problem hiding this comment.
(please don't resolve conversations until the author has had a chance to look at the answer)
| @@ -1296,10 +1296,11 @@ enum CompletionType { | |||
| eRemoteDiskFileCompletion = (1u << 22), | |||
| eRemoteDiskDirectoryCompletion = (1u << 23), | |||
| eTypeCategoryNameCompletion = (1u << 24), | |||
| eThreadIDCompletion = (1u << 25), | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@clayborg Is there anything special about
eCustomCompletionbeing 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.
There was a problem hiding this comment.
@jimingham What are your thoughts on putting eThreadIDCompletion before eCustomCompletion? (See @clayborg 's comments above)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…n enum; add completion terminator
|
@clayborg Latest change:
|
| eThreadIDCompletion = (1ul << 26), | ||
| // This last enum element is just for input validation. | ||
| // Add new completions before this element. | ||
| eTerminatorCompletion = (1ul << 63) |
There was a problem hiding this comment.
This isn't right, eTerminatorCompletion should be (1ul << 27) here. I want to be able to write:
if (number_passed_in >= eTerminatorCompletion)
error("You passed an invalid completion")
The value of eTerminatorCompletion gets shifted up each time someone adds a new completion, that allows each lldb version to check the input value against the completions it knows.
There was a problem hiding this comment.
@jimingham Thanks for the quick feedback, makes sense. Latest changes update eTerminatorCompletion value based on your comments.
|
@jimingham @clayborg IIUC I don't have merge permissions, otherwise I'd squash and merge this myself. Please let me know if there's anything I should do at this point. Thanks! |
We'd like a way to select the current thread by its thread ID (rather than its internal LLDB thread index).
This PR adds a
-toption (--thread_idlong option) that tells thethread selectcommand to interpret the<thread-index>argument as a thread ID.Here's an example of it working: