gdb: ensure normal stop finishes the thread state of all threads

This patch fixes a multi-target issue where the normal_stop function
can fail to finish the thread state of threads from a non current
target, this leaves the threads marked as running in GDB core, while
the threads is actually stopped.

For testing I used this test program:

  #include <unistd.h>

  int
  main ()
  {
    while (1)
      sleep (1);
    return 0;
  }

Compile this to make '/tmp/spin', then the bug can be shown using this
command:

  $ gdb -ex 'file /tmp/spin' \
        -ex 'start' \
	-ex 'add-inferior' \
	-ex 'inferior 2' \
	-ex 'set sysroot' \
	-ex 'target extended-remote | gdbserver --multi --once - /tmp/spin' \
        -ex 'inferior 1' \
	-ex 'continue&' \
	-ex 'inferior 2' \
	-ex 'search sleep' \
	-ex 'break $_ inferior 2' \
	-ex 'continue' \
	-ex 'info threads'

The interesting part of the output is:

    Id   Target Id                     Frame
    1.1  process 1610445 "spin"        (running)
  * 2.1  Thread 1610451.1610451 "spin" main () at spin.c:7
  (gdb)

Notice that thread 1.1 is marked as running when it should be
stopped.  We can see that the thread is actually stopped if we try
this:

  (gdb) inferior 1
  [Switching to inferior 1 [process 1610445] (/tmp/spin)]
  [Switching to thread 1.1 (process 1610445)](running)
  (gdb) continue
  Cannot execute this command while the selected thread is running.
  (gdb) interrupt
  (gdb) info threads
    Id   Target Id                     Frame
  * 1.1  process 1610445 "spin"        (running)
    2.1  Thread 1610451.1610451 "spin" main () at spin.c:7
  (gdb)

We can see the expected behaviour if both inferiors run on the same
target, like this:

  $ gdb -ex 'file /tmp/spin' \
        -ex 'start' \
	-ex 'add-inferior' \
	-ex 'inferior 2' \
	-ex 'file /tmp/spin' \
	-ex 'start' \
	-ex 'inferior 1' \
	-ex 'continue&' \
	-ex 'inferior 2' \
	-ex 'search sleep' \
	-ex 'break $_ inferior 2' \
	-ex 'continue' \
	-ex 'info threads'

The 'info threads' from this series of commands looks like this:

    Id   Target Id              Frame
    1.1  process 1611589 "spin" 0x00007ffff7e951e7 in nanosleep () from /lib64/libc.so.6
  * 2.1  process 1611593 "spin" main () at spin.c:7
  (gdb)

Now both threads are stopped as we'd expect.

The problem is in normal_stop.  The scoped_finish_thread_state uses
user_visible_resume_target to select the target(s) over which GDB will
iterate to find the threads to update.

The problem with this is that when the ptid_t is minus_one_ptid,
meaning all threads, user_visible_resume_target only returns nullptr,
meaning all targets, when sched_multi is true.

This dependency on sched_multi makes sense when _resuming_ threads.
If we are resuming all threads, then when sched_multi (the
schedule-multiple setting) is off (the default), all threads actually
means all threads in the current inferior only.  When sched_multi is
true (schedule-multiple is on) then this means all threads, from all
inferiors, which means GDB needs to consider every target.

However, when stopping an inferior in all-stop mode (non_stop is
false), then GDB wants to stop all threads from all inferiors,
regardless of the sched_multi setting.

What this means is that, when 'non_stop' is false, then we should be
passing nullptr as the target selection to scoped_finish_thread_state.

My proposal is that we should stop using user_visible_resume_target in
the normal_stop function for the target selection of the
scoped_finish_thread_state, instead we should manually figure out the
correct target value and pass this in.

There is precedent for this in GDB, see run_command_1, where
'finish_target' is calculated directly within the function rather than
using user_visible_resume_target.

After this commit, when using two different targets (native and
remote) as in my first example above, both threads will be correctly
stopped.

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 289c503963cfa2203d04a36d253e36b475a43241..eeef01ae66d565fdd0aa4fec59b219b458ee307d 100644 (file)
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -9525,6 +9525,7 @@ normal_stop ()
      here, so do this before any filtered output.  */
 
   ptid_t finish_ptid = null_ptid;
+  process_stratum_target *finish_target = nullptr;
 
   if (!non_stop)
     finish_ptid = minus_one_ptid;
@@ -9537,17 +9538,30 @@ normal_stop ()
         linux-fork.c automatically switches to another fork from
         within target_mourn_inferior.  */
       if (inferior_ptid != null_ptid)
-       finish_ptid = ptid_t (inferior_ptid.pid ());
+       {
+         finish_ptid = ptid_t (inferior_ptid.pid ());
+         finish_target = current_inferior ()->process_target ();
+       }
     }
   else if (last.kind () != TARGET_WAITKIND_NO_RESUMED
           && last.kind () != TARGET_WAITKIND_THREAD_EXITED)
-    finish_ptid = inferior_ptid;
+    {
+      finish_ptid = inferior_ptid;
+      finish_target = current_inferior ()->process_target ();
+    }
 
   std::optional<scoped_finish_thread_state> maybe_finish_thread_state;
   if (finish_ptid != null_ptid)
     {
-      maybe_finish_thread_state.emplace
-       (user_visible_resume_target (finish_ptid), finish_ptid);
+      /* It might be tempting to use user_visible_resume_target to compute
+        FINISH_TARGET from FINISH_PTID, however, that is the wrong choice
+        in this case.
+
+        When resuming, we only resume the current target unless
+        schedule-multiple is in effect.  However, when handling a stop, if
+        FINISH_PTID is minus_one_ptid, then we really do want to look for
+        stop events from _any_ target.  */
+      maybe_finish_thread_state.emplace (finish_target, finish_ptid);
     }
 
   /* As we're presenting a stop, and potentially removing breakpoints,

diff --git a/gdb/testsuite/gdb.multi/interrupt-bg-exec.c b/gdb/testsuite/gdb.multi/interrupt-bg-exec.c
new file mode 100644 (file)
index 0000000..b5fa568
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/interrupt-bg-exec.c
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2025 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/>.  */
+
+#include <unistd.h>
+
+volatile int wait_for_gdb = 1;
+
+void
+breakpt (void)
+{
+  /* Nothing.  */
+}
+
+void
+all_started (void)
+{
+  /* Nothing.  */
+}
+
+int
+main (void)
+{
+  alarm (360);
+
+  all_started ();
+
+  while (wait_for_gdb)
+    sleep (1);
+
+  breakpt ();
+
+  return 0;
+}

diff --git a/gdb/testsuite/gdb.multi/interrupt-bg-exec.exp b/gdb/testsuite/gdb.multi/interrupt-bg-exec.exp
new file mode 100644 (file)
index 0000000..065a112
--- /dev/null
+++ b/gdb/testsuite/gdb.multi/interrupt-bg-exec.exp
@@ -0,0 +1,143 @@
+# Copyright 2025 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 all-stop mode, set one inferior running in the background, then
+# continue a second inferior.  When the second inferior hits a breakpoint,
+# both inferiors should be stopped.
+
+# This tests the use of native and remote targets.  If we try to run
+# it with a board that forces native targets to become remote, then
+# this doesn't really make sense (for this test).
+require {string equal [target_info gdb_protocol] ""}
+
+source $srcdir/$subdir/multi-target.exp.tcl
+
+load_lib gdbserver-support.exp
+
+require allow_gdbserver_tests
+
+# This overrides the call in multi-target.exp.tcl.
+standard_testfile
+
+if { [build_executable "failed to build" $testfile $srcfile] } {
+    return
+}
+
+# Start two inferiors, TARGET_TYPE_1 and TARGET_TYPE_2 are strings, either
+# 'extended-remote' or 'native', and control the connection type of each
+# inferior.
+#
+# Set the first inferior running in the background, then continue theA
+# second inferior allowing it to hit a breakpoint.
+#
+# Once the breakpoint is hit, both inferiors should be stopped.
+proc run_test { target_type_1 target_type_2 } {
+    cleanup_gdbservers
+
+    clean_restart
+
+    gdb_test "disconnect" ".*"
+
+    gdb_test_no_output "set sysroot"
+
+    # multi-target depends on target running in non-stop mode.  Force it
+    # on for remote targets, until this is the default.
+    gdb_test_no_output "maint set target-non-stop on"
+
+    # Run in all-stop mode.
+    gdb_test_no_output "set non-stop off"
+
+    if {![add_inferior 2 $target_type_1 $::binfile]} {
+       return 0
+    }
+
+    if {![add_inferior 3 $target_type_2 $::binfile]} {
+       return 0
+    }
+
+    # Check we see all the expected threads.
+    gdb_test "info threads" \
+       [multi_line \
+            "\\s+Id\\s+Target Id\\s+Frame\\s*" \
+            "\\s+2\\.1\\s+\[^\r\n\]+" \
+            "\\*\\s+3\\.1\\s+\[^\r\n\]+"] \
+       "check expected threads exist"
+
+    # The breakpoint will be set in both inferiors, but only inferior 3
+    # will hit it as 'wait_for_gdb' is cleared only in that inferior.
+    gdb_breakpoint breakpt
+    gdb_test "thread apply 3.1 set wait_for_gdb = 0"
+
+    # Let inferior 2 run in the background.
+    gdb_test "thread 2.1"
+    gdb_test -no-prompt-anchor "continue&"
+
+    # Run inferior 3 until it hits a breakpoint.
+    gdb_test "thread 3.1"
+    gdb_test "continue" \
+       [multi_line \
+            "Thread 3\\.1 \[^\r\n\]+ hit Breakpoint \[^\r\n\]+, breakpt \\(\\) \[^\r\n\]+" \
+            "$::decimal\\s+\[^\r\n\]+"] \
+       "continue to breakpt function"
+
+    # Check the state of all threads.  None should be running.
+    set saw_inferior_2 false
+    set saw_inferior_3 false
+    gdb_test_multiple "info threads" "check threads after stop" {
+       -re "^info threads\r\n" {
+           exp_continue
+       }
+
+       -re "^\\s+Id\\s+Target Id\\s+Frame\\s*\r\n" {
+           exp_continue
+       }
+
+       -re "^\\s+2\\.1\\s+\[^\r\n\]+\\s+\\(running\\)\\s*\r\n" {
+           # Don't count this as seeing inferior 2 as the thread is
+           # incorrectly still marked as running.  By not setting the
+           # SAW_INFERIOR_2 flag this test will now fail.
+           exp_continue
+       }
+
+       -re "^\\s+2\\.1\\s+\[^\r\n\]+\r\n" {
+           set saw_inferior_2 true
+           exp_continue
+       }
+
+       -re "^\\*\\s+3\\.1\\s+\[^\r\n\]+\r\n" {
+           set saw_inferior_3 true
+           exp_continue
+       }
+
+       -re "^$::gdb_prompt $" {
+           gdb_assert { $saw_inferior_2 && $saw_inferior_3 } \
+               $gdb_test_name
+       }
+
+       -re "^\[^\r\n\]*\r\n" {
+           exp_continue
+       }
+    }
+}
+
+set all_target_types { extended-remote native }
+
+foreach_with_prefix target_type_1 $all_target_types {
+    foreach_with_prefix target_type_2 $all_target_types {
+       run_test $target_type_1 $target_type_2
+    }
+}
+
+multi_target_cleanup