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

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> 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, I merged everything except the quotemeta related changes.  Not a
huge fan, and in this case the imbalanced quotes was mucking up the
syntax highlighting in my editor.  I know, that's a "me" problem, but I
really don't want to push a patch that makes my life harder.

I'll hold off pushing this until we've discussed your other review email
about this change.

Thanks,
Andrew




>
> Thanks,
> - Tom
> 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