Re: [PATCH] [remote/gdbserver] Don't lose signals when reconnecting.

[Pedro Alves <palves at redhat dot com>]

On 11/06/2013 06:40 AM, Yao Qi wrote:

>> The only user visible change (other than that the signal is program is
>> stopped at isn't lost / is passed to the program), is in "info
>> program", that now can show the signal the program stopped for.  Of
> 
> We need two NEWS entries? one is about "don't lose signals in 
> reconnection", and the other is about "output changes in info program".

Hmm, I didn't think NEWS entries were needed, since I was looking
at this as just bug fixing?  I didn't do anything special to
"info program" -- it's always displayed the signal the program
is stopped for:

 (gdb)
 ^C
 Program received signal SIGINT, Interrupt.
 0x000000323d4e8b94 in __GI___poll (fds=0xe54f20, nfds=3, timeout=-1) at ../sysdeps/unix/sysv/linux/poll.c:83
 83          return INLINE_SYSCALL (poll, 3, CHECK_N (fds, nfds), nfds, timeout);
 (top-gdb) info program
         Using the running image of child Thread 0x7ffff7fc9740 (LWP 24841).
 Program stopped at 0x323d4e8b94.
 It stopped with signal SIGINT, Interrupt.

It's just that on reconnection, GDB was losing that signal.

I've now filed PR16126 for this:
https://sourceware.org/bugzilla/show_bug.cgi?id=16126

> 
>> +/* Callback for for_each_inferior.  If the thread is stopped with an
>> +   interesting event, mark it as having a pending event.  */
>> +
>> +static void
>> +set_pending_status_callback (struct inferior_list_entry *entry)
>> +{
>> +  struct thread_info *thread = (struct thread_info *) entry;
>> +
>> +  if (thread->last_status.kind != TARGET_WAITKIND_STOPPED
>> +      || (thread->last_status.value.sig != GDB_SIGNAL_0
>> +	  /* A breakpoint, watchpoint or finished step from a previous
>> +	     GDB run isn't considered interesting for a new GDB run.
>> +	     If we left those pending, the new GDB could consider them
>> +	     random SIGTRAPs.  This leaves out real async traps.  We'd
>> +	     have to peek into the (target-specific) siginfo to
>> +	     distinguish those.  */
> 
> The comments are quite clear to me except that last two sentences.  Can 
> you elaborate?

We'll still lose an asynchronous SIGTRAP.  That is, e.g., if the
program was stopped due to a '$kill -SIGTRAP $pid' from the shell.
On Linux, we can tell whether a signal was sent by the kernel or
by the program by looking at siginfo->si_code (SI_USER, SI_KERNEL,
etc., see /usr/include/bits/siginfo.h).

Note that since most stubs out there, including GDBserver, are
always reporting GDB_SIGNAL_TRAP on initial connection (instead of
GDB_SIGNAL_0), GDB will have to keep silently swallowing
GDB_SIGNAL_TRAP.  We'd also need a new target feature to get
around that.  Given that "all-stop on top of non-stop" wouldn't
have that problem (as stubs report GDB_SIGNAL_0 for stopped threads,
not GDB_SIGNAL_TRAP), I'm not bothering with that.

Here's the new version of the comment:

static void
set_pending_status_callback (struct inferior_list_entry *entry)
{
  struct thread_info *thread = (struct thread_info *) entry;

  if (thread->last_status.kind != TARGET_WAITKIND_STOPPED
      || (thread->last_status.value.sig != GDB_SIGNAL_0
	  /* A breakpoint, watchpoint or finished step from a previous
	     GDB run isn't considered interesting for a new GDB run.
	     If we left those pending, the new GDB could consider them
	     random SIGTRAPs.  Unfortunately, this means we'll lose
	     asynchronous SIGTRAPs on reconnection.  We'd have to
	     consult the target to distinguish those (e.g.,
	     siginfo->si_code on Linux), and also, somehow tell GDB to
	     not ignore GDB_SIGNAL_TRAPs on all-stop connection
	     setup.  */
	  && thread->last_status.value.sig != GDB_SIGNAL_TRAP))
    thread->status_pending_p = 1;
}

> 
>> +	  && thread->last_status.value.sig != GDB_SIGNAL_TRAP))
>> +    thread->status_pending_p = 1;
>> +}
>> +
>> +/* Callback for find_inferior.  Return true if ENTRY (a thread) has a
>> +   pending status to report to GDB.  */
>> +
>> +static int
>> +find_status_pending_thread_callback (struct inferior_list_entry *entry, void *data)
> 
> This line is too long.

Fixed.

>> @@ -2544,13 +2635,15 @@ handle_status (char *own_buf)
>>     /* GDB is connected, don't forward events to the target anymore.  */
>>     for_each_inferior (&all_processes, gdb_reattached_process);
>>
>> +  discard_queued_stop_replies (-1);
>> +  for_each_inferior (&all_threads, clear_pending_status_callback);
>> +
>>     /* In non-stop mode, we must send a stop reply for each stopped
>>        thread.  In all-stop mode, just send one for the first stopped
>>        thread we find.  */
>>
>>     if (non_stop)
>>       {
>> -      discard_queued_stop_replies (-1);
> 
> I'd like to figure out the reason call discard_queued_stop_replies in 
> all-stop mode.  We want to handle the case that all-stop GDB connects to 
> a non-stop GDBserver, and discard all previous stop replies, 

Yes, that.  Perhaps even better would be to discard them immediately
when the previous GDB disconnects.  We don't queue stop replies if
GDB isn't connected (disconnected tracing) anyway.

> or something else?

-- 
Pedro Alves