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

[Pedro Alves <pedro@palves.net>]

FYI, I've merged this one too.

Pedro Alves

On 2025-05-19 14:22, Pedro Alves wrote:
> 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}
> +    }
> +}