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

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

[Pedro Alves]

Currently, when GDB connects in all-stop mode, GDBserver always
responds to the status packet with a GDB_SIGNAL_TRAP, even if the
program is actually stopped for some other signal.

 (gdb) tar rem ...
 ...
 (gdb) c
 Program received signal SIGUSR1, User defined signal 1.
 (gdb) disconnect
 (gdb) tar rem ...
 (gdb) c

(Or a GDB crash instead of an explicit disconnect.)

This results in the program losing that signal on that last continue,
because gdb will tell the target to resume with no signal (to suppress
the GDB_SIGNAL_TRAP, due to 'handle SISGTRAP nopass'), and that will
actually suppress the real signal the program had stopped for
(SIGUSR1).  To fix that, I think we should make GDBserver report the
real signal the thread had stopped for in response to the status
packet:

 @item ?
 @cindex @samp{?} packet
 Indicate the reason the target halted.  The reply is the same as for
 step and continue.

But, that raises the question -- which thread are we reporting the
status for?  Due to how the RSP in all-stop works, we can only report
one status.  The status packet's response is a stop reply packet, so
it includes the thread identifier, so it's not a problem packet-wise.
However, GDBserver is currently always reporting the status for first
thread in the thread list, even though that may well not be the thread
that got the signal that caused the program to stop.  So the next
logical step would be to report the status for the
last_ptid/last_status thread (the last event reported to gdb), if it's
still around; and if not, fallback to some other thread.

There's an issue on the GDB side with that, though...

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.

However (yes, there's more...), the previous GDB that was connected
might have had gdbserver running in non-stop mode, or could have left
gdbserver doing disconnected tracing (which also forces non-stop), and
if the new gdb/connection is in all-stop mode, we can end up with more
than one thread with a signal to report back to gdb.  As we can only
report one thread/status (in the all-stop RSP variant; the non-stop
variant doesn't have this issue), we get to do what we do at every
other place we have this situation -- leave events we can't report
right now as pending, so that the next resume picks them up.

Note all this ammounts to a QoI change, within the existing framework.
There's really no RSP change here.

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
course, the next resume will respect the pass/nopass setting for the
signal in question.  It'd be reasonable to have the initial connection
tell the user the program was stopped with a signal, similar to when
we load a core to debug, but I'm leaving that out for a future change.
I think we'll need to either change how handle_inferior_event & co
handle stop_soon, or maybe bypass them completely (like
fork-child.c:startup_inferior) for that.

Tested on x86_64 Fedora 17.

Comments?

gdb/gdbserver/
2013-11-04  Pedro Alves  <palves@redhat.com>

	* gdbthread.h (struct thread_info) <status_pending_p>: New field.
	* server.c (visit_actioned_threads, handle_pending_status): New
	function.
	(handle_v_cont): Factor out parts to ...
	(resume): ... this new function.  If in all-stop, and a thread
	being resumed has a pending status, report it without actually
	resuming.
	(myresume): Adjust to use the new 'resume' function.
	(clear_pending_status_callback, set_pending_status_callback)
	(find_status_pending_thread_callback): New functions.
	(handle_status): Handle the case of multiple threads having
	interesting statuses to report.  Report threads' real last signal
	instead of always reporting GDB_SIGNAL_TRAP.  Look for a thread
	with an interesting thread to report the status for, instead of
	always reporting the status of the first thread.

gdb/
2013-11-04  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_add_thread): Add threads silently if starting
	up.
	(remote_notice_new_inferior): If in all-stop, and starting up,
	don't call notice_new_inferior.
	(get_current_thread): New function, factored out from ...
	(add_current_inferior_and_thread): ... this.  Adjust.
	(remote_start_remote) <all-stop>: Fetch the thread list.  If we
	found any thread, then select the remote's current thread as GDB's
	current thread too.

gdb/testsuite/
2013-11-04  Pedro Alves  <palves@redhat.com>

	* gdb.threads/disconnect.c: New file.
	* gdb.threads/disconnect.exp: New file.
---
 gdb/gdbserver/gdbthread.h                      |   3 +
 gdb/gdbserver/server.c                         | 212 ++++++++++++++++++++-----
 gdb/remote.c                                   |  86 ++++++++--
 gdb/testsuite/gdb.threads/reconnect-signal.c   |  67 ++++++++
 gdb/testsuite/gdb.threads/reconnect-signal.exp |  84 ++++++++++
 5 files changed, 394 insertions(+), 58 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/reconnect-signal.c
 create mode 100644 gdb/testsuite/gdb.threads/reconnect-signal.exp

diff --git a/gdb/gdbserver/gdbthread.h b/gdb/gdbserver/gdbthread.h
index b43d676..1bb2178 100644
--- a/gdb/gdbserver/gdbthread.h
+++ b/gdb/gdbserver/gdbthread.h
@@ -36,6 +36,9 @@ struct thread_info
   /* The last wait status reported for this thread.  */
   struct target_waitstatus last_status;
 
+  /* True if LAST_STATUS hasn't been reported to GDB yet.  */
+  int status_pending_p;
+
   /* Given `while-stepping', a thread may be collecting data for more
      than one tracepoint simultaneously.  E.g.:
 
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index e0af785..2d528bd 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2016,6 +2016,63 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 }
 
 static void gdb_wants_all_threads_stopped (void);
+static void resume (struct thread_resume *actions, size_t n);
+
+/* Call CALLBACK for any thread to which ACTIONS applies to.  Returns
+   true if CALLBACK returns true.  Returns false if no matching thread
+   is found or CALLBACK results false.  */
+
+static int
+visit_actioned_threads (const struct thread_resume *actions,
+			size_t num_actions,
+			int (*callback) (const struct thread_resume *,
+					 struct thread_info *))
+{
+  struct inferior_list_entry *entry;
+
+  for (entry = all_threads.head; entry != NULL; entry = entry->next)
+    {
+      size_t i;
+
+      for (i = 0; i < num_actions; i++)
+	{
+	  const struct thread_resume *action = &actions[i];
+
+	  if (ptid_equal (action->thread, minus_one_ptid)
+	      || ptid_equal (action->thread, entry->id)
+	      || ((ptid_get_pid (action->thread)
+		   == ptid_get_pid (entry->id))
+		  && ptid_get_lwp (action->thread) == -1))
+	    {
+	      struct thread_info *thread = (struct thread_info *) entry;
+
+	      if ((*callback) (action, thread))
+		return 1;
+	    }
+	}
+    }
+
+  return 0;
+}
+
+/* Callback for visit_actioned_threads.  If the thread has a pending
+   status to report, report it now.  */
+
+static int
+handle_pending_status (const struct thread_resume *resumption,
+		       struct thread_info *thread)
+{
+  if (thread->status_pending_p)
+    {
+      thread->status_pending_p = 0;
+
+      last_status = thread->last_status;
+      last_ptid = thread->entry.id;
+      prepare_resume_reply (own_buf, last_ptid, &last_status);
+      return 1;
+    }
+  return 0;
+}
 
 /* Parse vCont packets.  */
 void
@@ -2128,12 +2185,34 @@ handle_v_cont (char *own_buf)
     cont_thread = minus_one_ptid;
   set_desired_inferior (0);
 
+  resume (resume_info, n);
+  free (resume_info);
+  return;
+
+err:
+  write_enn (own_buf);
+  free (resume_info);
+  return;
+}
+
+/* Resume target with ACTIONS, an array of NUM_ACTIONS elements.  */
+
+static void
+resume (struct thread_resume *actions, size_t num_actions)
+{
   if (!non_stop)
-    enable_async_io ();
+    {
+      /* Check if among the threads that GDB wants actioned, there's
+	 one with a pending status to report.  If so, skip actually
+	 resuming/stopping and report the pending event
+	 immediately.  */
+      if (visit_actioned_threads (actions, num_actions, handle_pending_status))
+	return;
 
-  (*the_target->resume) (resume_info, n);
+      enable_async_io ();
+    }
 
-  free (resume_info);
+  (*the_target->resume) (actions, num_actions);
 
   if (non_stop)
     write_ok (own_buf);
@@ -2157,12 +2236,6 @@ handle_v_cont (char *own_buf)
           || last_status.kind == TARGET_WAITKIND_SIGNALLED)
         mourn_inferior (find_process_pid (ptid_get_pid (last_ptid)));
     }
-  return;
-
-err:
-  write_enn (own_buf);
-  free (resume_info);
-  return;
 }
 
 /* Attach to a new program.  Return 1 if successful, 0 if failure.  */
@@ -2422,31 +2495,7 @@ myresume (char *own_buf, int step, int sig)
       n++;
     }
 
-  if (!non_stop)
-    enable_async_io ();
-
-  (*the_target->resume) (resume_info, n);
-
-  if (non_stop)
-    write_ok (own_buf);
-  else
-    {
-      last_ptid = mywait (minus_one_ptid, &last_status, 0, 1);
-
-      if (last_status.kind != TARGET_WAITKIND_EXITED
-          && last_status.kind != TARGET_WAITKIND_SIGNALLED)
-	{
-	  current_inferior->last_resume_kind = resume_stop;
-	  current_inferior->last_status = last_status;
-	}
-
-      prepare_resume_reply (own_buf, last_ptid, &last_status);
-      disable_async_io ();
-
-      if (last_status.kind == TARGET_WAITKIND_EXITED
-          || last_status.kind == TARGET_WAITKIND_SIGNALLED)
-        mourn_inferior (find_process_pid (ptid_get_pid (last_ptid)));
-    }
+  resume (resume_info, n);
 }
 
 /* Callback for for_each_inferior.  Make a new stop reply for each
@@ -2536,6 +2585,48 @@ gdb_reattached_process (struct inferior_list_entry *entry)
   process->gdb_detached = 0;
 }
 
+/* Callback for for_each_inferior.  Clear the thread's pending status
+   flag.  */
+
+static void
+clear_pending_status_callback (struct inferior_list_entry *entry)
+{
+  struct thread_info *thread = (struct thread_info *) entry;
+
+  thread->status_pending_p = 0;
+}
+
+/* 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.  */
+	  && 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)
+{
+  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);
       find_inferior (&all_threads, queue_stop_reply_callback, NULL);
 
       /* The first is sent immediatly.  OK is sent if there is no
@@ -2560,18 +2653,53 @@ handle_status (char *own_buf)
     }
   else
     {
+      struct inferior_list_entry *thread = NULL;
+
       pause_all (0);
       stabilize_threads ();
       gdb_wants_all_threads_stopped ();
 
-      if (all_threads.head)
-	{
-	  struct target_waitstatus status;
+      /* We can only report one status, but we might be coming out of
+	 non-stop -- if more than one thread is stopped with
+	 interesting events, leave events for the threads we're not
+	 reporting now pending.  They'll be reported the next time the
+	 threads are resumed.  Start by marking all interesting events
+	 as pending.  */
+      for_each_inferior (&all_threads, set_pending_status_callback);
+
+      /* Prefer the last thread that reported an event to GDB (even if
+	 that was a GDB_SIGNAL_TRAP).  */
+      if (last_status.kind != TARGET_WAITKIND_IGNORE
+	  && last_status.kind != TARGET_WAITKIND_EXITED
+	  && last_status.kind != TARGET_WAITKIND_SIGNALLED)
+	thread = find_inferior_id (&all_threads, last_ptid);
+
+      /* If the last event thread is not found for some reason, look
+	 for some other thread that might have an event to report.  */
+      if (thread == NULL)
+	thread = find_inferior (&all_threads,
+				find_status_pending_thread_callback, NULL);
+
+      /* If we're still out of luck, simply pick the first thread in
+	 the thread list.  */
+      if (thread == NULL)
+	thread = all_threads.head;
+
+      if (thread != NULL)
+	{
+	  struct thread_info *tp = (struct thread_info *) thread;
+
+	  /* We're reporting this event, so it's no longer
+	     pending.  */
+	  tp->status_pending_p = 0;
+
+	  /* GDB assumes the current thread is the thread we're
+	     reporting the status for.  */
+	  general_thread = thread->id;
+	  set_desired_inferior (1);
 
-	  status.kind = TARGET_WAITKIND_STOPPED;
-	  status.value.sig = GDB_SIGNAL_TRAP;
-	  prepare_resume_reply (own_buf,
-				all_threads.head->id, &status);
+	  gdb_assert (tp->last_status.kind != TARGET_WAITKIND_IGNORE);
+	  prepare_resume_reply (own_buf, tp->entry.id, &tp->last_status);
 	}
       else
 	strcpy (own_buf, "W00");
diff --git a/gdb/remote.c b/gdb/remote.c
index 7bd9b2a..9639802 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1564,7 +1564,18 @@ remote_add_inferior (int fake_pid_p, int pid, int attached)
 static void
 remote_add_thread (ptid_t ptid, int running)
 {
-  add_thread (ptid);
+  struct remote_state *rs = get_remote_state ();
+
+  /* GDB historically didn't pull threads in the initial connection
+     setup.  If the remote target doesn't even have a concept of
+     threads (e.g., a bare-metal target), even if internally we
+     consider that a single-threaded target, mentioning a new thread
+     might be confusing to the user.  Be silent then, preserving the
+     age old behavior.  */
+  if (rs->starting_up)
+    add_thread_silent (ptid);
+  else
+    add_thread (ptid);
 
   set_executing (ptid, running);
   set_running (ptid, running);
@@ -1642,9 +1653,15 @@ remote_notice_new_inferior (ptid_t currthread, int running)
 
       /* If we found a new inferior, let the common code do whatever
 	 it needs to with it (e.g., read shared libraries, insert
-	 breakpoints).  */
+	 breakpoints), unless we're just setting up an all-stop
+	 connection.  */
       if (inf != NULL)
-	notice_new_inferior (currthread, running, 0);
+	{
+	  struct remote_state *rs = get_remote_state ();
+
+	  if (non_stop || !rs->starting_up)
+	    notice_new_inferior (currthread, running, 0);
+	}
     }
 }
 
@@ -3312,6 +3329,28 @@ stop_reply_extract_thread (char *stop_reply)
   return null_ptid;
 }
 
+/* Determine the remote side's current thread.  If we have a stop
+   reply handy (in WAIT_STATUS), maybe it's a T stop reply with a
+   "thread" register we can extract the current thread from.  If not,
+   ask the remote which is the current thread with qC.  The former
+   method avoids a roundtrip.  */
+
+static ptid_t
+get_current_thread (char *wait_status)
+{
+  ptid_t ptid;
+
+  /* Note we don't use remote_parse_stop_reply as that makes use of
+     the target architecture, which we haven't yet fully determined at
+     this point.  */
+  if (wait_status != NULL)
+    ptid = stop_reply_extract_thread (wait_status);
+  if (ptid_equal (ptid, null_ptid))
+    ptid = remote_current_thread (inferior_ptid);
+
+  return ptid;
+}
+
 /* Query the remote target for which is the current thread/process,
    add it to our tables, and update INFERIOR_PTID.  The caller is
    responsible for setting the state such that the remote end is ready
@@ -3332,18 +3371,8 @@ add_current_inferior_and_thread (char *wait_status)
 
   inferior_ptid = null_ptid;
 
-  /* Now, if we have thread information, update inferior_ptid.  First
-     if we have a stop reply handy, maybe it's a T stop reply with a
-     "thread" register we can extract the current thread from.  If
-     not, ask the remote which is the current thread, with qC.  The
-     former method avoids a roundtrip.  Note we don't use
-     remote_parse_stop_reply as that makes use of the target
-     architecture, which we haven't yet fully determined at this
-     point.  */
-  if (wait_status != NULL)
-    ptid = stop_reply_extract_thread (wait_status);
-  if (ptid_equal (ptid, null_ptid))
-    ptid = remote_current_thread (inferior_ptid);
+  /* Now, if we have thread information, update inferior_ptid.  */
+  ptid = get_current_thread (wait_status);
 
   if (!ptid_equal (ptid, null_ptid))
     {
@@ -3513,10 +3542,35 @@ remote_start_remote (int from_tty, struct target_ops *target, int extended_p)
 	  strcpy (wait_status, rs->buf);
 	}
 
+      /* Fetch thread list.  */
+      target_find_new_threads ();
+
       /* Let the stub know that we want it to return the thread.  */
       set_continue_thread (minus_one_ptid);
 
-      add_current_inferior_and_thread (wait_status);
+      if (thread_count () == 0)
+	{
+	  /* Target has no concept of threads at all.  GDB treats
+	     non-threaded target as single-threaded; add a main
+	     thread.  */
+	  add_current_inferior_and_thread (wait_status);
+	}
+      else
+	{
+	  /* We have thread information; select the thread the target
+	     says should be current.  If we're reconnecting to a
+	     multi-threaded program, this will ideally be the thread
+	     that last reported an event before GDB disconnected.  */
+	  inferior_ptid = get_current_thread (wait_status);
+	  if (ptid_equal (inferior_ptid, null_ptid))
+	    {
+	      /* Odd... The target was able to list threads, but not
+		 tell us which thread was current (no "thread"
+		 register in T stop reply?).  Just pick the first
+		 thread in the thread list then.  */
+	      inferior_ptid = thread_list->ptid;
+	    }
+	}
 
       /* init_wait_for_inferior should be called before get_offsets in order
 	 to manage `inserted' flag in bp loc in a correct state.
diff --git a/gdb/testsuite/gdb.threads/reconnect-signal.c b/gdb/testsuite/gdb.threads/reconnect-signal.c
new file mode 100644
index 0000000..e63c565
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/reconnect-signal.c
@@ -0,0 +1,67 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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 <pthread.h>
+#include <signal.h>
+#include <unistd.h>
+
+static pthread_t thread_2;
+sig_atomic_t unlocked;
+
+/* The test has three threads, and it's always thread 2 that gets the
+   signal, to avoid spurious passes in case the remote side happens to
+   always pick the first or the last thread in the list as the
+   current/status thread on reconnection.  */
+
+static void *
+start2 (void *arg)
+{
+  unsigned int count;
+
+  pthread_kill (thread_2, SIGUSR1);
+
+  for (count = 1; !unlocked && count != 0; count++)
+    usleep (1);
+  return NULL;
+}
+
+static void *
+start (void *arg)
+{
+  pthread_t thread;
+
+  pthread_create (&thread, NULL, start2, NULL);
+  pthread_join (thread, NULL);
+  return NULL;
+}
+
+void
+handle (int sig)
+{
+  unlocked = 1;
+}
+
+int
+main ()
+{
+  signal (SIGUSR1, handle);
+
+  pthread_create (&thread_2, NULL, start, NULL);
+  pthread_join (thread_2, NULL);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/reconnect-signal.exp b/gdb/testsuite/gdb.threads/reconnect-signal.exp
new file mode 100644
index 0000000..47a32b3
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/reconnect-signal.exp
@@ -0,0 +1,84 @@
+# Copyright 2013 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 disconnecting and reconnecting doesn't lose signals.
+
+set gdbserver_reconnect_p 1
+if { [info proc gdb_reconnect] == "" } {
+    return 0
+}
+
+standard_testfile
+set executable ${testfile}
+
+if { [gdb_compile_pthreads \
+	  "${srcdir}/${subdir}/${srcfile}" \
+	  "${binfile}" \
+	  executable {debug}] != "" } {
+    untested "Couldn't compile test program."
+    return -1
+}
+
+clean_restart $executable
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+gdb_test "continue" "signal SIGUSR1.*" "continue to signal"
+
+# Check that it's thread 2 that is selected.
+gdb_test "info threads" "\\* 2 .*" "thread 2 is selected"
+
+set msg "save \$pc after signal"
+set saved_pc ""
+gdb_test_multiple "print/x \$pc" $msg {
+    -re "\\\$$decimal = (\[^\r\n\]*)\r\n$gdb_prompt $" {
+	set saved_pc $expect_out(1,string)
+	pass $msg
+    }
+}
+
+# Switch to the other thread.
+gdb_test "thread 1" "thread 1.*" "switch to thread 1"
+
+# Force GDB to select thread 1 on the remote end as well.
+gdb_test "print/x \$pc"
+
+gdb_test "disconnect" "Ending remote debugging\\." "disconnect after signal"
+
+set test "reconnect after signal"
+
+set res [gdb_reconnect]
+if { [lindex $res 0] == 0 } {
+    pass $test
+} else {
+    fail $test
+    return 0
+}
+
+# Check that thread 2 is re-selected.
+gdb_test "info threads" "\\* 2 .*" "thread 2 is selected on reconnect"
+
+# Check that the program is still alive, and stopped in the same spot.
+gdb_test "print/x \$pc" "\\\$$decimal = $saved_pc" "check \$pc after signal"
+
+# Check that we didn't lose the signal.
+gdb_test "info program" "stopped with signal SIGUSR1,.*"
+
+# Nor does the program.
+gdb_test "b handle" "Breakpoint .*" "set breakpoint in signal handler"
+gdb_test "continue" "handle.*" "continue to signal handler"
-- 
1.7.11.7

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

[Yao Qi]

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

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

[Pedro Alves]

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

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

[Yao Qi]

On 11/06/2013 08:12 PM, Pedro Alves wrote:
> 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.

That is clear to me now, thanks.

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

Yes, I agree.  I have no other comments on this patch.

-- 
Yao (齐尧)

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

[Pedro Alves]

On 11/06/2013 12:28 PM, Yao Qi wrote:
> On 11/06/2013 08:12 PM, Pedro Alves wrote:
>> 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.
> 
> That is clear to me now, thanks.

Thanks.  I've now pushed the patch and ...

>>>>>> @@ -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.
>>
> 
> Yes, I agree.  I have no other comments on this patch.

... pushed the following patch on top.

-----------
Subject: [PATCH] GDBserver: Discard previous queued events when GDB
 disconnects.

... not when a new GDB connection sends the status packet ('?').
Mainly just a cleanup/simplification, as GDB always sends '?' first.

Tested on x86_64 Fedora 17.

2014-01-08  Pedro Alves  <palves@redhat.com>

	* server.c (handle_status): Don't discard previous queued stop
	replies or thread's pending status here.
	(main) <disconnection>: Do it here instead.
---
 gdb/gdbserver/ChangeLog | 6 ++++++
 gdb/gdbserver/server.c  | 9 ++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index bf874a1..adba6f6 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,11 @@
 2014-01-08  Pedro Alves  <palves@redhat.com>
 
+	* server.c (handle_status): Don't discard previous queued stop
+	replies or thread's pending status here.
+	(main) <disconnection>: Do it here instead.
+
+2014-01-08  Pedro Alves  <palves@redhat.com>
+
 	* gdbthread.h (struct thread_info) <status_pending_p>: New field.
 	* server.c (visit_actioned_threads, handle_pending_status): New
 	function.
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 5e80075..c9d9eec 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2635,9 +2635,6 @@ 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.  */
@@ -3140,6 +3137,12 @@ main (int argc, char *argv[])
 	       "Remote side has terminated connection.  "
 	       "GDBserver will reopen the connection.\n");
 
+      /* Get rid of any pending statuses.  An eventual reconnection
+	 (by the same GDB instance or another) will refresh all its
+	 state from scratch.  */
+      discard_queued_stop_replies (-1);
+      for_each_inferior (&all_threads, clear_pending_status_callback);
+
       if (tracing)
 	{
 	  if (disconnected_tracing)
-- 
1.7.11.7



-- 
Pedro Alves