Page MenuHomePhabricator

[LLDB] CommandObjectThreadUntil::DoExecute() sets the wrong selected thread ID
ClosedPublic

Authored by RamNalamothu on Jul 3 2018, 12:07 AM.

Details

Summary

For the 'thread until' command, the selected thread ID, to perform the operation on, could be of the current thread or the specified thread.

Diff Detail

Event Timeline

ramana-nvr created this revision.Jul 3 2018, 12:07 AM

This is correct, thanks for catching it!

There are a few other places that use m_thread_idx above this change. Everything after the:

if (thread == nullptr) {

check should also use thread->GetID not m_options.m_thread_idx. This is all error reporting, so it's not a big deal. But having the error be "failed to resolve line table for frame 5 of thread 4294967295" would be confusing. Could you change those uses as well?

check should also use thread->GetID not m_options.m_thread_idx. This is all error reporting, so it's not a big deal. But having the error be "failed to resolve line table for frame 5 of thread 4294967295" would be confusing. Could you change those uses as well?

On the trunk, the error message will be "Failed to resolve the line table for frame 5 of thread index 3", which I think is okay.

Changed couple of others.

ramana-nvr updated this revision to Diff 154059.Jul 4 2018, 1:12 AM

Sorry, I wasn't clear. Suppose that the m_thread_idx coming into DoExecute is LLDB_INVALID_THREAD_ID. That's what happens if you don't select a thread id on the command line, for instance. Then the code around 1164 will set thread to be the default thread. But m_thread_idx will still be LLDB_INVALID_THREAD_ID. Then for instance if the selected frame of thread has no debug info, the error messages will read:

Frame index 0 of thread index 4294967295 has no debug information.

(the value of LLDB_INVALID_THREAD_ID) since the error is still printing m_thread_idx . The errors also need to print thread->GetID()...

The error messages now refer to the thread ID instead of the thread index.

jingham accepted this revision.Jul 12 2018, 10:16 AM

Thanks.

This revision is now accepted and ready to land.Jul 12 2018, 10:16 AM

Could you please add a test for the new behavior as well?

labath resigned from this revision.Aug 9 2019, 2:05 AM
RamNalamothu commandeered this revision.May 27 2022, 7:37 AM
RamNalamothu added a reviewer: ramana-nvr.
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2022, 7:38 AM

@jingham @labath

I have tried to add a test but it's becoming difficult for me to come-up with a failing test scenario. Is it okay to commit this without a test?

Otherwise, I could add the following or error capturing

diff --git a/lldb/source/Commands/CommandObjectThread.cpp b/lldb/source/Commands/CommandObjectThread.cpp
index 95a8301a318a..f91188f4446b 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -1096,7 +1096,7 @@ protected:
         return false;
       }
 
-      process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx);
+      assert(process->GetThreadList().SetSelectedThreadByID(m_options.m_thread_idx));
 
       StreamString stream;
       Status error;

which will break thread until functionality and then I can add the patch fixing it immediately.

Went ahead and added the error capturing for setting incorrect thread id and also the fix.

Without the following change/fix

diff --git a/lldb/source/Commands/CommandObjectThread.cpp b/lldb/source/Commands/CommandObjectThread.cpp
index 04457b232f3b..11affe8a7c13 100644
--- a/lldb/source/Commands/CommandObjectThread.cpp
+++ b/lldb/source/Commands/CommandObjectThread.cpp
@@ -1095,8 +1095,7 @@ protected:
         return false;
       }
 
-      if (!process->GetThreadList().SetSelectedThreadByID(
-              m_options.m_thread_idx)) {
+      if (!process->GetThreadList().SetSelectedThreadByID(thread->GetID())) {
         result.AppendErrorWithFormat(
             "Failed to set the selected thread to thread id %" PRIu64 ".\n",
             thread->GetID());

the lldb/test/API/commands/expression/formatters/TestFormatters.py:228: self.runCmd("thread until " + str(ret)) test would fail.

Herald added a project: Restricted Project. · View Herald TranscriptJun 2 2022, 11:44 PM

Does this look good now to land?

Jim Ingham should give any final OK since he is the code owner of the thread plans.

jingham accepted this revision.Jun 14 2022, 5:13 PM

Sorry, I missed the "ping" notification. Still LGTM.