[remote/rfc] Let GDB know if a remote server originally attached to a process.

[Pedro Alves <pedro at codesourcery dot com>]

Hi folks,

When gdbserver is used to attach to a running
process ('gdbserver --attach PID'), exiting GDB kills the inferior.  By
contrast, when GDB is attached to a running process, exiting GDB detaches.

There's an obvious workaround for this (detach explicitly, then quit GDB),
but this ought to be changed for consistency (*).

The issue here, is that GDB has no idea that gdbserver originaly
attached to the remote process, instead of having created
it (vs gdbserver :9999 foo).  This suggests that we need a new remote
protocol query.

I've came up with the attached patch as a sort of proof-of-concept.  This
is what happens with the patch installed:

 $ gdbserver :9999 --attach 10211

 (gdb) tar remote :9999
 Remote debugging using :9999
 (...)
 0x00007fc77a1b6796 in pthread_join () from /lib/libpthread.so.0
 (gdb) q
 The program is running.  Quit anyway (and detach it)? (y or n)    
                                       ^^^^^^^^^^^^^

Whereas with current GDB one gets:

 (gdb) q
 The program is running.  Quit anyway (and kill it)? (y or n)  
                                       ^^^^^^^^^^^

(another example, is, connecting to gdbserver with 'target extended-remote',
then issue an "attach" command.  In this case GDB *does* know that the
inferior was attached to.  Now, issue a "disconnect", and reconnect again.
This time, although gdbserver is still controlling the same inferior, GDB
doesn't know anymore that gdbserver originally attached to the inferior,
and will now offer to "kill" it.  )

This works by simply defining a new "qAttached:PID"/"qAttached" query to
GDB, to which the stub returns "1" --- attached, "0" --- not
attached, or "" --- not supported, as for all other optional packets.

This is simple enough --- the patch is smaller than this
whole description of it :-)  I was considering if we want to
generalize a mechanism for this or not, say similar to target
descriptions, but I don't have any other use case for such a thing.
The target description mechanism as is currently isn't exactly right
for this case.  E.g., there's only a single description for a
target interface, not one per inferior.  That *may* need to change
in the future, but I'm not certain yet.

What do you think?  Should I just stick with the below and go
write documentation for it?  Or...?


(*) - That was the simple version, which actually did come through a customer
request.  Here's another, albeit more complicated use case, which ends up
needing the same solution:  I have a multi-process aware remote stub, for
this OS where code sections are shared between processes.  One of the features
this enabled was support for so called "global breakpoints".  That is,
we insert a breakpoint at a given address, as usual, but, what can happen
is that a process that GDB and the stub wasn't attached to yet, hits
this breakpoint.  When this happens, the stub "auto-attaches" to the process
that hit the breakpoint, and reports the SIGTRAP as usual to GDB, which on
its end adds this process to its internal inferior table.  So far, so good,
all is simple in this regard.  Thing is, GDB has no way to know that we
had "attached" to these processes to begin with, and that we want
"quit" to *not* kill these processes, but to instead "detach" from them.  This
is seriously annoying when we add non-stop mode to the mix, since in
that case, we have to carefully and manually remove all breakpoints (in
case a process auto attaches just while we were exiting), then detach from
all inferiors manually, and only can we safelly "quit" from GDB.

-- 
Pedro Alves

2009-03-04  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* remote.c (PACKET_qAttached): New.
	(remote_query_attached): New.
	(remote_add_inferior): New.
	(notice_new_inferiors): Use it.
	(remote_start_remote): Use it.
	(_initialize_remote): Add "set remote query-attached-packet"
	command.

2009-03-04  Pedro Alves  <pedro@codesourcery.com>

	gdb/gdbserver/
	* server.c (handle_query): Handle "qAttached".

---
 gdb/gdbserver/server.c |    7 ++++++
 gdb/remote.c           |   57 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 61 insertions(+), 3 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2009-03-04 00:38:28.000000000 +0000
+++ src/gdb/remote.c	2009-03-04 00:41:14.000000000 +0000
@@ -992,6 +992,7 @@ enum {
   PACKET_vKill,
   PACKET_qXfer_siginfo_read,
   PACKET_qXfer_siginfo_write,
+  PACKET_qAttached,
   PACKET_MAX
 };
 
@@ -1118,6 +1119,53 @@ static ptid_t any_thread_ptid;
 static ptid_t general_thread;
 static ptid_t continue_thread;
 
+/* Try to find out if the stub attached to PID (and hence GDB should
+   detach instead of killing it when bailing out).  */
+
+static int
+remote_query_attached (int pid)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  if (remote_protocol_packets[PACKET_qAttached].support == PACKET_DISABLE)
+    return 0;
+
+  if (remote_multi_process_p (rs))
+    sprintf (rs->buf, "qAttached:%x", pid);
+  else
+    sprintf (rs->buf, "qAttached");
+
+  putpkt (rs->buf);
+  getpkt (&rs->buf, &rs->buf_size, 0);
+
+  if (packet_ok (rs->buf,
+		 &remote_protocol_packets[PACKET_qAttached]) == PACKET_OK)
+    {
+      if (strcmp (rs->buf, "1") == 0)
+	return 1;
+    }
+
+  return 0;
+}
+
+static void
+remote_add_inferior (int pid)
+{
+  struct remote_state *rs = get_remote_state ();
+  struct inferior *inf;
+  int attached;
+
+  /* Check whether this process we're learning about is to be
+     considered attached, or if is to be considered to have been
+     spawned by the stub.  Do this before adding the inferior to the
+     tables, in case we throw an unexpected error, to avoid leaving
+     things inconsistent.  */
+  attached = remote_query_attached (pid);
+
+  inf = add_inferior (pid);
+  inf->attach_flag = attached;
+}
+
 static void
 notice_new_inferiors (ptid_t currthread)
 {
@@ -1161,7 +1209,7 @@ notice_new_inferiors (ptid_t currthread)
 	 may not know about it yet.  Add it before adding its child
 	 thread, so notifications are emitted in a sensible order.  */
       if (!in_inferior_list (ptid_get_pid (currthread)))
-	add_inferior (ptid_get_pid (currthread));
+	remote_add_inferior (ptid_get_pid (currthread));
 
       /* This is really a new thread.  Add it.  */
       add_thread (currthread);
@@ -2157,7 +2205,7 @@ remote_threads_info (struct target_ops *
 			 threads, so notifications are emitted in a
 			 sensible order.  */
 		      if (!in_inferior_list (ptid_get_pid (new_thread)))
-			add_inferior (ptid_get_pid (new_thread));
+			remote_add_inferior (ptid_get_pid (new_thread));
 
 		      add_thread (new_thread);
 
@@ -2628,7 +2676,7 @@ remote_start_remote (struct ui_out *uiou
       /* Now, if we have thread information, update inferior_ptid.  */
       inferior_ptid = remote_current_thread (inferior_ptid);
 
-      add_inferior (ptid_get_pid (inferior_ptid));
+      remote_add_inferior (ptid_get_pid (inferior_ptid));
 
       /* Always add the main thread.  */
       add_thread_silent (inferior_ptid);
@@ -9127,6 +9175,9 @@ Show the maximum size of the address (in
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vKill],
 			 "vKill", "kill", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_qAttached],
+			 "qAttached", "query-attached", 0);
+
   /* Keep the old ``set remote Z-packet ...'' working.  Each individual
      Z sub-packet has its own set and show commands, but users may
      have sets to this variable in their .gdbinit files (or in their
Index: src/gdb/gdbserver/server.c
===================================================================
--- src.orig/gdb/gdbserver/server.c	2009-03-04 00:38:28.000000000 +0000
+++ src/gdb/gdbserver/server.c	2009-03-04 00:41:14.000000000 +0000
@@ -1040,6 +1040,13 @@ handle_query (char *own_buf, int packet_
       return;
     }
 
+  if (strcmp (own_buf, "qAttached") == 0)
+    {
+      require_running (own_buf);
+      strcpy (own_buf, attached ? "1" : "0");
+      return;
+    }
+
   /* Otherwise we didn't know what packet it was.  Say we didn't
      understand it.  */
   own_buf[0] = 0;