Re: [PATCH] gdb: Don't ignore all SIGSTOP when the signal handler is set to pass

Andrew Burgess
2019-09-26T23:06:21Z

Ping!

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-09-10 18:37:57 +0100]:

> It was observed that in a multi-threaded application on GNU/Linux,
> that if the user has set the SIGSTOP to be pass (using GDB's handle
> command) then the inferior would hang upon hitting a breakpoint.
>
> What happens is that when a thread hits the breakpoint GDB tries to
> stop all of the other threads by sending them a SIGSTOP and setting
> the stop_requested flag in the target_ops structure - this can be seen
> in infrun.c:stop_all_threads.
>
> GDB then waits for all of the other threads to stop.
>
> When the SIGSTOP event arrives we eventually end up in
> linux-nat.c:linux_nat_filter_event, which has the job of deciding if
> the event we're looking at (the SIGSTOP arriving in this case) is
> something that should be reported back to the core of GDB.
>
> One of the final actions of this function is to check if we stopped
> due to a signal, and if we did, and the signal has been set to 'pass'
> by the user then we ignore the event and resume the thread.
>
> This code already has some conditions in place that mean the event is
> reported to GDB even if the signal is in the set of signals to be
> passed to the inferior.
>
> In this commit I extend this condition to such that:
>
> If the signal is a SIGSTOP, and the thread's stop_requested flag is
> set (indicating we're waiting for the thread to stop with a SIGSTOP)
> then we should report this SIGSTOP to GDB and not pass it to the
> inferior.
>
> With this change in place the test now passes. Regression tested on
> x86-64 GNU/Linux with no regressions.
>
> gdb/ChangeLog:
>
> * linux-nat.c (linux_nat_filter_event): Don't ignore SIGSTOP if we
> have just sent the thread a SIGSTOP and are waiting for it to
> arrive.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.threads/stop-with-handle.c: New file.
> * gdb.threads/stop-with-handle.exp: New file.
> ---
> gdb/ChangeLog | 6 +++
> gdb/linux-nat.c | 5 +-
> gdb/testsuite/ChangeLog | 5 ++
> gdb/testsuite/gdb.threads/stop-with-handle.c | 70 ++++++++++++++++++++++++++
> gdb/testsuite/gdb.threads/stop-with-handle.exp | 52 +++++++++++++++++++
> 5 files changed, 137 insertions(+), 1 deletion(-)
> create mode 100644 gdb/testsuite/gdb.threads/stop-with-handle.c
> create mode 100644 gdb/testsuite/gdb.threads/stop-with-handle.exp
>
> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index 945c19f6668..5cf5c57c043 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -3150,9 +3150,12 @@ linux_nat_filter_event (int lwpid, int status)
>
> /* When using hardware single-step, we need to report every signal.
> Otherwise, signals in pass_mask may be short-circuited
> - except signals that might be caused by a breakpoint. */
> + except signals that might be caused by a breakpoint, or SIGSTOP
> + if we sent the SIGSTOP and are waiting for it to arrive. */
> if (!lp->step
> && WSTOPSIG (status) && sigismember (&pass_mask, WSTOPSIG (status))
> + && (WSTOPSIG (status) != SIGSTOP
> + || !find_thread_ptid (lp->ptid)->stop_requested)
> && !linux_wstatus_maybe_breakpoint (status))
> {
> linux_resume_one_lwp (lp, lp->step, signo);
> diff --git a/gdb/testsuite/gdb.threads/stop-with-handle.c b/gdb/testsuite/gdb.threads/stop-with-handle.c
> new file mode 100644
> index 00000000000..da1b3d45498
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/stop-with-handle.c
> @@ -0,0 +1,70 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2019 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 <stdio.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +
> +/* A place to record the thread. */
> +pthread_t the_thread;
> +
> +/* The worker thread just spins forever. */
> +void*
> +thread_worker (void *payload)
> +{
> + while (1)
> + {
> + sleep (1);
> + }
> +
> + return NULL;
> +}
> +
> +/* Create a worker thread. */
> +int
> +spawn_thread ()
> +{
> + if (pthread_create (&the_thread, NULL, thread_worker, NULL))
> + {
> + fprintf (stderr, "Unable to create thread.\n");
> + return 0;
> + }
> + return 1;
> +}
> +
> +/* A place for GDB to place a breakpoint. */
> +void
> +breakpt ()
> +{
> + asm ("" ::: "memory");
> +}
> +
> +/* Create a worker thread that just spins forever, then enter a loop
> + periodically calling the BREAKPT function. */
> +int main() {
> + spawn_thread ();
> +
> + while (1)
> + {
> + sleep (1);
> +
> + breakpt ();
> + }
> +
> + return 0;
> +}
> +
> diff --git a/gdb/testsuite/gdb.threads/stop-with-handle.exp b/gdb/testsuite/gdb.threads/stop-with-handle.exp
> new file mode 100644
> index 00000000000..bab58a6129d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/stop-with-handle.exp
> @@ -0,0 +1,52 @@
> +# Copyright 2019 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/>.
> +
> +# This test covers a case where SIGSTOP has been configured to be
> +# passed to the target with GDB's 'handle' command, and then a
> +# multi-threaded inferior encounters an event that causes all theads
> +# to be stopped.
> +#
> +# The problem that (used) to exist was that GDB would see the SIGSTOP,
> +# but decide to ignore the signal based on the handle table.
> +
> +standard_testfile
> +
> +if {[prepare_for_testing "failed to prepare" \
> + "${testfile}" "${srcfile}" {debug pthreads}]} {
> + return -1
> +}
> +
> +if ![runto_main] then {
> + fail "can't run to main"
> + return 0
> +}
> +
> +# Have SIGSTOP sent to the inferior.
> +gdb_test "handle SIGSTOP nostop noprint pass" \
> + [multi_line \
> + "Signal\[ \t\]+Stop\[ \t\]+Print\[ \t\]+Pass to program\[ \t\]+Description" \
> + "SIGSTOP\[ \t\]+No\[ \t\]+No\[ \t\]+Yes\[ \t\]+Stopped \\(signal\\)"]
> +
> +# Create a breakpoint, and when we hit it automatically finish the
> +# current frame.
> +gdb_breakpoint breakpt
> +
> +# When the bug triggers this continue never completes. GDB hits the
> +# breakpoint in thread 1, and then tries to stop the second thread by
> +# sending it SIGSTOP. GDB sees the SIGSTOP arrive in thread 2 but
> +# incorrect decides to pass the SIGSTOP to the thread rather than
> +# bringing the thread to a stop.
> +gdb_continue_to_breakpoint breakpt
> +
> --
> 2.14.5
>