[v2] Always pass signals to the right thread.

From: Pedro Alves <palves@redhat.com>

  On 06/23/2014 03:28 AM, Yao Qi wrote:
> 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.

Well, stop and think is the point of the warning/query.  :-)

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

Thanks.  Even though it first appeared promising, I've been playing
with this since yesterday and in the end I can't seem to find it
really better.  :-/
(I had already contemplated lots of different options before
posting the original patch.)

That kind of reads to me like only those three threads
are continued.  With my user hat on, that then makes me wonder what
will happen to all the other threads not listed.  And why is GDB
really telling about these signal that I didn't specify?  And which
is the one that is getting the signal because I'm overriding
it?  Thread 3 or thread 1?  And that output would seem to make more
sense to me if we always asked for confirmation even if no thread
other than the current would get a signal, otherwise, the user has
no clue really why he's being asked for confirmation), but I'm not sure
that'd be a good idea.  (Also, I think the output should still
make sense if we ever find a need to add a cap on the number of
threads listed and say "many threads" instead of showing a list.)

In any case, I still tried this direction, by doing two passes
over the threads, to determine whether we need to ask for confirmation,
and added an intro text:

(gdb) signal SIGUSR1
Some of the threads other than the current that are also about to be resumed had previously stopped for signals.
If you proceed, those signals will be delivered to the respective threads.
  Continuing thread 3 with signal SIGUSR2.
  Continuing thread 1 with signal SIGUSR1.
Continue anyway? (y or n)

But, what about "signal 0" ?  That results in:

(gdb) signal 0
Some of the threads other than the current that are also about to be resumed had previously stopped for signals.
If you proceed, those signals will be delivered to the respective threads.
  Continuing thread 3 with signal SIGUSR2.
  Continuing thread 1 with no signal.
Continue anyway? (y or n)

And again, that makes me wonder why "no signal" is mentioned for only
one thread and not any other that will be resumed with no signal
either (because it hadn't stopped with one).

We could then list what will happen to _all_ threads that will be
resumed, with or without signal, but I _really_ don't think it's
a good idea -- that set will usually be _much_ larger than the
signalled threads, which will normally be just one.

So all in all, in the end, I still prefer my original output.

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

Done.

>> +      ALL_NON_EXITED_THREADS (tp)
>> +        {
> 
> Replace eight spaces with a tab.

Thanks, fixed.

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

We can get here with the stop signal set to GDB_SIGNAL_TRAP due
to a breakpoint of a finished step.  If we don't clear it now,
the next time we'll resume the target, and this thread isn't
current, we'd pass the trap to the thread.  See below for further
explanation.

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

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

Nope, "signal SIGFOO" should still pass SIGFOO even if
"handle SIGFOO nopass".  So in that case, 
signal_pass_state (thread->suspend.stop_signal) is false,
but we should still pass stop_signal.  We assume the thread
that was current at "signal SIGFOO" time will be resumed immediately,
because GDB may need to e.g., step-over a breakpoint for another
thread first.  So we store the intent to pass SIGFOO once the
step over is finished in stop_signal.

I've added this comment:

 --- c/gdb/gdbthread.h
 +++ w/gdb/gdbthread.h
 @@ -135,7 +135,13 @@ struct thread_control_state

  struct thread_suspend_state
  {
 -  /* Last signal that the inferior received (why it stopped).  */
 +  /* Last signal that the inferior received (why it stopped).  When
 +     the thread is resumed, this signal is delivered.  Note: the
 +     target should not check whether the signal is in pass state,
 +     because the signal may have been explicitly passed with the
 +     "signal" command, which overrides "handle nopass".  If the signal
 +     should be suppressed, the core will take care of clearing this
 +     before the target is resumed.  */
    enum gdb_signal stop_signal;
  };

Does that make things clearer ?

>> +
>> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
>> +	 executable { debug }] != "" } {
>> +    return -1
>> +}
> 
> Need [target_info exists gdb,nosignals] check?

Yeah, thanks.

> 
>> +
>> +# 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"

The reason I don't like "info threads 1" this is that if the test fails,
when you go look at the logs, you don't see which thread was actually
current, which tends to make debugging things a little easier.   But we
can still simplify the patterns.  I was aiming at having no wildcards in
the middle of the output, but that's really not necessary.
I've simplified the patterns now.

Here's the updated patch.

8<-------------------------------
From 5834f1fea7d303e3964751d6af505cac44fd1754 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 24 Jun 2014 12:12:57 +0100
Subject: [PATCH] Always pass signals to the right thread.

Currently, GDB can pass a signal to the wrong thread in several
different but related scenarios.

E.g., if thread 1 stops for signal SIGFOO, the user switches to thread
2, and then issues "continue", SIGFOO is actually delivered to thread
2, not thread 1.  This obviously messes up programs that use
pthread_kill to send signals to specific threads.

This has been a known issue for a long while.  Back in 2008 when I
made stop_signal be per-thread (2020b7ab), I kept the behavior -- see
code in 'proceed' being removed -- wanting to come back to it later.
The time has finally come now.

The patch fixes this -- on resumption, intercepted signals are always
delivered to the thread that had intercepted them.

Another example: if thread 1 stops for a breakpoint, the user switches
to thread 2, and then issues "signal SIGFOO", SIGFOO is actually
delivered to thread 1, not thread 2, because 'proceed' first switches
to thread 1 to step over its breakpoint...  If the user deletes the
breakpoint before issuing "signal FOO", then the signal is delivered
to thread 2 (the current thread).

"signal SIGFOO" can be used for two things: inject a signal in the
program while the program/thread had stopped for none, bypassing
"handle nopass"; or changing/suppressing a signal the program had
stopped for.  These scenarios are really two faces of the same coin,
and GDB can't really guess what the user is trying to do.  GDB might
have intercepted signals in more than one thread even (see the new
signal-command-multiple-signals-pending.exp test).  At least in the
inject case, it's obviously clear to me that the user means to deliver
the signal to the currently selected thread, so best is to make the
command's behavior consistent and easy to explain.

Then, if the user is trying to suppress/change a signal the program
had stopped for instead of injecting a new signal, but, the user had
changed threads meanwhile, then she will be surprised that with:

  (gdb) continue
  Thread 1 stopped for signal SIGFOO.
  (gdb) thread 2
  (gdb) signal SIGBAR

... GDB actually delivers SIGFOO to thread 1, and SIGBAR to thread 2
(with scheduler-locking off, which is the default, because then
"signal" or any other resumption command resumes all threads).

So the patch makes GDB detect that, and ask for confirmation:

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

All these scenarios are covered by the new tests.

Tested on x86_64 Fedora 20, native and gdbserver.

gdb/
2014-06-24  Pedro Alves  <palves@redhat.com>

	* NEWS: Mention signal passing and "signal" command changes.

	* gdbthread.h (struct thread_suspend_state) <stop_signal>: Extend
	comment.
	* breakpoint.c (until_break_command): Adjust clear_proceed_status
	call.
	* infcall.c (run_inferior_call): Adjust clear_proceed_status call.
	* infcmd.c (proceed_thread_callback, continue_1, step_once)
	(jump_command): Adjust clear_proceed_status call.
	(signal_command): Warn if other thread that are resumed have
	signals that will be delivered.  Adjust clear_proceed_status call.
	(until_next_command, finish_command)
	(proceed_after_attach_callback, attach_command_post_wait)
	(attach_command): Adjust clear_proceed_status call.
	* infrun.c (proceed_after_vfork_done): Likewise.
	(proceed_after_attach_callback): Adjust comment.
	(clear_proceed_status_thread): Clear stop_signal if not in pass
	state.
	(clear_proceed_status_callback): Delete.
	(clear_proceed_status): New 'step' parameter.  Only clear the
	proceed status of threads the command being prepared is about to
	resume.
	(proceed): If passed in an explicit signal, override stop_signal
	with it.  Don't pass the last stop signal to the thread we're
	resuming.
	(init_wait_for_inferior): Adjust clear_proceed_status call.
	(switch_back_to_stepped_thread): Clear the signal if it should not
	be passed.
	* infrun.h (clear_proceed_status): New 'step' parameter.
	(user_visible_resume_ptid): Add comment.
	* linux-nat.c (linux_nat_resume_callback): Don't check whether the
	signal is in pass state.
	* remote.c (append_pending_thread_resumptions): Likewise.
	* mi/mi-main.c (proceed_thread): Adjust clear_proceed_status call.

gdb/doc/
2014-06-24  Pedro Alves  <palves@redhat.com>

	* gdb.texinfo (Signaling) <signal command>: Explain what happens
	with multi-threaded programs.

gdb/testsuite/
2014-06-24  Pedro Alves  <palves@redhat.com>

	* gdb.threads/signal-command-handle-nopass.c: New file.
	* gdb.threads/signal-command-handle-nopass.exp: New file.
	* gdb.threads/signal-command-multiple-signals-pending.c: New file.
	* gdb.threads/signal-command-multiple-signals-pending.exp: New file.
	* gdb.threads/signal-delivered-right-thread.c: New file.
	* gdb.threads/signal-delivered-right-thread.exp: New file.
---
 gdb/NEWS                                           |  11 ++
 gdb/breakpoint.c                                   |   2 +-
 gdb/doc/gdb.texinfo                                |  10 +-
 gdb/gdbthread.h                                    |   8 +-
 gdb/infcall.c                                      |   2 +-
 gdb/infcmd.c                                       |  64 ++++++--
 gdb/infrun.c                                       |  93 +++++-------
 gdb/infrun.h                                       |   8 +-
 gdb/linux-nat.c                                    |   3 +-
 gdb/mi/mi-main.c                                   |   2 +-
 gdb/remote.c                                       |   3 +-
 .../gdb.threads/signal-command-handle-nopass.c     |  49 ++++++
 .../gdb.threads/signal-command-handle-nopass.exp   |  78 ++++++++++
 .../signal-command-multiple-signals-pending.c      |  98 ++++++++++++
 .../signal-command-multiple-signals-pending.exp    | 166 +++++++++++++++++++++
 .../gdb.threads/signal-delivered-right-thread.c    |  61 ++++++++
 .../gdb.threads/signal-delivered-right-thread.exp  |  85 +++++++++++
 17 files changed, 664 insertions(+), 79 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/signal-command-handle-nopass.c
 create mode 100644 gdb/testsuite/gdb.threads/signal-command-handle-nopass.exp
 create mode 100644 gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.c
 create mode 100644 gdb/testsuite/gdb.threads/signal-command-multiple-signals-pending.exp
 create mode 100644 gdb/testsuite/gdb.threads/signal-delivered-right-thread.c
 create mode 100644 gdb/testsuite/gdb.threads/signal-delivered-right-thread.exp

[v2] Always pass signals to the right thread.

Commit Message

Comments

Patch