[binutils-gdb] infrun: Split currently_stepping, fix sw watchpoints issue

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=a65981ee2e6edb9db11d80f19997c648fd5a09a0

commit a65981ee2e6edb9db11d80f19997c648fd5a09a0
Author: Pedro Alves <[email protected]>
Date:   Fri May 16 21:05:17 2025 +0100

    infrun: Split currently_stepping, fix sw watchpoints issue
    
    The gdb.base/watchpoint.exp on Windows with non-stop support added
    (later in the series) exposed an issue with the currently_stepping
    logic when tested with software watchpoints.
    
    The issue only happens when:
    
     - You have multiple threads.  gdb.base/watchpoint.exp exposed it on
       Windows because there the OS always spawns a few extra threads.
    
     - Displaced stepping is not available.  The Windows non-stop work
       does not implement displaced stepping yet.  That is left as an
       optimization for later.
    
     - The target backend is working in non-stop mode.
    
    I've written a new test that exposes the issue on GNU/Linux as well
    (on hardware single-step targets, like x86-64).  There, we see:
    
     continue
     Continuing.
     ../../src/gdb/infrun.c:2918: internal-error: resume_1: Assertion `!(thread_has_single_step_breakpoints_set (tp) && step)' failed.
     A problem internal to GDB has been detected,
     further debugging may prove unreliable.
     ----- Backtrace -----
     FAIL: gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp: target-non-stop=on: displaced-stepping=off: continue until exit (GDB internal error)
    
    Currently, software watchpoints are implemented by forcing
    single-stepping.  That is done by currently_stepping returning true
    when we have a software watchpoint.  proceed calls resume, which calls
    resume_1, which then ends up always requesting a single-step resume,
    even if the higher layers wanted a continue.
    
    Now, if you set a software watchpoint, and then continue the program,
    and there's a breakpoint at the current PC, GDB needs to step over
    that breakpoint first.  If displaced stepping is not available, then
    GDB temporarily pauses all threads, removes the breakpoint,
    single-steps the thread that needs to move past the breakpoint, and
    then finally, reinserts the breakpoint, and restarts all threads
    again.  That last restarting step happens in the restart_threads
    infrun function.
    
    restart_threads iterates over all threads trying to restart them one
    by one.  There, we have:
    
          if (currently_stepping (tp))
            {
              infrun_debug_printf ("restart threads: [%s] was stepping",
                                   tp->ptid.to_string ().c_str ());
    
    but, what if TP is actually a new thread that hasn't yet ever set
    stepping?  currently_stepping still returns true, due to the software
    watchpoint, and we end up in keep_going_stepped_thread, here:
    
      if (tp->stop_pc () != tp->prev_pc)
        {
          ptid_t resume_ptid;
    
          infrun_debug_printf ("expected thread advanced also (%s -> %s)",
                               paddress (current_inferior ()->arch (), tp->prev_pc),
                               paddress (current_inferior ()->arch (),
                                         tp->stop_pc ()));
    
    ... because prev_pc was stale at that point (we had no reason to
    update it earlier).  E.g. on Windows we see something like:
    
      [infrun] restart_threads: start: event_thread=1867996.1867996.0, inf=-1
        [infrun] restart_threads: restart threads: [1867996.1867996.0] is event thread
        [infrun] restart_threads: restart threads: [1867996.1868003.0] was stepping
        [infrun] keep_going_stepped_thread: resuming previously stepped thread
        [infrun] keep_going_stepped_thread: expected thread advanced also (0 -> 0x7ffff7ce57f8)
        [infrun] clear_step_over_info: clearing step over info
        [infrun] do_target_resume: resume_ptid=1867996.1868003.0, step=0, sig=GDB_SIGNAL_0
    
    On GNU/Linux, we may see:
    
        [infrun] keep_going_stepped_thread: expected thread advanced also (0x7ffff7d2683d -> 0x7ffff7ce57f8)
    
    there prev_pc might have been updated on an earlier proceed call,
    which makes the issue harder to see, but it is stale too here.
    
    That means we insert a single-step breakpoint at the current PC, and
    continue the thread, with target_resume directly, asking for a normal
    continue.
    
    Eventually, something causes a user-visible stop.  For example, the
    software watchpoint triggers.  That makes GDB stop all threads.
    
    Now, if the user re-resumes the program, say, with "continue", we fail
    this assertion in resume_1 coming from proceed:
    
      /* If STEP is set, it's a request to use hardware stepping
         facilities.  But in that case, we should never
         use singlestep breakpoint.  */
      gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step));
    
    "step" is true because currently_stepping returns true since we have a
    software watchpoint.  And the thread has a single-step breakpoint
    installed from earlier, because of that code mentioned above, in
    keep_going_stepped_thread reached from restart_threads.
    
    This patch fixes the root cause -- the currently_stepping call in
    restart_threads returned true for a thread that has never set stepping
    in the first place.  This is because currently_stepping really serves
    two purposes currently:
    
      #1 - for a thread that we are about to resume, should we set it
           stepping?
    
      #2 - for a thread that just stopped, was it stepping before?
    
    The fix is thus to decouple those two aspects:
    
      - for #1, we simply rename currently_stepping to should_step.
    
      - for #2, we record whether the thread was stepping before in a new
        currently_stepping flag in thread_info.
    
    As mentioned, there's a new testcase included.  I tested this on
    x86-64 GNU/Linux, native and gdbserver, and on Windows x64 with the
    non-stop series.  The assertion triggers on all of those with the fix,
    and is fixed by this patch on all of those, too.
    
    Change-Id: Id7aa00692531450695771f8110893cc25626262f

Diff:
---
 gdb/gdbthread.h                                    |  4 +
 gdb/infrun.c                                       | 28 +++----
 .../sw-watchpoint-step-over-bp-with-threads.c      | 65 ++++++++++++++++
 .../sw-watchpoint-step-over-bp-with-threads.exp    | 91 ++++++++++++++++++++++
 4 files changed, 174 insertions(+), 14 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 68d7aebbbf7..7644924b614 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -152,6 +152,10 @@ struct thread_control_state
      the finished single step.  */
   int trap_expected = 0;
 
+  /* True if the thread TP is in the middle of (software or hardware)
+     single-stepping.  */
+  bool currently_stepping = false;
+
   /* Nonzero if the thread is being proceeded for a "finish" command
      or a similar situation when return value should be printed.  */
   int proceed_to_finish = 0;
diff --git a/gdb/infrun.c b/gdb/infrun.c
index 14fc56976e2..2e53d2a6c30 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -86,7 +86,7 @@ static void sig_print_header (void);
 
 static void follow_inferior_reset_breakpoints (void);
 
-static bool currently_stepping (struct thread_info *tp);
+static bool should_step (thread_info *tp);
 
 static void insert_hp_step_resume_breakpoint_at_frame (const frame_info_ptr &);
 
@@ -2676,7 +2676,7 @@ resume_1 (enum gdb_signal sig)
 	 "status %s (currently_stepping=%d).",
 	 tp->ptid.to_string ().c_str (),
 	 tp->pending_waitstatus ().to_string ().c_str (),
-	 currently_stepping (tp));
+	 tp->control.currently_stepping);
 
       tp->inf->process_target ()->threads_executing = true;
       tp->set_resumed (true);
@@ -2705,7 +2705,7 @@ resume_1 (enum gdb_signal sig)
   tp->stepped_breakpoint = 0;
 
   /* Depends on stepped_breakpoint.  */
-  step = currently_stepping (tp);
+  step = tp->control.currently_stepping = should_step (tp);
 
   if (current_inferior ()->thread_waiting_for_vfork_done != nullptr)
     {
@@ -3081,7 +3081,7 @@ clear_proceed_status_thread (struct thread_info *tp)
 	    ("thread %s has pending wait status %s (currently_stepping=%d).",
 	     tp->ptid.to_string ().c_str (),
 	     tp->pending_waitstatus ().to_string ().c_str (),
-	     currently_stepping (tp));
+	     tp->control.currently_stepping);
 	}
     }
 
@@ -5054,7 +5054,7 @@ adjust_pc_after_break (struct thread_info *thread,
 	 we also need to back up to the breakpoint address.  */
 
       if (thread_has_single_step_breakpoints_set (thread)
-	  || !currently_stepping (thread)
+	  || !thread->control.currently_stepping
 	  || (thread->stepped_breakpoint
 	      && thread->prev_pc == breakpoint_pc))
 	regcache_write_pc (regcache, breakpoint_pc);
@@ -5381,7 +5381,7 @@ save_waitstatus (struct thread_info *tp, const target_waitstatus &ws)
 	       && software_breakpoint_inserted_here_p (aspace, pc))
 	tp->set_stop_reason (TARGET_STOPPED_BY_SW_BREAKPOINT);
       else if (!thread_has_single_step_breakpoints_set (tp)
-	       && currently_stepping (tp))
+	       && tp->control.currently_stepping)
 	tp->set_stop_reason (TARGET_STOPPED_BY_SINGLE_STEP);
     }
 }
@@ -5576,7 +5576,7 @@ handle_one (const wait_one_event &event)
 			       paddress (current_inferior ()->arch (),
 					 t->stop_pc ()),
 			       t->ptid.to_string ().c_str (),
-			       currently_stepping (t));
+			       t->control.currently_stepping);
 	}
     }
 
@@ -6683,7 +6683,7 @@ restart_threads (struct thread_info *event_thread, inferior *inf)
 			  tp.ptid.to_string ().c_str ());
 	}
 
-      if (currently_stepping (&tp))
+      if (tp.control.currently_stepping)
 	{
 	  infrun_debug_printf ("restart threads: [%s] was stepping",
 			       tp.ptid.to_string ().c_str ());
@@ -6810,7 +6810,7 @@ finish_step_over (struct execution_control_state *ecs)
 			       paddress (current_inferior ()->arch (),
 					 tp->stop_pc ()),
 			       tp->ptid.to_string ().c_str (),
-			       currently_stepping (tp));
+			       tp->control.currently_stepping);
 
 	  /* This in-line step-over finished; clear this so we won't
 	     start a new one.  This is what handle_signal_stop would
@@ -7230,7 +7230,7 @@ handle_signal_stop (struct execution_control_state *ecs)
   /* If not, perhaps stepping/nexting can.  */
   if (random_signal)
     random_signal = !(ecs->event_thread->stop_signal () == GDB_SIGNAL_TRAP
-		      && currently_stepping (ecs->event_thread));
+		      && ecs->event_thread->control.currently_stepping);
 
   /* Perhaps the thread hit a single-step breakpoint of _another_
      thread.  Single-step breakpoints are transparent to the
@@ -8658,12 +8658,12 @@ keep_going_stepped_thread (struct thread_info *tp)
   return true;
 }
 
-/* Is thread TP in the middle of (software or hardware)
-   single-stepping?  (Note the result of this function must never be
-   passed directly as target_resume's STEP parameter.)  */
+/* Should thread TP be stepped (software or hardware)?  (Note the
+   result of this function must never be passed directly as
+   target_resume's STEP parameter.)  */
 
 static bool
-currently_stepping (struct thread_info *tp)
+should_step (thread_info *tp)
 {
   return ((tp->control.step_range_end
 	   && tp->control.step_resume_breakpoint == nullptr)
diff --git a/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c
new file mode 100644
index 00000000000..f05304749e2
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c
@@ -0,0 +1,65 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2025-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/>.  */
+
+#include <pthread.h>
+#include <assert.h>
+#include <unistd.h>
+#include "gdb_watchdog.h"
+
+static pthread_barrier_t threads_started_barrier;
+
+static void *
+thread_func (void *arg)
+{
+  pthread_barrier_wait (&threads_started_barrier);
+
+  while (1)
+    sleep (1);
+
+  return NULL;
+}
+
+static void
+dummy ()
+{
+}
+
+static unsigned watched_global = 0;
+
+int
+main (void)
+{
+  pthread_t thread;
+  int ret;
+
+  gdb_watchdog (30);
+
+  pthread_barrier_init (&threads_started_barrier, NULL, 2);
+
+  ret = pthread_create (&thread, NULL, thread_func, NULL);
+  assert (ret == 0);
+
+  /* Make sure all threads are scheduled before hitting the
+     breakpoint.  */
+  pthread_barrier_wait (&threads_started_barrier);
+
+  ++watched_global; /* break here start */
+
+  dummy (); /* just so there's extra code here */
+
+  return 0; /* break here end */
+}
diff --git a/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp
new file mode 100644
index 00000000000..3dac2b85987
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp
@@ -0,0 +1,91 @@
+# Copyright 2025-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/>.
+
+# Test continuing with a software watchpoint installed, when there are
+# multiple threads, and previously we stepped over a breakpoint.
+#
+# This is a regression test for a GDB bug where stepping over a
+# breakpoint in-line made GDB insert a software single-step breakpoint
+# in some threads by mistake, which later would cause an assertion to
+# fail.
+#
+# The issue only triggered when:
+#
+#  - The program has multiple threads.
+#  - The target backend is working in non-stop mode.
+#  - Displaced stepping is not available.
+#  - The target supports hardware single-step.
+#
+# However, we exercise more combinations for completeness.
+
+standard_testfile .c
+
+if { [build_executable "failed to prepare" $testfile $srcfile \
+	  {debug pthreads}] \
+	 == -1 } {
+    return
+}
+
+proc test {target-non-stop displaced-stepping} {
+
+    save_vars ::GDBFLAGS {
+	append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-stop}\""
+	append ::GDBFLAGS " -ex \"set displaced ${displaced-stepping}\""
+	clean_restart $::testfile
+    }
+
+    if {![runto_main]} {
+	return
+    }
+
+    # Run to a breakpoint, and leave it installed, so that GDB needs
+    # to step over it before continuing.
+    gdb_breakpoint [gdb_get_line_number "break here start"]
+    gdb_continue_to_breakpoint "started"
+
+    # GDB should know about at least two threads by now.
+    gdb_test "p \$_inferior_thread_count >= 2" " = 1"
+
+    # Set a software watchpoint.  This makes GDB single-step all
+    # instructions when we next continue.
+    gdb_test_no_output "set can-use-hw-watchpoints 0"
+    gdb_test "watch watched_global" "Watchpoint $::decimal: watched_global"
+
+    # Continue with the software watchpoint in place.  In the original
+    # bug, with displaced stepping disabled, this would make GDB
+    # incorrectly install a software single-step breakpoint on threads
+    # other than the main one.
+    gdb_test_multiple "cont" "continue to watchpoint" {
+	-re -wrap "Continuing.*Watchpoint.*watched_global.*Old value = 0.*New value = 1.*" {
+	    pass $gdb_test_name
+	}
+    }
+
+    # The final continue, with the software watchpoint set, so that
+    # GDB single-steps all threads (if the target is non-stop).  With
+    # the buggy GDB, the non-main thread had a software single-step
+    # breakpoint set, and on hardware single-step targets, GDB would
+    # fail an assertion that checks that we never ask the target to
+    # hardware single-step a thread when we have a software
+    # single-step breakpoint set for that thread.
+    gdb_breakpoint [gdb_get_line_number "break here end"]
+    gdb_continue_to_breakpoint "end"
+}
+
+foreach_with_prefix target-non-stop {auto on off} {
+    foreach_with_prefix displaced-stepping {auto on off} {
+	test ${target-non-stop} ${displaced-stepping}
+    }
+}