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

Andrew Burgess
2019-10-03T11:55:48Z

I plan to merge this patch tomorrow unless anyone complains before
then.

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-09-27 00:06:13 +0100]:

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