[PATCH v2 04/47] infrun: Split currently_stepping, fix sw watchpoints issue

[Pedro Alves <pedro@palves.net>]

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: I7b07bc62e8570333d2e4856d2e55ae6e58f8260c
---
 gdb/gdbthread.h                               |  4 +
 gdb/infrun.c                                  | 28 +++---
 .../sw-watchpoint-step-over-bp-with-threads.c | 64 +++++++++++++
 ...w-watchpoint-step-over-bp-with-threads.exp | 91 +++++++++++++++++++
 4 files changed, 173 insertions(+), 14 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c
 create mode 100644 gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index c561e9a7b64..a8fd967c702 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 119bd151034..9f625a7bd44 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -87,7 +87,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 &);
 
@@ -2656,7 +2656,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);
@@ -2685,7 +2685,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)
     {
@@ -3060,7 +3060,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);
 	}
     }
 
@@ -5041,7 +5041,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);
@@ -5368,7 +5368,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);
     }
 }
@@ -5563,7 +5563,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);
 	}
     }
 
@@ -6668,7 +6668,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 ());
@@ -6795,7 +6795,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
@@ -7204,7 +7204,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
@@ -8632,12 +8632,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..7f014036c9b
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.c
@@ -0,0 +1,64 @@
+/* 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 <pthread.h>
+#include <assert.h>
+#include <unistd.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;
+
+  alarm (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..b45db57e80f
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/sw-watchpoint-step-over-bp-with-threads.exp
@@ -0,0 +1,91 @@
+# 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/>.
+
+# 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 $::binfile
+    }
+
+    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}
+    }
+}
-- 
2.49.0