Re: [PATCH] Always pass signals to the right thread.

[Yao Qi <yao at codesourcery dot com>]

On 06/19/2014 09:09 PM, Pedro Alves wrote:
>   (gdb) thread 1
>   [Switching to thread 1 (Thread 10979)]
>   (gdb) signal SIGUSR2
>   Note:
>     Thread 3 previously stopped with signal SIGUSR2, User defined signal 2.
>     Thread 2 previously stopped with signal SIGUSR1, User defined signal 1.
>   Continuing thread 1 (the current thread) with specified signal will
>   still deliver the signals noted above to their respective threads.
>   Continue anyway? (y or n)

This message isn't clear to me, because user has to read this sentence
and think what GDB will do for these threads.  IMO, it is better to say
explicitly what GDB will do for each thread, like

(gdb) signal SIGUSR2
  Continuing Thread 3 with signal SIGUSR2, User defined signal 2.
  Continuing Thread 2 with signal SIGUSR1, User defined signal 1.
  Continuing Thread 1 with signal SIGUSR2, User defined signal 2.
Continue anyway? (y or n)

We may need a NEWS entry, as this is a user-visible change.

> @@ -1245,6 +1245,50 @@ signal_command (char *signum_exp, int from_tty)
>  	oursig = gdb_signal_from_command (num);
>      }
>  
> +  /* Look for threads other than the current that this command ends up
> +     resuming too (due to schedlock off), and warn if they'll get a
> +     signal delivered.  "signal 0" is used to suppress a previous
> +     signal, but if the current thread is no longer the one that got
> +     the signal, then the user is potentially suppressing the signal
> +     of the wrong thread.  */
> +  if (!non_stop)
> +    {
> +      struct thread_info *tp;
> +      ptid_t resume_ptid;
> +      int must_confirm = 0;
> +
> +      /* This indicates what will be resumed.  Either a single thread,
> +	 a whole process, or all threads of all processes.  */
> +      resume_ptid = user_visible_resume_ptid (0);
> +
> +      ALL_NON_EXITED_THREADS (tp)
> +        {

Replace eight spaces with a tab.

> @@ -5190,6 +5161,10 @@ switch_back_to_stepped_thread (struct execution_control_state *ecs)
>  	 what keep_going does as well, if we call it.  */
>        ecs->event_thread->control.trap_expected = 0;
>  
> +      /* Likewise, clear the signal if it should not be passed.  */
> +      if (!signal_program[ecs->event_thread->suspend.stop_signal])
> +	ecs->event_thread->suspend.stop_signal = GDB_SIGNAL_0;
> +

I can understand this patch except this part.  Why do we need to set
stop_signal here?

> diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
> index c9677ca..8f2600d 100644
> --- a/gdb/linux-nat.c
> +++ b/gdb/linux-nat.c
> @@ -1694,8 +1694,7 @@ linux_nat_resume_callback (struct lwp_info *lp, void *except)
>        thread = find_thread_ptid (lp->ptid);
>        if (thread != NULL)
>  	{
> -	  if (signal_pass_state (thread->suspend.stop_signal))
> -	    signo = thread->suspend.stop_signal;
> +	  signo = thread->suspend.stop_signal;
>  	  thread->suspend.stop_signal = GDB_SIGNAL_0;
>  	}
>      }  
> diff --git a/gdb/remote.c b/gdb/remote.c
> index b5318f1..e56cfea 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -4629,8 +4629,7 @@ append_pending_thread_resumptions (char *p, char *endp, ptid_t ptid)
>    ALL_NON_EXITED_THREADS (thread)
>      if (ptid_match (thread->ptid, ptid)
>  	&& !ptid_equal (inferior_ptid, thread->ptid)
> -	&& thread->suspend.stop_signal != GDB_SIGNAL_0
> -	&& signal_pass_state (thread->suspend.stop_signal))
> +	&& thread->suspend.stop_signal != GDB_SIGNAL_0)
>        {
>  	p = append_resumption (p, endp, thread->ptid,
>  			       0, thread->suspend.stop_signal);

You didn't say much about these two changes.  Is it because we've
already set thread->suspend.stop_signal to GDB_SIGNAL_0 if
signal_pass_state (tp->suspend.stop_signal) is false,

+  /* If this signal should not be seen by program, give it zero.
+     Used for debugging signals.  */
+  if (!signal_pass_state (tp->suspend.stop_signal))
+    tp->suspend.stop_signal = GDB_SIGNAL_0;
+

that means (tp->suspend.stop_signal == GDB_SIGNAL_0 ||
signal_pass_state (tp->suspend.stop_signal)) is constantly true.
so in the callees of target_resume, signal_pass_state
(thread->suspend.stop_signal) is always true if
thread->suspend.stop_signal isn't GDB_SIGNAL_0?

> diff --git a/gdb/testsuite/gdb.threads/signal-command-handle-nopass.exp b/gdb/testsuite/gdb.threads/signal-command-handle-nopass.exp
> new file mode 100644
> index 0000000..893e266
> --- /dev/null
> +++ b/gdb/testsuite/gdb.threads/signal-command-handle-nopass.exp
> @@ -0,0 +1,78 @@
> +# Copyright (C) 2014 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/>.  */
> +
> +# Test that an explicit "signal FOO" delivers FOO even if "handle" for
> +# that same signal is set to "nopass".  Also make sure the signal is
> +# delivered to the right thread, even if GDB has to step over a
> +# breakpoint in some other thread first.
> +
> +standard_testfile
> +
> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
> +	 executable { debug }] != "" } {
> +    return -1
> +}

Need [target_info exists gdb,nosignals] check?

> +
> +# Run the test proper.  STEP_OVER indicates whether we leave in place
> +# a breakpoint that needs to be stepped over when we explicitly
> +# request a signal be delivered with the "signal" command.
> +
> +proc test { step_over } {
> +    global srcfile binfile
> +
> +    with_test_prefix "step-over $step_over" {
> +	clean_restart ${binfile}
> +
> +	if ![runto_main] then {
> +	    fail "Can't run to main"
> +	    return 0
> +	}
> +
> +	gdb_test "handle SIGUSR1 stop print nopass"
> +
> +	gdb_test "b thread_function" "Breakpoint .* at .*$srcfile.*"
> +	gdb_test "continue" "thread_function.*" "stopped in thread"
> +
> +	# Thread 2 is stopped at a breakpoint.  If we leave the
> +	# breakpoint in place, GDB needs to move thread 2 past the
> +	# breakpoint before delivering the signal to thread 1.  We
> +	# want to be sure that GDB doesn't mistakenly deliver the
> +	# signal to thread 1 while doing that.
> +	if { $step_over == "no" } {
> +	    delete_breakpoints
> +	}
> +
> +	gdb_test "break handler" "Breakpoint .* at .*$srcfile.*"
> +
> +	gdb_test "thread 1" "Switching to thread 1.*"
> +
> +	set ws "\[ \t\]+"
> +	set line "\[^\r\n\]+"
> +
> +	set pattern ""
> +	append pattern "${ws}2${ws}Thread ${line}\r\n"
> +	append pattern "\\\* 1${ws}Thread ${line}.*"
> +
> +	gdb_test "info threads" $pattern "thread 1 selected"

We want to know whether thread 1 is selected, so using "info threads 1"
is simpler in the regexp pattern, like

 gdb_test "info threads 1" "\\\* 1${ws}Thread .*" "thread 1 selected"

or we can use gdb_test_sequence with which, the regexp pattern can be
simpler too.

> +
> +	gdb_test "signal SIGUSR1" "handler .*"
> +
> +	gdb_test "info threads" $pattern "thread 1 got the signal"

Likewise.

-- 
Yao (éå)