Re: [PATCH 2/2] gdb: share some thread proceed related code between CLI and MI

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4845 bytes --]

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

[-- Attachment #2: 0001-gdb-testsuite-Make-gdb.mi-mi-corefile-and-live.exp-r.patch --]
[-- Type: text/x-patch, Size: 3293 bytes --]

From a88acf26e0346c57fa8275af4d0b26514873aa8a Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Fri, 7 Aug 2026 09:44:12 +0200
Subject: [PATCH] [gdb/testsuite] Make gdb.mi/mi-corefile-and-live.exp regexps
 more readable

---
 gdb/testsuite/gdb.mi/mi-corefile-and-live.exp | 38 ++++++++++---------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp b/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp
index a3a57d3c81a..1a34029d087 100644
--- a/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp
+++ b/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp
@@ -39,8 +39,8 @@ if {$corefile == ""} {
 
 # Start GDB in non-stop and schedule-multiple mode.
 save_vars { GDBFLAGS } {
-    append GDBFLAGS " -ex \"set non-stop on\""
-    append GDBFLAGS " -ex \"set schedule-multiple on\""
+    append GDBFLAGS { -ex "set non-stop on"}
+    append GDBFLAGS { -ex "set schedule-multiple on"}
     mi_clean_restart $::testfile
 }
 
@@ -56,42 +56,46 @@ mi_create_breakpoint "-g i1 foo" \
 
 # Setup inferior 2, this will load the core file.
 mi_gdb_test "-add-inferior" \
-    [multi_line "=thread-group-added,id=\"i2\"" \
-	 "~\"\\\[New inferior 2\\\]\\\\n\"" \
-	 "\~\"Added inferior 2\[^\r\n\]*\\\\n\"" \
-	 "\\^done,inferior=\"\[^\"\]+\"(?:,connection={.*})?" ] \
+    [quotemeta \
+	 [multi_line \
+	      {=thread-group-added,id="i2"} \
+	      {~"[New inferior 2]\n"} \
+	      {~"Added inferior 2@/[^\r\n]*/\n"} \
+	      {^done,inferior="@/[^"]+/"@/(?:,connection={.*})?/}]] \
     "add inferior 2"
 
 # Set the executable for inferior 2.
 mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \
-    "\\^done" \
+    [string_to_regexp "^done"] \
     "set executable of inferior 2"
 
 # Load the core file into inferior 2.
 mi_gdb_test \
     "-target-select --thread-group i2 core $::corefile" \
-    [multi_line \
-	 "=thread-group-started,id=\"i2\",.*" \
-	 "=thread-created,id=\"2\",group-id=\"i2\"" \
-	 ".*\\^connected,frame=.*"] \
+    [quotemeta \
+	 [multi_line \
+	      {=thread-group-started,id="i2",@...} \
+	      {=thread-created,id="2",group-id="i2"} \
+	      {@...^connected,frame=@...}]] \
     "load core file in inferior 2"
 
 # Check the core file thread is initially shown as stopped.
-mi_gdb_test "-thread-info 2" ".*,state=\"stopped\".*" \
+mi_gdb_test "-thread-info 2" {.*,state="stopped".*} \
     "core file thread is initially stopped"
 
 # Resume "all" threads.  As the core target doesn't support execution
 # this should not try to set the core target threads running.
 mi_gdb_test "-exec-continue --all" \
-    [multi_line \
-	 "\\^running" \
-	 "\\*running,thread-id=\"1\""] \
+    [string_to_regexp \
+	 [multi_line \
+	      "^running" \
+	      {*running,thread-id="1"}]] \
     "resume all"
 
 # Wait for the non-core target thread to stop.
 mi_expect_stop "breakpoint-hit" \
-    "foo" ".*" ".*" ".*" {"" "disp=\"keep\""} "w1,i2 stop"
+    "foo" ".*" ".*" ".*" {"" {disp="keep"}} "w1,i2 stop"
 
 # Check that the core target thread is still showing as stopped.
-mi_gdb_test "-thread-info 2" ".*,state=\"stopped\".*" \
+mi_gdb_test "-thread-info 2" {.*,state="stopped".*} \
     "core file thread is still stopped"

base-commit: 4df4712cb94f40463daf6a75c71a64eaf84e7901
-- 
2.51.0