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

[RamNalamothu]

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.

[jingham]

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?

[ramana-nvr]

In D48865#1150995, @jingham wrote:

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.

[jingham]

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

[ramana-nvr]

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

[jingham]

Thanks.

[labath]

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

[RamNalamothu]

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

[RamNalamothu]

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.

[RamNalamothu]

Does this look good now to land?

[clayborg]

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

[RamNalamothu]

Ping.

[RamNalamothu]

Ping.

[jingham]

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

lldb/source/Commands/CommandObjectThread.cpp

if (process == nullptr) {
       }
 
       const bool abort_other_plans = false;
 
       StackFrame *frame =
           thread->GetStackFrameAtIndex(m_options.m_frame_idx).get();
       if (frame == nullptr) {
         result.AppendErrorWithFormat(
-            "Frame index %u is out of range for thread %u.\n",
-            m_options.m_frame_idx, m_options.m_thread_idx);
+            "Frame index %u is out of range for thread id %" PRIu64 ".\n",
+            m_options.m_frame_idx, thread->GetID());
         return false;
       }
 
       ThreadPlanSP new_plan_sp;
       Status new_plan_status;
 
       if (frame->HasDebugInformation()) {
         // Finally we got here...  Translate the given line number to a bunch
         // of addresses:
         SymbolContext sc(frame->GetSymbolContext(eSymbolContextCompUnit));
         LineTable *line_table = nullptr;
         if (sc.comp_unit)
           line_table = sc.comp_unit->GetLineTable();
 
         if (line_table == nullptr) {
           result.AppendErrorWithFormat("Failed to resolve the line table for "
-                                       "frame %u of thread index %u.\n",
-                                       m_options.m_frame_idx,
-                                       m_options.m_thread_idx);
+                                       "frame %u of thread id %" PRIu64 ".\n",
+                                       m_options.m_frame_idx, thread->GetID());
           return false;
         }
 
         LineEntry function_start;
         uint32_t index_ptr = 0, end_ptr;
         std::vector<addr_t> address_list;
 
         // Find the beginning & end index of the function, but first make
if (process == nullptr) {
           // resume the original plan.
           new_plan_sp->SetIsControllingPlan(true);
           new_plan_sp->SetOkayToDiscard(false);
         } else {
           result.SetError(new_plan_status);
           return false;
         }
       } else {
-        result.AppendErrorWithFormat(
-            "Frame index %u of thread %u has no debug information.\n",
-            m_options.m_frame_idx, m_options.m_thread_idx);
+        result.AppendErrorWithFormat("Frame index %u of thread id %" PRIu64
+                                     " has no debug information.\n",
+                                     m_options.m_frame_idx, thread->GetID());
         return false;
       }
 
-      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());
+        return false;
+      }
 
       StreamString stream;
       Status error;
       if (synchronous_execution)
         error = process->ResumeSynchronous(&stream);
       else
         error = process->Resume();