From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 1551) id 96A6E3858D37; Wed, 20 Jul 2022 14:10:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 96A6E3858D37 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Pedro Alves To: gdb-cvs@sourceware.org Subject: [binutils-gdb] Don't stop all threads prematurely after first step of "step N" X-Act-Checkin: binutils-gdb X-Git-Author: Pedro Alves X-Git-Refname: refs/heads/master X-Git-Oldrev: 1bc99604e84163fc3d749866ee9819d5aa22c28e X-Git-Newrev: e0c01ce66d0215d87d1173003eb6104d3f62bcdf Message-Id: <20220720141045.96A6E3858D37@sourceware.org> Date: Wed, 20 Jul 2022 14:10:45 +0000 (GMT) X-BeenThere: gdb-cvs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jul 2022 14:10:45 -0000 https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3De0c01ce66d02= 15d87d1173003eb6104d3f62bcdf commit e0c01ce66d0215d87d1173003eb6104d3f62bcdf Author: Pedro Alves Date: Mon Jul 18 18:22:15 2022 +0100 Don't stop all threads prematurely after first step of "step N" =20 In all-stop mode, when the target is itself in non-stop mode (like GNU/Linux), if you use the "step N" (or "stepi/next/nexti N") to step a thread a number of times: =20 (gdb) help step step, s Step program until it reaches a different source line. Usage: step [N] Argument N means step N times (or till program stops for another reaso= n). =20 ... GDB prematurely stops all threads after the first step, and doesn't re-resume them for the subsequent N-1 steps. It's as if for the 2nd and subsequent steps, the command was running with scheduler-locking enabled. =20 This can be observed with the testcase added by this commit, which looks like this: =20 static pthread_barrier_t barrier; =20 static void * thread_func (void *arg) { pthread_barrier_wait (&barrier); return NULL; } =20 int main () { pthread_t thread; int ret; =20 pthread_barrier_init (&barrier, NULL, 2); =20 /* We run to this line below, and then issue "next 3". That should step over the 3 lines below and land on the return statement. If GDB prematurely stops the thread_func thread after the first of the 3 nexts (and never resumes it again), then the join won't ever return. */ pthread_create (&thread, NULL, thread_func, NULL); /* set break here= */ pthread_barrier_wait (&barrier); pthread_join (thread, NULL); =20 return 0; } =20 The test hangs and times out without the GDB fix: =20 (gdb) next 3 [New Thread 0x7ffff7d89700 (LWP 525772)] FAIL: gdb.threads/step-N-all-progress.exp: non-stop=3Doff: target-non-= stop=3Don: next 3 (timeout) =20 The problem is a core gdb bug. =20 When you do "step/stepi/next/nexti N", GDB internally creates a thread_fsm object and associates it with the stepping thread. For the stepping commands, the FSM's class is step_command_fsm. That object is what keeps track of how many steps are left to make. When one step finishes, handle_inferior_event calls stop_waiting and returns, and then fetch_inferior_event calls the "should_stop" method of the event thread's FSM. The implementation of that method decrements the steps-left counter. If the counter is 0, it returns true and we proceed to presenting the stop to the user. If it isn't 0 yet, then the method returns false, indicating to fetch_inferior_event to "keep going". =20 Focusing now on when the first step finishes -- we're in "all-stop" mode, with the target in non-stop mode. When a step finishes, handle_inferior_event calls stop_waiting, which itself calls stop_all_threads to stop everything. I.e., after the first step completes, all threads are stopped, before handle_inferior_event returns. And after that, now in fetch_inferior_event, we consult the thread's thread_fsm::should_stop, which as we've seen, for the first step returns false -- i.e., we need to keep_going for another step. However, since the target is in non-stop mode, keep_going resumes _only_ the current thread. All the other threads remain stopped, inadvertently. =20 If the target is in non-stop mode, we don't actually need to stop all threads right after each step finishes, and then re-resume them again. We can instead defer stopping all threads until all the steps are completed. =20 So fix this by delaying the stopping of all threads until after we called the FSM's "should_stop" method. I.e., move it from stop_waiting, to handle_inferior_events's callers, fetch_inferior_event and wait_for_inferior. =20 New test included. Tested on x86-64 GNU/Linux native and gdbserver. =20 Change-Id: Iaad50dcfea4464c84bdbac853a89df92ade6ae01 Diff: --- gdb/infrun.c | 19 ++++++-- gdb/testsuite/gdb.threads/step-N-all-progress.c | 51 ++++++++++++++++++++ gdb/testsuite/gdb.threads/step-N-all-progress.exp | 59 +++++++++++++++++++= ++++ 3 files changed, 124 insertions(+), 5 deletions(-) diff --git a/gdb/infrun.c b/gdb/infrun.c index a6694230d29..a59cbe64537 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3971,6 +3971,16 @@ prepare_for_detach (void) } } =20 +/* If all-stop, but there exists a non-stop target, stop all threads + now that we're presenting the stop to the user. */ + +static void +stop_all_threads_if_all_stop_mode () +{ + if (!non_stop && exists_non_stop_target ()) + stop_all_threads ("presenting stop to user in all-stop"); +} + /* Wait for control to return from inferior to debugger. =20 If inferior gets a signal, we may decide to start it up again @@ -4017,6 +4027,8 @@ wait_for_inferior (inferior *inf) break; } =20 + stop_all_threads_if_all_stop_mode (); + /* No error, don't finish the state yet. */ finish_state.release (); } @@ -4240,6 +4252,8 @@ fetch_inferior_event () bool should_notify_stop =3D true; int proceeded =3D 0; =20 + stop_all_threads_if_all_stop_mode (); + clean_up_just_stopped_threads_fsms (ecs); =20 if (thr !=3D nullptr && thr->thread_fsm () !=3D nullptr) @@ -8138,11 +8152,6 @@ stop_waiting (struct execution_control_state *ecs) =20 /* Let callers know we don't want to wait for the inferior anymore. */ ecs->wait_some_more =3D 0; - - /* If all-stop, but there exists a non-stop target, stop all - threads now that we're presenting the stop to the user. */ - if (!non_stop && exists_non_stop_target ()) - stop_all_threads ("presenting stop to user in all-stop"); } =20 /* Like keep_going, but passes the signal to the inferior, even if the diff --git a/gdb/testsuite/gdb.threads/step-N-all-progress.c b/gdb/testsuit= e/gdb.threads/step-N-all-progress.c new file mode 100644 index 00000000000..8d3fbebfa07 --- /dev/null +++ b/gdb/testsuite/gdb.threads/step-N-all-progress.c @@ -0,0 +1,51 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2022 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 barrier; + +static void * +thread_func (void *arg) +{ + pthread_barrier_wait (&barrier); + return NULL; +} + +int +main () +{ + pthread_t thread; + int ret; + + alarm (30); + + pthread_barrier_init (&barrier, NULL, 2); + + /* We run to this line below, and then issue "next 3". That should + step over the 3 lines below and land on the return statement. If + GDB prematurely stops the thread_func thread after the first of + the 3 nexts (and never resumes it again), then the join won't + ever return. */ + pthread_create (&thread, NULL, thread_func, NULL); /* set break here */ + pthread_barrier_wait (&barrier); + pthread_join (thread, NULL); + + return 0; +} diff --git a/gdb/testsuite/gdb.threads/step-N-all-progress.exp b/gdb/testsu= ite/gdb.threads/step-N-all-progress.exp new file mode 100644 index 00000000000..b1d1ba75b67 --- /dev/null +++ b/gdb/testsuite/gdb.threads/step-N-all-progress.exp @@ -0,0 +1,59 @@ +# Copyright 2022 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 that with "next N", all threads are resumed at each of the N +# steps. GDB used to have a bug where in target-non-stop targets, and +# in all-stop mode, after the first "next" (or "step/stepi/nexti"), +# GDB would prematurely stop all threads. For the subsequent N-1 +# steps, only the stepped thread would continue running, while all the +# other threads would remain stopped. + +standard_testfile + +if { [build_executable "failed to prepare" $testfile \ + $srcfile {debug pthreads}] =3D=3D -1 } { + return +} + +proc test {non-stop target-non-stop} { + save_vars ::GDBFLAGS { + append ::GDBFLAGS " -ex \"maintenance set target-non-stop ${target-non-st= op}\"" + append ::GDBFLAGS " -ex \"set non-stop ${non-stop}\"" + clean_restart $::binfile + } + + if { ![runto_main] } { + return + } + + set lineno [gdb_get_line_number "set break here"] + + gdb_breakpoint $lineno + + gdb_continue_to_breakpoint "break here" + + gdb_test "next 3" "return 0;" +} + +foreach_with_prefix non-stop {off on} { + foreach_with_prefix target-non-stop {off on} { + if {${non-stop} =3D=3D "on" && ${target-non-stop} =3D=3D "off"} { + # Invalid combination. + continue + } + + test ${non-stop} ${target-non-stop} + } +}