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

[Andrew Burgess <andrew.burgess@embecosm.com>]

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
>