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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

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