From: Pedro Alves Date: Fri, 16 May 2025 20:05:17 +0000 (+0100) Subject: infrun: Split currently_stepping, fix sw watchpoints issue X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b7b95b2ced253c9b87ed55a14b054049058f315b;p=thirdparty%2Fbinutils-gdb.git 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 --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 0e501d8515f..70a799a8269 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 &); @@ -2655,7 +2655,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); @@ -2684,7 +2684,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) { @@ -3059,7 +3059,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); } } @@ -5040,7 +5040,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); @@ -5367,7 +5367,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); } } @@ -5562,7 +5562,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); } } @@ -6667,7 +6667,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 ()); @@ -6794,7 +6794,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 @@ -7203,7 +7203,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 @@ -8631,12 +8631,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 . */ + +#include +#include +#include + +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 . + +# 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} + } +}