On 8/6/26 10:54 PM, Andrew Burgess wrote:
> I noticed that some code related to proceeding threads could be shared
> between CLI and MI.  This fixes a bug as the CLI code contains a fix
> that the MI code is missing.
> 
> In continue_1 (in infcmd.c) we have a loop that iterates over all
> threads looking for threads that are THREAD_STOPPED and are in an
> inferior that has_execution.  For each thread found we then call:
> 
>    switch_to_thread (&thread);
>    clear_proceed_status (0);
>    proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT);
> 
> In exec_continue (in mi/mi-main.c) we also have a loop over all
> threads that calls the proceed_thread helper function which skips
> threads that are not THREAD_STOPPED, does some PID related
> filtering (more on this later) and then calls the same three
> functions: switch_to_thread, clear_proceed_status, proceed.
> 
> The PID filtering mentioned above allows proceed_thread to do two
> jobs, if the PID is zero then we proceed all threads.  If PID is
> non-zero then we proceed only the threads in the inferior with that
> PID.
> 
> You might also have spotted that in continue_1 we checked if the
> inferior has execution or not.  This check was added in commit:
> 
>    commit 5b6d1e4fa4fc6827c7b3f0e99ff120dfa14d65d2
>    Date:   Fri Jan 10 20:06:08 2020 +0000
> 
>        Multi-target support
> 
> A matching check was not added into the MI at this point, nor did the
> commit message mention why such a check was not added.  I'm choosing
> to believe that this was an oversight in the 5b6d1e4fa4fc6827 commit.
> And this is the bug I mentioned above.
> 
> If we call `proceed` with a thread that is part of an inferior that
> does not "has_execution" then the thread will be marked running even
> though it will never actually be set running.  See the early return at
> the top of `proceed_resume_thread_checked` and the call to set_state
> in `proceed`.
> 
> I do worry that there might be a bigger set of bugs here if proceed
> can set a thread's state to THREAD_RUNNING, but then never actually
> sets the underlying thread running.  But in this case, just having the
> MI share code with the CLI means that we pick up the fix for this case
> basically for free.
> 
> I propose adding a new global helper function `proceed_one_thread`,
> this will check if the thread is THREAD_STOPPED and is in an inferior
> which has_execution.  If these conditions are met then the three
> functions mentioned above will be called to proceed the thread.
> 
> I will then add a second new function `proceed_all_threads`, this will
> iterate over all threads and call proceed_one_thread.
> 
> We can then use proceed_all_threads from continue_1, replacing the
> existing loop.
> 
> In exec_continue we can move the PID (or rather inferior) check
> earlier, outside the loop.  If we want to resume all threads (the old
> PID is zero path) then we call proceed_all_threads.  If we only want
> to proceed threads with one PID then we loop over threads in the
> matching inferior and call proceed_one_thread on each.
> 
> As the code I am factoring out is all within non_stop only paths I
> have added `gdb_assert (non_stop);` to each of the new helper
> functions, this will prevent these functions accidentally being called
> in the all_stop code path.
> 
> While moving the two `for (...)` loops I have replaced 'auto' with
> 'thread_info' for additional type clarity.
> 
> With the exception of the new has_execution check in the MI path there
> should be no other user visible changes with this commit.  I've added
> a new test which exposes the missing has_execution check issue.

Hi Andrew,

thanks for finding and fixing this.

The functional change is minimal (add one check), and uses a pattern 
already used elsewhere in the code, so LGTM.

Approved-By: Tom de Vries <tdevries@suse.de>

FWIW, while reviewing the patch I came to the hypothesis that there are 
really three parts:
- refactoring in infcmd.c
- minimal fix
- refactoring in exec_continue

To verify this, I split off the first two parts, and confirmed that this 
minimal fix (not showing the part removing proceed_thread):
...
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 8b6da41ffeb..a7e3845d6e3 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -279,7 +265,12 @@ exec_continue (const char *const *argv, int argc)
  	    }

  	  for (auto &thread : all_threads ())
-	    proceed_thread (&thread, pid);
+	    {
+	      if (pid != 0 && thread.ptid.pid () != pid)
+		continue;
+	      proceed_one_thread (thread);
+	    }
+
  	  disable_commit_resumed.reset_and_commit ();
  	}
        else
...
fixes the test-case failure.

I didn't like the escaping in the test-case much, so I wrote a patch 
fixing this (attached).  You could merge before committing, or I can do 
a follow-up commit, as you like.

Thanks,
- Tom