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

Pedro Alves
2026-04-02T13:34:00Z

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}
> + }
> +}