[binutils-gdb] gdb: share some thread proceed related code between CLI and MI
Andrew Burgess via Gdb-cvs <[email protected]>
| Newsgroups | gmane.comp.gdb.cvs |
|---|---|
| Message-ID | <[email protected]> |
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=86331c2681c289d07672c9d006f6933f5317fbf4 commit 86331c2681c289d07672c9d006f6933f5317fbf4 Author: Andrew Burgess <[email protected]> Date: Thu Aug 6 11:31:54 2026 +0100 gdb: share some thread proceed related code between CLI and MI 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. Approved-By: Tom de Vries <[email protected]> Diff: --- gdb/infcmd.c | 62 +++++++++++------ gdb/inferior.h | 11 +++ gdb/mi/mi-main.c | 31 +++------ gdb/testsuite/gdb.mi/mi-corefile-and-live.c | 37 ++++++++++ gdb/testsuite/gdb.mi/mi-corefile-and-live.exp | 99 +++++++++++++++++++++++++++ 5 files changed, 197 insertions(+), 43 deletions(-) diff --git a/gdb/infcmd.c b/gdb/infcmd.c index 3e943123519..23e93587874 100644 --- a/gdb/infcmd.c +++ b/gdb/infcmd.c @@ -722,6 +722,46 @@ ensure_not_running (void) error_is_running (); } +/* See inferior.h. */ + +void +proceed_one_thread (thread_info &thread) +{ + gdb_assert (non_stop); + + if (thread.state () != THREAD_STOPPED) + return; + + if (!thread.inf->has_execution ()) + return; + + switch_to_thread (&thread); + clear_proceed_status (0); + proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); +} + +/* See inferior.h. */ + +void +proceed_all_threads () +{ + gdb_assert (non_stop); + + for (thread_info &thread : all_threads ()) + { + /* We go through all threads individually instead of compressing + into a single target `resume_all' request, because some threads + may be stopped in internal breakpoints/events, or stopped waiting + for its turn in the displaced stepping queue (that is, they are + running from the user's perspective but internally stopped). The + target side has no idea about why the thread is stopped, so a + `resume_all' command would resume too much. If/when GDB gains a + way to tell the target `hold this thread stopped until I say + otherwise', then we can optimize this. */ + proceed_one_thread (thread); + } +} + void continue_1 (bool all_threads_p) { @@ -739,27 +779,7 @@ continue_1 (bool all_threads_p) scoped_disable_commit_resumed disable_commit_resumed ("continue all threads in non-stop"); - for (auto &thread : all_threads ()) - { - /* We go through all threads individually instead of compressing - into a single target `resume_all' request, because some threads - may be stopped in internal breakpoints/events, or stopped waiting - for its turn in the displaced stepping queue (that is, they are - running from the user's perspective but internally stopped). The - target side has no idea about why the thread is stopped, so a - `resume_all' command would resume too much. If/when GDB gains a - way to tell the target `hold this thread stopped until I say - otherwise', then we can optimize this. */ - if (thread.state () != THREAD_STOPPED) - continue; - - if (!thread.inf->has_execution ()) - continue; - - switch_to_thread (&thread); - clear_proceed_status (0); - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); - } + proceed_all_threads (); if (current_ui->prompt_state == PROMPT_BLOCKED) { diff --git a/gdb/inferior.h b/gdb/inferior.h index 217741adc07..8255654d9b8 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -227,6 +227,17 @@ extern void registers_info (const char *, bool); extern void continue_1 (bool all_threads_p); +/* For use only when non_stop is true. Proceed all threads in every + inferior. */ + +extern void proceed_all_threads (); + +/* For use only when non_stop is true. If THREAD is stopped, and is in an + inferior that has_execution then switch to THREAD, clear its proceed + status, and proceed the thread. */ + +extern void proceed_one_thread (thread_info &thread); + extern void interrupt_target_1 (bool all_threads); using delete_longjmp_breakpoint_cleanup diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 5d1aeb87d43..d462ab9ac2b 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -235,20 +235,6 @@ mi_cmd_exec_jump (const char *args, const char *const *argv, int argc) mi_execute_async_cli_command ("jump", argv, argc); } -static void -proceed_thread (struct thread_info *thread, int pid) -{ - if (thread->state () != THREAD_STOPPED) - return; - - if (pid != 0 && thread->ptid.pid () != pid) - return; - - switch_to_thread (thread); - clear_proceed_status (0); - proceed ((CORE_ADDR) -1, GDB_SIGNAL_DEFAULT); -} - static void exec_continue (const char *const *argv, int argc) { @@ -262,24 +248,25 @@ exec_continue (const char *const *argv, int argc) all threads in all inferiors, we need to iterate over threads. - See comment on infcmd.c:proceed_thread_callback for rationale. */ + See comment in infcmd.c:proceed_all_threads for rationale. */ if (current_context->all || current_context->thread_group != -1) { scoped_restore_current_thread restore_thread; scoped_disable_commit_resumed disable_commit_resumed ("MI continue all threads in non-stop"); - int pid = 0; + inferior *inf = nullptr; if (!current_context->all) - { - struct inferior *inf - = find_inferior_id (current_context->thread_group); + inf = find_inferior_id (current_context->thread_group); - pid = inf->pid; + if (inf == nullptr) + proceed_all_threads (); + else + { + for (thread_info &thread : inf->threads ()) + proceed_one_thread (thread); } - for (auto &thread : all_threads ()) - proceed_thread (&thread, pid); disable_commit_resumed.reset_and_commit (); } else diff --git a/gdb/testsuite/gdb.mi/mi-corefile-and-live.c b/gdb/testsuite/gdb.mi/mi-corefile-and-live.c new file mode 100644 index 00000000000..831c7f0de7f --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-corefile-and-live.c @@ -0,0 +1,37 @@ +/* Copyright 2026 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <stdlib.h> + +void +bar (void) +{ + abort (); +} + +void +foo (void) +{ + bar (); +} + +int +main (int argc, char **argv) +{ + foo (); + return 0; +} diff --git a/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp b/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp new file mode 100644 index 00000000000..9568562591a --- /dev/null +++ b/gdb/testsuite/gdb.mi/mi-corefile-and-live.exp @@ -0,0 +1,99 @@ +# Copyright 2026 Free Software Foundation, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# In non-stop mode with schedule-multiple turned on, create two +# inferiors, a core target and one other. Then use the command +# '-exec-continue --all'. +# +# The core target threads should not be marked as running by this. + +load_lib mi-support.exp +set MIFLAGS "-i=mi" + +require isnative +require allow_multi_inferior_tests + +standard_testfile + +if {[build_executable "build executable" $testfile $srcfile] == -1} { + return +} + +set corefile [core_find $binfile] +if {$corefile == ""} { + untested "unable to create or find corefile" + return +} + +# 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" } + mi_clean_restart $::testfile +} + +if {[mi_runto_main] == -1} { + return +} + +# Arrange for inferior 1, our non core file inferior, to stop before +# it hits the abort call. +mi_create_breakpoint "-g i1 foo" \ + "set breakpoint on foo" \ + -inferior 1 + +# 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={.*})?" ] \ + "add inferior 2" + +# Set the executable for inferior 2. +mi_gdb_test "-file-exec-and-symbols --thread-group i2 $::binfile" \ + [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=.*"] \ + "load core file in inferior 2" + +# Check the core file thread is initially shown as 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" \ + [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" + +# Check that the core target thread is still showing as stopped. +mi_gdb_test "-thread-info 2" {.*,state="stopped".*} \ + "core file thread is still stopped"