Re: QPassSignals patch to go with proposed protocol

[Daniel Jacobowitz <drow at false dot org>]

On Fri, Oct 27, 2006 at 10:11:06AM +0200, Eli Zaretskii wrote:
> 
> Is the ``inform'' part really necessary?  How about this instead?
> 
>   @cindex remote target, signals to ignore
> 
> Other than that, the new text is okay with me.  Thanks.

As I slowly clean out my backlog... sometimes I wonder if I'll ever
catch up with myself.

Eli, you had a legitimate concern about the relationship between the
new command and "handle".  I believe this is fixed in the current
manual (by the patch I checked in earlier today, which rewrote the
relevant section of Remote Configuration).  If so, this revised patch
(which adds an entry to that new table) ought to be just what we
wanted.  Could you take a look?  If it isn't, I'm happy to revise,
but I'll need a pointer.

-- 
Daniel Jacobowitz
CodeSourcery

2006-11-14  Daniel Jacobowitz  <dan@codesourcery.com>

	* remote.c (PACKET_QPassSignals): New.
	(last_pass_packet, remote_pass_signals): New.
	(remote_protocol_features): Add QPassSignals.
	(remote_query_supported): Correct an infinite loop.
	(remote_open_1): Reset last_pass_packet.
	(remote_resume): Call remote_pass_signals.
	(_initialize_remote): Register "set remote pass-signals".

2006-11-14  Daniel Jacobowitz  <dan@codesourcery.com>

	* gdb.texinfo (Remote configuration): Mention
	"pass-signals-packet".
	(General Query Packets): Document QPassSignals.  Fix
	a typo.

2006-11-14  Daniel Jacobowitz  <dan@codesourcery.com>

	* linux-low.c (linux_wait_for_event): Reformat.  Use the
	pass_signals array.
	* remote-utils.c (decode_address_to_semicolon): New.
	* server.c (pass_signals, handle_general_set): New.
	(handle_query): Mention QPassSignals for qSupported.
	(main): Call handle_general_set.
	* server.h (pass_signals, decode_address_to_semicolon): New.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.237
diff -u -p -r1.237 remote.c
--- remote.c	14 Nov 2006 21:40:19 -0000	1.237
+++ remote.c	14 Nov 2006 22:31:58 -0000
@@ -867,6 +867,7 @@ enum {
   PACKET_qXfer_memory_map,
   PACKET_qGetTLSAddr,
   PACKET_qSupported,
+  PACKET_QPassSignals,
   PACKET_MAX
 };
 
@@ -1004,6 +1005,65 @@ record_currthread (int currthread)
     }
 }
 
+static char *last_pass_packet;
+
+/* If 'QPassSignals' is supported, tell the remote stub what signals
+   it can simply pass through to the inferior without reporting.  */
+
+static void
+remote_pass_signals (void)
+{
+  if (remote_protocol_packets[PACKET_QPassSignals].support != PACKET_DISABLE)
+    {
+      char *pass_packet, *p;
+      int numsigs = (int) TARGET_SIGNAL_LAST;
+      int count = 0, i;
+
+      gdb_assert (numsigs < 256);
+      for (i = 0; i < numsigs; i++)
+	{
+	  if (signal_stop_state (i) == 0
+	      && signal_print_state (i) == 0
+	      && signal_pass_state (i) == 1)
+	    count++;
+	}
+      pass_packet = xmalloc (count * 3 + strlen ("QPassSignals:") + 1);
+      strcpy (pass_packet, "QPassSignals:");
+      p = pass_packet + strlen (pass_packet);
+      for (i = 0; i < numsigs; i++)
+	{
+	  if (signal_stop_state (i) == 0
+	      && signal_print_state (i) == 0
+	      && signal_pass_state (i) == 1)
+	    {
+	      if (i >= 16)
+		*p++ = tohex (i >> 4);
+	      *p++ = tohex (i & 15);
+	      if (count)
+		*p++ = ';';
+	      else
+		break;
+	      count--;
+	    }
+	}
+      *p = 0;
+      if (!last_pass_packet || strcmp (last_pass_packet, pass_packet))
+	{
+	  struct remote_state *rs = get_remote_state ();
+	  char *buf = rs->buf;
+
+	  putpkt (pass_packet);
+	  getpkt (&rs->buf, &rs->buf_size, 0);
+	  packet_ok (buf, &remote_protocol_packets[PACKET_QPassSignals]);
+	  if (last_pass_packet)
+	    xfree (last_pass_packet);
+	  last_pass_packet = pass_packet;
+	}
+      else
+	xfree (pass_packet);
+    }
+}
+
 #define MAGIC_NULL_PID 42000
 
 static void
@@ -2206,7 +2266,9 @@ static struct protocol_feature remote_pr
   { "qXfer:auxv:read", PACKET_DISABLE, remote_supported_packet,
     PACKET_qXfer_auxv },
   { "qXfer:memory-map:read", PACKET_DISABLE, remote_supported_packet,
-    PACKET_qXfer_memory_map }
+    PACKET_qXfer_memory_map },
+  { "QPassSignals", PACKET_DISABLE, remote_supported_packet,
+    PACKET_QPassSignals },
 };
 
 static void
@@ -2260,14 +2322,14 @@ remote_query_supported (void)
 	}
       else
 	{
+	  *end = '\0';
+	  next = end + 1;
+
 	  if (end == p)
 	    {
 	      warning (_("empty item in \"qSupported\" response"));
 	      continue;
 	    }
-
-	  *end = '\0';
-	  next = end + 1;
 	}
 
       name_end = strchr (p, '=');
@@ -2354,6 +2416,10 @@ remote_open_1 (char *name, int from_tty,
 
   unpush_target (target);
 
+  /* Make sure we send the passed signals list the next time we resume.  */
+  xfree (last_pass_packet);
+  last_pass_packet = NULL;
+
   remote_fileio_reset ();
   reopen_exec_file ();
   reread_symbols ();
@@ -2728,6 +2794,9 @@ remote_resume (ptid_t ptid, int step, en
   if (deprecated_target_resume_hook)
     (*deprecated_target_resume_hook) ();
 
+  /* Update the inferior on signals to silently pass, if they've changed.  */
+  remote_pass_signals ();
+
   /* The vCont packet doesn't need to specify threads via Hc.  */
   if (remote_vcont_resume (ptid, step, siggnal))
     return;
@@ -6281,6 +6350,9 @@ Show the maximum size of the address (in
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vCont],
 			 "vCont", "verbose-resume", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_QPassSignals],
+			 "QPassSignals", "pass-signals", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_qSymbol],
 			 "qSymbol", "symbol-lookup", 0);
 
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.368
diff -u -p -r1.368 gdb.texinfo
--- doc/gdb.texinfo	14 Nov 2006 21:53:59 -0000	1.368
+++ doc/gdb.texinfo	14 Nov 2006 22:32:01 -0000
@@ -12841,6 +12841,10 @@ The available settings are:
 @tab @code{qSupported}
 @tab Remote communications parameters
 
+@item @code{pass-signals-packet}
+@tab @code{QPassSignals}
+@tab @code{handle @var{signal}}
+
 @end multitable
 
 @node remote stub
@@ -23347,8 +23351,8 @@ The @samp{C}, @samp{c}, @samp{S}, @samp{
 receive any of the below as a reply.  In the case of the @samp{C},
 @samp{c}, @samp{S} and @samp{s} packets, that reply is only returned
 when the target halts.  In the below the exact meaning of @dfn{signal
-number} is poorly defined.  In general one of the UNIX signal
-numbering conventions is used.
+number} is defined by the header @file{include/gdb/signals.h} in the
+@value{GDBN} source code.
 
 As in the description of request packets, we include spaces in the
 reply templates for clarity; these are not part of the reply packet's
@@ -23604,6 +23608,37 @@ Don't use this packet; use the @samp{qTh
 
 Reply: see @code{remote.c:remote_unpack_thread_info_response()}.
 
+@item QPassSignals: @var{signal} @r{[};@var{signal}@r{]}@dots{}
+@cindex pass signals to inferior, remote request
+@cindex @samp{QPassSignals} packet
+Each listed @var{signal} should be passed directly to the inferior process. 
+Signals are numbered identically to continue packets and stop replies
+(@pxref{Stop Reply Packets}).  Each @var{signal} list item should be
+strictly greater than the previous item.  These signals do not need to stop
+the inferior, or be reported to @value{GDBN}.  All other signals should be
+reported to @value{GDBN}.  Multiple @samp{QPassSignals} packets do not
+combine; any earlier @samp{QPassSignals} list is completely replaced by the
+new list.  This packet improves performance when using @samp{handle
+@var{signal} nostop noprint pass}.
+
+Reply:
+@table @samp
+@item OK
+The request succeeded.
+
+@item E @var{nn}
+An error occurred.  @var{nn} are hex digits.
+
+@item
+An empty reply indicates that @samp{QPassSignals} is not supported by
+the stub.
+@end table
+
+Use of this packet is controlled by the @code{set remote pass-signals}
+command (@pxref{Remote configuration, set remote pass-signals}).
+This packet is not probed by default; the remote stub must request it,
+by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
+
 @item qRcmd,@var{command}
 @cindex execute remote command, remote request
 @cindex @samp{qRcmd} packet
@@ -23747,6 +23782,11 @@ These are the currently defined stub fea
 @tab @samp{-}
 @tab Yes
 
+@item @samp{QPassSignals}
+@tab No
+@tab @samp{-}
+@tab Yes
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -23862,7 +23902,7 @@ Access the target's @dfn{auxiliary vecto
 auxiliary vector}.  Note @var{annex} must be empty.
 
 This packet is not probed by default; the remote stub must request it,
-by suppling an appropriate @samp{qSupported} response (@pxref{qSupported}).
+by supplying an appropriate @samp{qSupported} response (@pxref{qSupported}).
 @end table
 
 @table @samp
Index: gdbserver/linux-low.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/linux-low.c,v
retrieving revision 1.47
diff -u -p -r1.47 linux-low.c
--- gdbserver/linux-low.c	17 Oct 2006 16:02:27 -0000	1.47
+++ gdbserver/linux-low.c	14 Nov 2006 22:32:02 -0000
@@ -498,69 +498,76 @@ linux_wait_for_event (struct thread_info
       current_inferior = (struct thread_info *)
 	find_inferior_id (&all_threads, event_child->tid);
 
-      if (using_threads)
+      /* Check for thread exit.  */
+      if (using_threads && ! WIFSTOPPED (wstat))
 	{
-	  /* Check for thread exit.  */
-	  if (! WIFSTOPPED (wstat))
-	    {
-	      if (debug_threads)
-		fprintf (stderr, "Thread %ld (LWP %ld) exiting\n",
-			 event_child->tid, event_child->head.id);
-
-	      /* If the last thread is exiting, just return.  */
-	      if (all_threads.head == all_threads.tail)
-		return wstat;
-
-	      dead_thread_notify (event_child->tid);
-
-	      remove_inferior (&all_processes, &event_child->head);
-	      free (event_child);
-	      remove_thread (current_inferior);
-	      current_inferior = (struct thread_info *) all_threads.head;
-
-	      /* If we were waiting for this particular child to do something...
-		 well, it did something.  */
-	      if (child != NULL)
-		return wstat;
+	  if (debug_threads)
+	    fprintf (stderr, "Thread %ld (LWP %ld) exiting\n",
+		     event_child->tid, event_child->head.id);
 
-	      /* Wait for a more interesting event.  */
-	      continue;
-	    }
+	  /* If the last thread is exiting, just return.  */
+	  if (all_threads.head == all_threads.tail)
+	    return wstat;
+
+	  dead_thread_notify (event_child->tid);
+
+	  remove_inferior (&all_processes, &event_child->head);
+	  free (event_child);
+	  remove_thread (current_inferior);
+	  current_inferior = (struct thread_info *) all_threads.head;
+
+	  /* If we were waiting for this particular child to do something...
+	     well, it did something.  */
+	  if (child != NULL)
+	    return wstat;
 
-	  if (WIFSTOPPED (wstat)
-	      && WSTOPSIG (wstat) == SIGSTOP
-	      && event_child->stop_expected)
-	    {
-	      if (debug_threads)
-		fprintf (stderr, "Expected stop.\n");
-	      event_child->stop_expected = 0;
-	      linux_resume_one_process (&event_child->head,
-					event_child->stepping, 0, NULL);
-	      continue;
-	    }
+	  /* Wait for a more interesting event.  */
+	  continue;
+	}
 
-	  /* FIXME drow/2002-06-09: Get signal numbers from the inferior's
-	     thread library?  */
-	  if (WIFSTOPPED (wstat)
-	      && (WSTOPSIG (wstat) == __SIGRTMIN
-		  || WSTOPSIG (wstat) == __SIGRTMIN + 1))
-	    {
-	      siginfo_t info, *info_p;
+      if (using_threads
+	  && WIFSTOPPED (wstat)
+	  && WSTOPSIG (wstat) == SIGSTOP
+	  && event_child->stop_expected)
+	{
+	  if (debug_threads)
+	    fprintf (stderr, "Expected stop.\n");
+	  event_child->stop_expected = 0;
+	  linux_resume_one_process (&event_child->head,
+				    event_child->stepping, 0, NULL);
+	  continue;
+	}
 
-	      if (debug_threads)
-		fprintf (stderr, "Ignored signal %d for %ld (LWP %ld).\n",
-			 WSTOPSIG (wstat), event_child->tid,
-			 event_child->head.id);
+      /* If GDB is not interested in this signal, don't stop other
+	 threads, and don't report it to GDB.  Just resume the
+	 inferior right away.  We do this for threading-related
+	 signals as well as any that GDB specifically requested
+	 we ignore.  But never ignore SIGSTOP if we sent it
+	 ourselves.  */
+      /* FIXME drow/2002-06-09: Get signal numbers from the inferior's
+	 thread library?  */
+      if (WIFSTOPPED (wstat)
+	  && ((using_threads && (WSTOPSIG (wstat) == __SIGRTMIN
+				 || WSTOPSIG (wstat) == __SIGRTMIN + 1))
+	      || (pass_signals[target_signal_from_host (WSTOPSIG (wstat))]
+		  && (WSTOPSIG (wstat) != SIGSTOP
+		      || !event_child->sigstop_sent))))
+	{
+	  siginfo_t info, *info_p;
 
-	      if (ptrace (PTRACE_GETSIGINFO, event_child->lwpid, 0, &info) == 0)
-		info_p = &info;
-	      else
-		info_p = NULL;
-	      linux_resume_one_process (&event_child->head,
-					event_child->stepping,
-					WSTOPSIG (wstat), info_p);
-	      continue;
-	    }
+	  if (debug_threads)
+	    fprintf (stderr, "Ignored signal %d for %ld (LWP %ld).\n",
+		     WSTOPSIG (wstat), event_child->tid,
+		     event_child->head.id);
+
+	  if (ptrace (PTRACE_GETSIGINFO, event_child->lwpid, 0, &info) == 0)
+	    info_p = &info;
+	  else
+	    info_p = NULL;
+	  linux_resume_one_process (&event_child->head,
+				    event_child->stepping,
+				    WSTOPSIG (wstat), info_p);
+	  continue;
 	}
 
       /* If this event was not handled above, and is not a SIGTRAP, report
Index: gdbserver/remote-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/remote-utils.c,v
retrieving revision 1.33
diff -u -p -r1.33 remote-utils.c
--- gdbserver/remote-utils.c	17 Oct 2006 16:02:27 -0000	1.33
+++ gdbserver/remote-utils.c	14 Nov 2006 22:32:02 -0000
@@ -296,6 +296,22 @@ decode_address (CORE_ADDR *addrp, const 
   *addrp = addr;
 }
 
+const char *
+decode_address_to_semicolon (CORE_ADDR *addrp, const char *start)
+{
+  const char *end;
+
+  end = start;
+  while (*end != '\0' && *end != ';')
+    end++;
+
+  decode_address (addrp, start, end - start);
+
+  if (*end == ';')
+    end++;
+  return end;
+}
+
 /* Convert number NIB to a hex digit.  */
 
 static int
Index: gdbserver/server.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.c,v
retrieving revision 1.41
diff -u -p -r1.41 server.c
--- gdbserver/server.c	6 Nov 2006 21:50:32 -0000	1.41
+++ gdbserver/server.c	14 Nov 2006 22:32:02 -0000
@@ -36,6 +36,8 @@ unsigned long old_thread_from_wait;
 int extended_protocol;
 int server_waiting;
 
+int pass_signals[TARGET_SIGNAL_LAST];
+
 jmp_buf toplevel;
 
 /* The PID of the originally created or attached inferior.  Used to
@@ -157,6 +159,40 @@ write_qxfer_response (char *buf, unsigne
 			       PBUFSIZ - 2) + 1;
 }
 
+/* Handle all of the extended 'Q' packets.  */
+void
+handle_general_set (char *own_buf)
+{
+  if (strncmp ("QPassSignals:", own_buf, strlen ("QPassSignals:")) == 0)
+    {
+      int numsigs = (int) TARGET_SIGNAL_LAST, i;
+      const char *p = own_buf + strlen ("QPassSignals:");
+      CORE_ADDR cursig;
+
+      p = decode_address_to_semicolon (&cursig, p);
+      for (i = 0; i < numsigs; i++)
+	{
+	  if (i == cursig)
+	    {
+	      pass_signals[i] = 1;
+	      if (*p == '\0')
+		/* Keep looping, to clear the remaining signals.  */
+		cursig = -1;
+	      else
+		p = decode_address_to_semicolon (&cursig, p);
+	    }
+	  else
+	    pass_signals[i] = 0;
+	}
+      strcpy (own_buf, "OK");
+      return;
+    }
+
+  /* Otherwise we didn't know what packet it was.  Say we didn't
+     understand it.  */
+  own_buf[0] = 0;
+}
+
 /* Handle all of the extended 'q' packets.  */
 void
 handle_query (char *own_buf, int *new_packet_len_p)
@@ -248,7 +284,7 @@ handle_query (char *own_buf, int *new_pa
   if (strncmp ("qSupported", own_buf, 10) == 0
       && (own_buf[10] == ':' || own_buf[10] == '\0'))
     {
-      sprintf (own_buf, "PacketSize=%x", PBUFSIZ - 1);
+      sprintf (own_buf, "PacketSize=%x;QPassSignals+", PBUFSIZ - 1);
 
       if (the_target->read_auxv != NULL)
 	strcat (own_buf, ";qXfer:auxv:read+");
@@ -601,6 +637,9 @@ main (int argc, char *argv[])
 	    case 'q':
 	      handle_query (own_buf, &new_packet_len);
 	      break;
+	    case 'Q':
+	      handle_general_set (own_buf);
+	      break;
 	    case 'd':
 	      remote_debug = !remote_debug;
 	      break;
Index: gdbserver/server.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbserver/server.h,v
retrieving revision 1.26
diff -u -p -r1.26 server.h
--- gdbserver/server.h	17 Oct 2006 16:02:27 -0000	1.26
+++ gdbserver/server.h	14 Nov 2006 22:32:02 -0000
@@ -129,6 +129,7 @@ extern unsigned long step_thread;
 extern unsigned long thread_from_wait;
 extern unsigned long old_thread_from_wait;
 extern int server_waiting;
+extern int pass_signals[];
 
 extern jmp_buf toplevel;
 
@@ -153,6 +154,7 @@ void new_thread_notify (int id);
 void dead_thread_notify (int id);
 void prepare_resume_reply (char *buf, char status, unsigned char sig);
 
+const char *decode_address_to_semicolon (CORE_ADDR *addrp, const char *start);
 void decode_address (CORE_ADDR *addrp, const char *start, int len);
 void decode_m_packet (char *from, CORE_ADDR * mem_addr_ptr,
 		      unsigned int *len_ptr);