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

[Yao Qi <yao at codesourcery dot com>]

On 11/05/2013 02:53 AM, Pedro Alves wrote:
> GDB currently always adds the thread reported in response to the
> status query as the first thread in its list.  That means that if we
> start with e.g.,
>
>   (gdb) info threads
>     3 Thread 1003 ...
>   * 2 Thread 1002 ...
>     1 Thread 1001 ...
>
> And reconnect:
>
>   (gdb) disconnect
>   (gdb) tar rem ...
>
> We end up with:
>
>   (gdb) info threads
>     3 Thread 1003 ...
>     2 Thread 1001 ...
>   * 1 Thread 1002 ...
>
> Not a real big issue, but it's reasonably fixable, by having GDB
> fetch/sync the thread list before fetching the status/'?', and then
> using the status to select the right thread as current on the GDB
> side.  Holes in the thread numbers are squashed before/after
> reconnection (e.g., 2,3,5 becomes 1,2,3), but the order is preserved,
> which I think is both good, and good enough.

That looks good to me, because in a fresh connection, it is reasonable 
that thread id started from 1 again, and that is what GDB behaves nowadays.

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

> +/* 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?

> +	  && 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.

> +{
> +  struct thread_info *thread = (struct thread_info *) entry;
> +
> +  return thread->status_pending_p;
> +}
> +
>   /* Status handler for the '?' packet.  */
>
>   static void
> @@ -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, or 
something else?

--
Yao (éå)