RFA/RFC: vCont for the remote protocol [client]

RFA/RFC: vCont for the remote protocol [client]

[Daniel Jacobowitz]

Hi all,

Here's the client-side support for the new vCont resume packet that Andrew
and I worked out on gdb@:
  http://sources.redhat.com/ml/gdb/2003-09/msg00259.html

Documentation and gdbserver support will follow momentarily.  Highlights:
  - The protocol and gdbserver implementations support freezing threads,
    et cetera.  The GDB part doesn't, because that requires substantial
    poking and prodding at the target interface for target_resume; but
    this cuts down one of the barriers should someone want to implement it.
  - I replaced most of remote_async_resume with a call to remote_resume
    after verifying that they were exactly identical.

Thoughts?  OK, desired protocol changes, ?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-09-29  Daniel Jacobowitz  <drow@mvista.com>

	* remote.c (remote_protocol_vcont): New variable.
	(set_remote_protocol_vcont_packet_cmd): New function.
	(show_remote_protocol_vcont_packet_cmd): New function.
	(init_all_packet_configs): Handle remote_protocol_vcont.
	(remote_vcont_resume): New function.
	(remote_resume): Use it.
	(remote_async_resume): Call remote_resume.
	(_initialize_remote): Add verbose-resume packet commands.

Index: remote.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/remote.c,v
retrieving revision 1.115
diff -u -p -r1.115 remote.c
--- remote.c	21 Sep 2003 01:26:45 -0000	1.115
+++ remote.c	29 Sep 2003 14:26:08 -0000
@@ -749,6 +749,23 @@ packet_ok (const char *buf, struct packe
     }
 }
 
+/* Should we try the 'vCont' (descriptive resume) request? */
+static struct packet_config remote_protocol_vcont;
+
+static void
+set_remote_protocol_vcont_packet_cmd (char *args, int from_tty,
+				      struct cmd_list_element *c)
+{
+  update_packet_config (&remote_protocol_vcont);
+}
+
+static void
+show_remote_protocol_vcont_packet_cmd (char *args, int from_tty,
+				       struct cmd_list_element *c)
+{
+  show_packet_config_cmd (&remote_protocol_vcont);
+}
+
 /* Should we try the 'qSymbol' (target symbol lookup service) request? */
 static struct packet_config remote_protocol_qSymbol;
 
@@ -2190,6 +2207,7 @@ init_all_packet_configs (void)
   update_packet_config (&remote_protocol_E);
   update_packet_config (&remote_protocol_P);
   update_packet_config (&remote_protocol_qSymbol);
+  update_packet_config (&remote_protocol_vcont);
   for (i = 0; i < NR_Z_PACKET_TYPES; i++)
     update_packet_config (&remote_protocol_Z[i]);
   /* Force remote_write_bytes to check whether target supports binary
@@ -2503,6 +2521,75 @@ bin2hex (const char *bin, char *hex, int
   return i;
 }
 \f
+/* Resume the remote inferior by using a "vCont" packet.  The thread
+   to be resumed is PTID; STEP and SIGGNAL indicate whether the
+   resumed thread should be single-stepped and/or signalled.  If PTID's
+   PID is -1, then all threads are resumed; the thread to be stepped and/or
+   signalled is given in the global INFERIOR_PTID.  This function returns
+   non-zero iff it resumes the inferior.
+
+   This function issues a strict subset of all possible vCont commands at the
+   moment.  */
+
+static int
+remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal)
+{
+  struct remote_state *rs = get_remote_state ();
+  int pid = PIDGET (ptid);
+  char *buf = alloca (rs->remote_packet_size);
+
+  if (remote_protocol_vcont.support == PACKET_DISABLE)
+    return 0;
+
+  /* If we could generate a wider range of packets, we'd have to worry
+     about overflowing BUF.  Should there be a generic
+     "multi-part-packet" packet?  */
+
+  if (PIDGET (inferior_ptid) == MAGIC_NULL_PID)
+    {
+      /* MAGIC_NULL_PTID means that we don't have any active threads, so we
+	 don't have any PID numbers the inferior will understand.  Make sure
+	 to only send forms that do not specify a PID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont:S%02x", siggnal);
+      else if (step)
+	sprintf (buf, "vCont:s");
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont:C%02x", siggnal);
+      else
+	sprintf (buf, "vCont:c");
+    }
+  else if (pid == -1)
+    {
+      /* Resume all threads, with preference for INFERIOR_PTID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont:S%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+      else if (step)
+	sprintf (buf, "vCont:s:%x;c", PIDGET (inferior_ptid));
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont:C%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+      else
+	sprintf (buf, "vCont:c");
+    }
+  else
+    {
+      /* Scheduler locking; resume only PTID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont:S%02x:%x", siggnal, pid);
+      else if (step)
+	sprintf (buf, "vCont:s:%x", pid);
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont:C%02x:%x", siggnal, pid);
+      else
+	sprintf (buf, "vCont:c:%x", pid);
+    }
+
+  putpkt (buf);
+  getpkt (buf, rs->remote_packet_size, 0);
+
+  return (packet_ok (buf, &remote_protocol_vcont) == PACKET_OK);
+}
+
 /* Tell the remote machine to resume.  */
 
 static enum target_signal last_sent_signal = TARGET_SIGNAL_0;
@@ -2517,20 +2604,24 @@ remote_resume (ptid_t ptid, int step, en
   int pid = PIDGET (ptid);
   char *p;
 
+  last_sent_signal = siggnal;
+  last_sent_step = step;
+
+  /* The vCont packet doesn't need to specify threads via Hc.  */
+  if (remote_vcont_resume (ptid, step, siggnal))
+    return;
+
+  /* All other supported resume packets do use Hc, so call set_thread.  */
   if (pid == -1)
     set_thread (0, 0);		/* run any thread */
   else
     set_thread (pid, 0);	/* run this thread */
 
-  last_sent_signal = siggnal;
-  last_sent_step = step;
-
   /* A hook for when we need to do something at the last moment before
      resumption.  */
   if (target_resume_hook)
     (*target_resume_hook) ();
 
-
   /* The s/S/c/C packets do not return status.  So if the target does
      not support the S or C packets, the debug agent returns an empty
      string which is detected in remote_wait().  This protocol defect
@@ -2602,91 +2693,8 @@ remote_resume (ptid_t ptid, int step, en
 static void
 remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal)
 {
-  struct remote_state *rs = get_remote_state ();
-  char *buf = alloca (rs->remote_packet_size);
-  int pid = PIDGET (ptid);
-  char *p;
-
-  if (pid == -1)
-    set_thread (0, 0);		/* run any thread */
-  else
-    set_thread (pid, 0);	/* run this thread */
-
-  last_sent_signal = siggnal;
-  last_sent_step = step;
-
-  /* A hook for when we need to do something at the last moment before
-     resumption.  */
-  if (target_resume_hook)
-    (*target_resume_hook) ();
-
-  /* The s/S/c/C packets do not return status.  So if the target does
-     not support the S or C packets, the debug agent returns an empty
-     string which is detected in remote_wait().  This protocol defect
-     is fixed in the e/E packets. */
-
-  if (step && step_range_end)
-    {
-      /* If the target does not support the 'E' packet, we try the 'S'
-	 packet.  Ideally we would fall back to the 'e' packet if that
-	 too is not supported.  But that would require another copy of
-	 the code to issue the 'e' packet (and fall back to 's' if not
-	 supported) in remote_wait().  */
-      
-      if (siggnal != TARGET_SIGNAL_0)
-	{
-	  if (remote_protocol_E.support != PACKET_DISABLE)
-	    {
-	      p = buf;
-	      *p++ = 'E';
-	      *p++ = tohex (((int) siggnal >> 4) & 0xf);
-	      *p++ = tohex (((int) siggnal) & 0xf);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_start);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_end);
-	      *p++ = 0;
-
-	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
-
-	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
-		goto register_event_loop;
-	    }
-	}
-      else
-	{
-	  if (remote_protocol_e.support != PACKET_DISABLE)
-	    {
-	      p = buf;
-	      *p++ = 'e';
-	      p += hexnumstr (p, (ULONGEST) step_range_start);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_end);
-	      *p++ = 0;
-
-	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+  remote_resume (ptid, step, siggnal);
 
-	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
-		goto register_event_loop;
-	    }
-	}
-    }
-
-  if (siggnal != TARGET_SIGNAL_0)
-    {
-      buf[0] = step ? 'S' : 'C';
-      buf[1] = tohex (((int) siggnal >> 4) & 0xf);
-      buf[2] = tohex ((int) siggnal & 0xf);
-      buf[3] = '\0';
-    }
-  else
-    strcpy (buf, step ? "s" : "c");
-  
-  putpkt (buf);
-
-register_event_loop:
   /* We are about to start executing the inferior, let's register it
      with the event loop. NOTE: this is the one place where all the
      execution commands end up. We could alternatively do this in each
@@ -6019,6 +6027,7 @@ show_remote_cmd (char *args, int from_tt
   show_remote_protocol_E_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_P_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL);
+  show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
 }
 
@@ -6191,6 +6200,13 @@ in a memory packet.\n",
 
   add_info ("remote-process", remote_info_process,
 	    "Query the remote system for process info.");
+
+  add_packet_config_cmd (&remote_protocol_vcont,
+			 "vCont", "verbose-resume",
+			 set_remote_protocol_vcont_packet_cmd,
+			 show_remote_protocol_vcont_packet_cmd,
+			 &remote_set_cmdlist, &remote_show_cmdlist,
+			 0);
 
   add_packet_config_cmd (&remote_protocol_qSymbol,
 			 "qSymbol", "symbol-lookup",

Re: RFA/RFC: vCont for the remote protocol [client]

[Daniel Jacobowitz]

On Mon, Sep 29, 2003 at 11:28:31AM -0400, Daniel Jacobowitz wrote:
> Hi all,
> 
> Here's the client-side support for the new vCont resume packet that Andrew
> and I worked out on gdb@:
>   http://sources.redhat.com/ml/gdb/2003-09/msg00259.html
> 
> Documentation and gdbserver support will follow momentarily.  Highlights:
>   - The protocol and gdbserver implementations support freezing threads,
>     et cetera.  The GDB part doesn't, because that requires substantial
>     poking and prodding at the target interface for target_resume; but
>     this cuts down one of the barriers should someone want to implement it.
>   - I replaced most of remote_async_resume with a call to remote_resume
>     after verifying that they were exactly identical.

Oh, and falls back to c/C/s/S automatically, of course.

> Thoughts?  OK, desired protocol changes, ?

Here, have a revision for the protocol changes.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-09-30  Daniel Jacobowitz  <drow@mvista.com>

	* remote.c (remote_protocol_vcont): New variable.
	(set_remote_protocol_vcont_packet_cmd): New function.
	(show_remote_protocol_vcont_packet_cmd): New function.
	(init_all_packet_configs): Handle remote_protocol_vcont.
	(remote_vcont_probe): New function.
	(remote_vcont_resume): New function.
	(remote_resume): Use it.
	(remote_async_resume): Call remote_resume.
	(_initialize_remote): Add verbose-resume packet commands.

Index: remote.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/remote.c,v
retrieving revision 1.115
diff -u -p -r1.115 remote.c
--- remote.c	21 Sep 2003 01:26:45 -0000	1.115
+++ remote.c	30 Sep 2003 20:30:40 -0000
@@ -749,6 +749,23 @@ packet_ok (const char *buf, struct packe
     }
 }
 
+/* Should we try the 'vCont' (descriptive resume) request? */
+static struct packet_config remote_protocol_vcont;
+
+static void
+set_remote_protocol_vcont_packet_cmd (char *args, int from_tty,
+				      struct cmd_list_element *c)
+{
+  update_packet_config (&remote_protocol_vcont);
+}
+
+static void
+show_remote_protocol_vcont_packet_cmd (char *args, int from_tty,
+				       struct cmd_list_element *c)
+{
+  show_packet_config_cmd (&remote_protocol_vcont);
+}
+
 /* Should we try the 'qSymbol' (target symbol lookup service) request? */
 static struct packet_config remote_protocol_qSymbol;
 
@@ -2190,6 +2207,7 @@ init_all_packet_configs (void)
   update_packet_config (&remote_protocol_E);
   update_packet_config (&remote_protocol_P);
   update_packet_config (&remote_protocol_qSymbol);
+  update_packet_config (&remote_protocol_vcont);
   for (i = 0; i < NR_Z_PACKET_TYPES; i++)
     update_packet_config (&remote_protocol_Z[i]);
   /* Force remote_write_bytes to check whether target supports binary
@@ -2503,115 +2521,103 @@ bin2hex (const char *bin, char *hex, int
   return i;
 }
 \f
-/* Tell the remote machine to resume.  */
-
-static enum target_signal last_sent_signal = TARGET_SIGNAL_0;
-
-static int last_sent_step;
+/* Check for the availability of vCont.  This function should also check
+   the response.  */
 
 static void
-remote_resume (ptid_t ptid, int step, enum target_signal siggnal)
+remote_vcont_probe (struct remote_state *rs, char *buf)
 {
-  struct remote_state *rs = get_remote_state ();
-  char *buf = alloca (rs->remote_packet_size);
-  int pid = PIDGET (ptid);
-  char *p;
-
-  if (pid == -1)
-    set_thread (0, 0);		/* run any thread */
-  else
-    set_thread (pid, 0);	/* run this thread */
-
-  last_sent_signal = siggnal;
-  last_sent_step = step;
+  strcpy (buf, "vCont?");
+  putpkt (buf);
+  getpkt (buf, rs->remote_packet_size, 0);
+  packet_ok (buf, &remote_protocol_vcont);
+}
 
-  /* A hook for when we need to do something at the last moment before
-     resumption.  */
-  if (target_resume_hook)
-    (*target_resume_hook) ();
+/* Resume the remote inferior by using a "vCont" packet.  The thread
+   to be resumed is PTID; STEP and SIGGNAL indicate whether the
+   resumed thread should be single-stepped and/or signalled.  If PTID's
+   PID is -1, then all threads are resumed; the thread to be stepped and/or
+   signalled is given in the global INFERIOR_PTID.  This function returns
+   non-zero iff it resumes the inferior.
 
+   This function issues a strict subset of all possible vCont commands at the
+   moment.  */
 
-  /* The s/S/c/C packets do not return status.  So if the target does
-     not support the S or C packets, the debug agent returns an empty
-     string which is detected in remote_wait().  This protocol defect
-     is fixed in the e/E packets. */
+static int
+remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal)
+{
+  struct remote_state *rs = get_remote_state ();
+  int pid = PIDGET (ptid);
+  char *buf = alloca (rs->remote_packet_size);
 
-  if (step && step_range_end)
-    {
-      /* If the target does not support the 'E' packet, we try the 'S'
-	 packet.  Ideally we would fall back to the 'e' packet if that
-	 too is not supported.  But that would require another copy of
-	 the code to issue the 'e' packet (and fall back to 's' if not
-	 supported) in remote_wait().  */
-      
-      if (siggnal != TARGET_SIGNAL_0)
-	{
-	  if (remote_protocol_E.support != PACKET_DISABLE)
-	    {
-	      p = buf;
-	      *p++ = 'E';
-	      *p++ = tohex (((int) siggnal >> 4) & 0xf);
-	      *p++ = tohex (((int) siggnal) & 0xf);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_start);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_end);
-	      *p++ = 0;
+  if (remote_protocol_vcont.support == PACKET_SUPPORT_UNKNOWN)
+    remote_vcont_probe (rs, buf);
 
-	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+  if (remote_protocol_vcont.support == PACKET_DISABLE)
+    return 0;
 
-	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
-		return;
-	    }
-	}
+  /* If we could generate a wider range of packets, we'd have to worry
+     about overflowing BUF.  Should there be a generic
+     "multi-part-packet" packet?  */
+
+  if (PIDGET (inferior_ptid) == MAGIC_NULL_PID)
+    {
+      /* MAGIC_NULL_PTID means that we don't have any active threads, so we
+	 don't have any PID numbers the inferior will understand.  Make sure
+	 to only send forms that do not specify a PID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x", siggnal);
+      else if (step)
+	sprintf (buf, "vCont;s");
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;C%02x", siggnal);
       else
-	{
-	  if (remote_protocol_e.support != PACKET_DISABLE)
-	    {
-	      p = buf;
-	      *p++ = 'e';
-	      p += hexnumstr (p, (ULONGEST) step_range_start);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_end);
-	      *p++ = 0;
-
-	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
-
-	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
-		return;
-	    }
-	}
+	sprintf (buf, "vCont;c");
     }
-
-  if (siggnal != TARGET_SIGNAL_0)
+  else if (pid == -1)
     {
-      buf[0] = step ? 'S' : 'C';
-      buf[1] = tohex (((int) siggnal >> 4) & 0xf);
-      buf[2] = tohex (((int) siggnal) & 0xf);
-      buf[3] = '\0';
+      /* Resume all threads, with preference for INFERIOR_PTID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+      else if (step)
+	sprintf (buf, "vCont;s:%x;c", PIDGET (inferior_ptid));
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;C%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+      else
+	sprintf (buf, "vCont;c");
     }
   else
-    strcpy (buf, step ? "s" : "c");
+    {
+      /* Scheduler locking; resume only PTID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x:%x", siggnal, pid);
+      else if (step)
+	sprintf (buf, "vCont;s:%x", pid);
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;C%02x:%x", siggnal, pid);
+      else
+	sprintf (buf, "vCont;c:%x", pid);
+    }
 
   putpkt (buf);
+
+  return 1;
 }
 
-/* Same as remote_resume, but with async support. */
+/* Tell the remote machine to resume.  */
+
+static enum target_signal last_sent_signal = TARGET_SIGNAL_0;
+
+static int last_sent_step;
+
 static void
-remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal)
+remote_resume (ptid_t ptid, int step, enum target_signal siggnal)
 {
   struct remote_state *rs = get_remote_state ();
   char *buf = alloca (rs->remote_packet_size);
   int pid = PIDGET (ptid);
   char *p;
 
-  if (pid == -1)
-    set_thread (0, 0);		/* run any thread */
-  else
-    set_thread (pid, 0);	/* run this thread */
-
   last_sent_signal = siggnal;
   last_sent_step = step;
 
@@ -2620,6 +2626,16 @@ remote_async_resume (ptid_t ptid, int st
   if (target_resume_hook)
     (*target_resume_hook) ();
 
+  /* The vCont packet doesn't need to specify threads via Hc.  */
+  if (remote_vcont_resume (ptid, step, siggnal))
+    return;
+
+  /* All other supported resume packets do use Hc, so call set_thread.  */
+  if (pid == -1)
+    set_thread (0, 0);		/* run any thread */
+  else
+    set_thread (pid, 0);	/* run this thread */
+
   /* The s/S/c/C packets do not return status.  So if the target does
      not support the S or C packets, the debug agent returns an empty
      string which is detected in remote_wait().  This protocol defect
@@ -2651,7 +2667,7 @@ remote_async_resume (ptid_t ptid, int st
 	      getpkt (buf, (rs->remote_packet_size), 0);
 
 	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
-		goto register_event_loop;
+		return;
 	    }
 	}
       else
@@ -2669,7 +2685,7 @@ remote_async_resume (ptid_t ptid, int st
 	      getpkt (buf, (rs->remote_packet_size), 0);
 
 	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
-		goto register_event_loop;
+		return;
 	    }
 	}
     }
@@ -2678,15 +2694,21 @@ remote_async_resume (ptid_t ptid, int st
     {
       buf[0] = step ? 'S' : 'C';
       buf[1] = tohex (((int) siggnal >> 4) & 0xf);
-      buf[2] = tohex ((int) siggnal & 0xf);
+      buf[2] = tohex (((int) siggnal) & 0xf);
       buf[3] = '\0';
     }
   else
     strcpy (buf, step ? "s" : "c");
-  
+
   putpkt (buf);
+}
+
+/* Same as remote_resume, but with async support. */
+static void
+remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal)
+{
+  remote_resume (ptid, step, siggnal);
 
-register_event_loop:
   /* We are about to start executing the inferior, let's register it
      with the event loop. NOTE: this is the one place where all the
      execution commands end up. We could alternatively do this in each
@@ -6019,6 +6041,7 @@ show_remote_cmd (char *args, int from_tt
   show_remote_protocol_E_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_P_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL);
+  show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
 }
 
@@ -6191,6 +6214,13 @@ in a memory packet.\n",
 
   add_info ("remote-process", remote_info_process,
 	    "Query the remote system for process info.");
+
+  add_packet_config_cmd (&remote_protocol_vcont,
+			 "vCont", "verbose-resume",
+			 set_remote_protocol_vcont_packet_cmd,
+			 show_remote_protocol_vcont_packet_cmd,
+			 &remote_set_cmdlist, &remote_show_cmdlist,
+			 0);
 
   add_packet_config_cmd (&remote_protocol_qSymbol,
 			 "qSymbol", "symbol-lookup",

Re: RFA/RFC: vCont for the remote protocol [client]

[Andrew Cagney]

[I think my GNU e-mail is down] [I'm also having trouble reading this 
unified diff, so watch out :-)]

+/* Check for the availability of vCont.  This function should also check
+   the response.  */

see below ...

>  static void
> -remote_resume (ptid_t ptid, int step, enum target_signal siggnal)
> +remote_vcont_probe (struct remote_state *rs, char *buf)
>  {
> -  struct remote_state *rs = get_remote_state ();
> -  char *buf = alloca (rs->remote_packet_size);
> -  int pid = PIDGET (ptid);
> -  char *p;
> -
> -  if (pid == -1)
> -    set_thread (0, 0);		/* run any thread */
> -  else
> -    set_thread (pid, 0);	/* run this thread */
> -
> -  last_sent_signal = siggnal;
> -  last_sent_step = step;
> +  strcpy (buf, "vCont?");
> +  putpkt (buf);
> +  getpkt (buf, rs->remote_packet_size, 0);
> +  packet_ok (buf, &remote_protocol_vcont);
> +}

packet_ok will go through the path:

       /* The packet may or may not be OK.  Just assume it is */
       return PACKET_OK;

Shouldn't remote_vcont_probe apply additional sanity checks.  For 
instance that the response is well formed, and that all the letters 
[SCsc] appeared?  If they are not all present, just mark the packet is 
not supported for now.  (But also comment this).

+static int
+remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal)

What's the return value?

+{
+  struct remote_state *rs = get_remote_state ();
+  int pid = PIDGET (ptid);
+  char *buf = alloca (rs->remote_packet_size);

Rather than ALLOCA here, can you ...

+  /* If we could generate a wider range of packets, we'd have to worry
+     about overflowing BUF.  Should there be a generic
+     "multi-part-packet" packet?  */
+
+  if (PIDGET (inferior_ptid) == MAGIC_NULL_PID)
+    {
+      /* MAGIC_NULL_PTID means that we don't have any active threads, so we
+	 don't have any PID numbers the inferior will understand.  Make sure
+	 to only send forms that do not specify a PID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x", siggnal);

... use XASPRINTF here (along with some sort of cleanup).

Otherwize ok.

Andrew

Re: RFA/RFC: vCont for the remote protocol [client]

[Daniel Jacobowitz]

On Tue, Oct 14, 2003 at 08:14:52PM -0400, Andrew Cagney wrote:
> [I think my GNU e-mail is down] [I'm also having trouble reading this 
> unified diff, so watch out :-)]
> 
> +/* Check for the availability of vCont.  This function should also check
> +   the response.  */
> 
> see below ...
> 
> > static void
> >-remote_resume (ptid_t ptid, int step, enum target_signal siggnal)
> >+remote_vcont_probe (struct remote_state *rs, char *buf)
> > {
> >-  struct remote_state *rs = get_remote_state ();
> >-  char *buf = alloca (rs->remote_packet_size);
> >-  int pid = PIDGET (ptid);
> >-  char *p;
> >-
> >-  if (pid == -1)
> >-    set_thread (0, 0);		/* run any thread */
> >-  else
> >-    set_thread (pid, 0);	/* run this thread */
> >-
> >-  last_sent_signal = siggnal;
> >-  last_sent_step = step;
> >+  strcpy (buf, "vCont?");
> >+  putpkt (buf);
> >+  getpkt (buf, rs->remote_packet_size, 0);
> >+  packet_ok (buf, &remote_protocol_vcont);
> >+}
> 
> packet_ok will go through the path:
> 
>       /* The packet may or may not be OK.  Just assume it is */
>       return PACKET_OK;
> 
> Shouldn't remote_vcont_probe apply additional sanity checks.  For 
> instance that the response is well formed, and that all the letters 
> [SCsc] appeared?  If they are not all present, just mark the packet is 
> not supported for now.  (But also comment this).

The "should" there was supposed to imply "but we don't".  But hey, it
was easy, so fixed.

> +static int
> +remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal)
> 
> What's the return value?

From the comment a few lines up.

   This function returns
   non-zero iff it resumes the inferior.

> +{
> +  struct remote_state *rs = get_remote_state ();
> +  int pid = PIDGET (ptid);
> +  char *buf = alloca (rs->remote_packet_size);
> 
> Rather than ALLOCA here, can you ...
> 
> +  /* If we could generate a wider range of packets, we'd have to worry
> +     about overflowing BUF.  Should there be a generic
> +     "multi-part-packet" packet?  */
> +
> +  if (PIDGET (inferior_ptid) == MAGIC_NULL_PID)
> +    {
> +      /* MAGIC_NULL_PTID means that we don't have any active threads, so we
> +	 don't have any PID numbers the inferior will understand.  Make sure
> +	 to only send forms that do not specify a PID.  */
> +      if (step && siggnal != TARGET_SIGNAL_0)
> +	sprintf (buf, "vCont;S%02x", siggnal);
> 
> ... use XASPRINTF here (along with some sort of cleanup).

Is GDB trying to move away from alloca?  The internals manual says:

   GDB can use the non-portable function `alloca' for the allocation of
   small temporary values (such as strings).

So I use it to avoid cleanups.  OTOH, it occurs to me that
rs->remote_packet_size is a bit large; OTOOH, remote.c uses this idiom
all over the place already.

I've used xmalloc instead, since the buf is used for getpkt and thus
must be remote_packet_size large.

Here's what I am about to check in.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-10-16  Daniel Jacobowitz  <drow@mvista.com>

	* remote.c (remote_protocol_vcont): New variable.
	(set_remote_protocol_vcont_packet_cmd): New function.
	(show_remote_protocol_vcont_packet_cmd): New function.
	(init_all_packet_configs): Handle remote_protocol_vcont.
	(remote_vcont_probe): New function.
	(remote_vcont_resume): New function.
	(remote_resume): Use it.
	(remote_async_resume): Call remote_resume.
	(_initialize_remote): Add verbose-resume packet commands.

Index: gdb/remote.c
===================================================================
RCS file: /big/fsf/rsync/src-cvs/src/gdb/remote.c,v
retrieving revision 1.117
diff -u -p -r1.117 remote.c
--- gdb/remote.c	15 Oct 2003 21:10:55 -0000	1.117
+++ gdb/remote.c	16 Oct 2003 20:09:26 -0000
@@ -749,6 +749,23 @@ packet_ok (const char *buf, struct packe
     }
 }
 
+/* Should we try the 'vCont' (descriptive resume) request? */
+static struct packet_config remote_protocol_vcont;
+
+static void
+set_remote_protocol_vcont_packet_cmd (char *args, int from_tty,
+				      struct cmd_list_element *c)
+{
+  update_packet_config (&remote_protocol_vcont);
+}
+
+static void
+show_remote_protocol_vcont_packet_cmd (char *args, int from_tty,
+				       struct cmd_list_element *c)
+{
+  show_packet_config_cmd (&remote_protocol_vcont);
+}
+
 /* Should we try the 'qSymbol' (target symbol lookup service) request? */
 static struct packet_config remote_protocol_qSymbol;
 
@@ -2190,6 +2207,7 @@ init_all_packet_configs (void)
   update_packet_config (&remote_protocol_E);
   update_packet_config (&remote_protocol_P);
   update_packet_config (&remote_protocol_qSymbol);
+  update_packet_config (&remote_protocol_vcont);
   for (i = 0; i < NR_Z_PACKET_TYPES; i++)
     update_packet_config (&remote_protocol_Z[i]);
   /* Force remote_write_bytes to check whether target supports binary
@@ -2503,115 +2521,144 @@ bin2hex (const char *bin, char *hex, int
   return i;
 }
 \f
-/* Tell the remote machine to resume.  */
-
-static enum target_signal last_sent_signal = TARGET_SIGNAL_0;
-
-static int last_sent_step;
+/* Check for the availability of vCont.  This function should also check
+   the response.  */
 
 static void
-remote_resume (ptid_t ptid, int step, enum target_signal siggnal)
+remote_vcont_probe (struct remote_state *rs, char *buf)
 {
-  struct remote_state *rs = get_remote_state ();
-  char *buf = alloca (rs->remote_packet_size);
-  int pid = PIDGET (ptid);
-  char *p;
+  strcpy (buf, "vCont?");
+  putpkt (buf);
+  getpkt (buf, rs->remote_packet_size, 0);
 
-  if (pid == -1)
-    set_thread (0, 0);		/* run any thread */
-  else
-    set_thread (pid, 0);	/* run this thread */
+  /* Make sure that the features we assume are supported.  */
+  if (strncmp (buf, "vCont", 5) == 0)
+    {
+      char *p = &buf[5];
+      int support_s, support_S, support_c, support_C;
 
-  last_sent_signal = siggnal;
-  last_sent_step = step;
+      support_s = 0;
+      support_S = 0;
+      support_c = 0;
+      support_C = 0;
+      while (p && *p == ';')
+	{
+	  p++;
+	  if (*p == 's' && (*(p + 1) == ';' || *(p + 1) == 0))
+	    support_s = 1;
+	  else if (*p == 'S' && (*(p + 1) == ';' || *(p + 1) == 0))
+	    support_S = 1;
+	  else if (*p == 'c' && (*(p + 1) == ';' || *(p + 1) == 0))
+	    support_c = 1;
+	  else if (*p == 'C' && (*(p + 1) == ';' || *(p + 1) == 0))
+	    support_C = 1;
 
-  /* A hook for when we need to do something at the last moment before
-     resumption.  */
-  if (target_resume_hook)
-    (*target_resume_hook) ();
+	  p = strchr (p, ';');
+	}
 
+      /* If s, S, c, and C are not all supported, we can't use vCont.  Clearing
+         BUF will make packet_ok disable the packet.  */
+      if (!support_s || !support_S || !support_c || !support_C)
+	buf[0] = 0;
+    }
 
-  /* The s/S/c/C packets do not return status.  So if the target does
-     not support the S or C packets, the debug agent returns an empty
-     string which is detected in remote_wait().  This protocol defect
-     is fixed in the e/E packets. */
+  packet_ok (buf, &remote_protocol_vcont);
+}
 
-  if (step && step_range_end)
-    {
-      /* If the target does not support the 'E' packet, we try the 'S'
-	 packet.  Ideally we would fall back to the 'e' packet if that
-	 too is not supported.  But that would require another copy of
-	 the code to issue the 'e' packet (and fall back to 's' if not
-	 supported) in remote_wait().  */
-      
-      if (siggnal != TARGET_SIGNAL_0)
-	{
-	  if (remote_protocol_E.support != PACKET_DISABLE)
-	    {
-	      p = buf;
-	      *p++ = 'E';
-	      *p++ = tohex (((int) siggnal >> 4) & 0xf);
-	      *p++ = tohex (((int) siggnal) & 0xf);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_start);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_end);
-	      *p++ = 0;
+/* Resume the remote inferior by using a "vCont" packet.  The thread
+   to be resumed is PTID; STEP and SIGGNAL indicate whether the
+   resumed thread should be single-stepped and/or signalled.  If PTID's
+   PID is -1, then all threads are resumed; the thread to be stepped and/or
+   signalled is given in the global INFERIOR_PTID.  This function returns
+   non-zero iff it resumes the inferior.
 
-	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+   This function issues a strict subset of all possible vCont commands at the
+   moment.  */
 
-	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
-		return;
-	    }
-	}
-      else
-	{
-	  if (remote_protocol_e.support != PACKET_DISABLE)
-	    {
-	      p = buf;
-	      *p++ = 'e';
-	      p += hexnumstr (p, (ULONGEST) step_range_start);
-	      *p++ = ',';
-	      p += hexnumstr (p, (ULONGEST) step_range_end);
-	      *p++ = 0;
+static int
+remote_vcont_resume (ptid_t ptid, int step, enum target_signal siggnal)
+{
+  struct remote_state *rs = get_remote_state ();
+  int pid = PIDGET (ptid);
+  char *buf = NULL;
+  struct cleanup *old_cleanup;
 
-	      putpkt (buf);
-	      getpkt (buf, (rs->remote_packet_size), 0);
+  buf = xmalloc (rs->remote_packet_size);
+  old_cleanup = make_cleanup (xfree, buf);
 
-	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
-		return;
-	    }
-	}
+  if (remote_protocol_vcont.support == PACKET_SUPPORT_UNKNOWN)
+    remote_vcont_probe (rs, buf);
+
+  if (remote_protocol_vcont.support == PACKET_DISABLE)
+    {
+      do_cleanups (old_cleanup);
+      return 0;
     }
 
-  if (siggnal != TARGET_SIGNAL_0)
+  /* If we could generate a wider range of packets, we'd have to worry
+     about overflowing BUF.  Should there be a generic
+     "multi-part-packet" packet?  */
+
+  if (PIDGET (inferior_ptid) == MAGIC_NULL_PID)
+    {
+      /* MAGIC_NULL_PTID means that we don't have any active threads, so we
+	 don't have any PID numbers the inferior will understand.  Make sure
+	 to only send forms that do not specify a PID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x", siggnal);
+      else if (step)
+	sprintf (buf, "vCont;s");
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;C%02x", siggnal);
+      else
+	sprintf (buf, "vCont;c");
+    }
+  else if (pid == -1)
     {
-      buf[0] = step ? 'S' : 'C';
-      buf[1] = tohex (((int) siggnal >> 4) & 0xf);
-      buf[2] = tohex (((int) siggnal) & 0xf);
-      buf[3] = '\0';
+      /* Resume all threads, with preference for INFERIOR_PTID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+      else if (step)
+	sprintf (buf, "vCont;s:%x;c", PIDGET (inferior_ptid));
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;C%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+      else
+	sprintf (buf, "vCont;c");
     }
   else
-    strcpy (buf, step ? "s" : "c");
+    {
+      /* Scheduler locking; resume only PTID.  */
+      if (step && siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;S%02x:%x", siggnal, pid);
+      else if (step)
+	sprintf (buf, "vCont;s:%x", pid);
+      else if (siggnal != TARGET_SIGNAL_0)
+	sprintf (buf, "vCont;C%02x:%x", siggnal, pid);
+      else
+	sprintf (buf, "vCont;c:%x", pid);
+    }
 
   putpkt (buf);
+
+  do_cleanups (old_cleanup);
+
+  return 1;
 }
 
-/* Same as remote_resume, but with async support. */
+/* Tell the remote machine to resume.  */
+
+static enum target_signal last_sent_signal = TARGET_SIGNAL_0;
+
+static int last_sent_step;
+
 static void
-remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal)
+remote_resume (ptid_t ptid, int step, enum target_signal siggnal)
 {
   struct remote_state *rs = get_remote_state ();
   char *buf = alloca (rs->remote_packet_size);
   int pid = PIDGET (ptid);
   char *p;
 
-  if (pid == -1)
-    set_thread (0, 0);		/* run any thread */
-  else
-    set_thread (pid, 0);	/* run this thread */
-
   last_sent_signal = siggnal;
   last_sent_step = step;
 
@@ -2620,6 +2667,16 @@ remote_async_resume (ptid_t ptid, int st
   if (target_resume_hook)
     (*target_resume_hook) ();
 
+  /* The vCont packet doesn't need to specify threads via Hc.  */
+  if (remote_vcont_resume (ptid, step, siggnal))
+    return;
+
+  /* All other supported resume packets do use Hc, so call set_thread.  */
+  if (pid == -1)
+    set_thread (0, 0);		/* run any thread */
+  else
+    set_thread (pid, 0);	/* run this thread */
+
   /* The s/S/c/C packets do not return status.  So if the target does
      not support the S or C packets, the debug agent returns an empty
      string which is detected in remote_wait().  This protocol defect
@@ -2651,7 +2708,7 @@ remote_async_resume (ptid_t ptid, int st
 	      getpkt (buf, (rs->remote_packet_size), 0);
 
 	      if (packet_ok (buf, &remote_protocol_E) == PACKET_OK)
-		goto register_event_loop;
+		return;
 	    }
 	}
       else
@@ -2669,7 +2726,7 @@ remote_async_resume (ptid_t ptid, int st
 	      getpkt (buf, (rs->remote_packet_size), 0);
 
 	      if (packet_ok (buf, &remote_protocol_e) == PACKET_OK)
-		goto register_event_loop;
+		return;
 	    }
 	}
     }
@@ -2678,15 +2735,21 @@ remote_async_resume (ptid_t ptid, int st
     {
       buf[0] = step ? 'S' : 'C';
       buf[1] = tohex (((int) siggnal >> 4) & 0xf);
-      buf[2] = tohex ((int) siggnal & 0xf);
+      buf[2] = tohex (((int) siggnal) & 0xf);
       buf[3] = '\0';
     }
   else
     strcpy (buf, step ? "s" : "c");
-  
+
   putpkt (buf);
+}
+
+/* Same as remote_resume, but with async support. */
+static void
+remote_async_resume (ptid_t ptid, int step, enum target_signal siggnal)
+{
+  remote_resume (ptid, step, siggnal);
 
-register_event_loop:
   /* We are about to start executing the inferior, let's register it
      with the event loop. NOTE: this is the one place where all the
      execution commands end up. We could alternatively do this in each
@@ -5952,6 +6015,7 @@ show_remote_cmd (char *args, int from_tt
   show_remote_protocol_E_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_P_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_qSymbol_packet_cmd (args, from_tty, NULL);
+  show_remote_protocol_vcont_packet_cmd (args, from_tty, NULL);
   show_remote_protocol_binary_download_cmd (args, from_tty, NULL);
 }
 
@@ -6124,6 +6188,13 @@ in a memory packet.\n",
 
   add_info ("remote-process", remote_info_process,
 	    "Query the remote system for process info.");
+
+  add_packet_config_cmd (&remote_protocol_vcont,
+			 "vCont", "verbose-resume",
+			 set_remote_protocol_vcont_packet_cmd,
+			 show_remote_protocol_vcont_packet_cmd,
+			 &remote_set_cmdlist, &remote_show_cmdlist,
+			 0);
 
   add_packet_config_cmd (&remote_protocol_qSymbol,
 			 "qSymbol", "symbol-lookup",

Re: RFA/RFC: vCont for the remote protocol [client]

[Andrew Cagney]

> Is GDB trying to move away from alloca?  The internals manual says:
> 
>    GDB can use the non-portable function `alloca' for the allocation of
>    small temporary values (such as strings).

> So I use it to avoid cleanups.  OTOH, it occurs to me that
> rs->remote_packet_size is a bit large; OTOOH, remote.c uses this idiom
> all over the place already.
> 
> I've used xmalloc instead, since the buf is used for getpkt and thus
> must be remote_packet_size large.
> 
> Here's what I am about to check in.

There are two probems:

- the buffer can get very very large and that can blow the stack
- it isn't possible to audit this code (with out a deep understanding of 
that value) and hence demonstrate that the sprintf won't smash the 
stack/heap

You'll need to also change the sprintf to snprintf (parameterized with 
remote_packet_size.

Andrew

Re: RFA/RFC: vCont for the remote protocol [client]

[Daniel Jacobowitz]

On Thu, Oct 16, 2003 at 05:18:35PM -0400, Andrew Cagney wrote:
> 
> >Is GDB trying to move away from alloca?  The internals manual says:
> >
> >   GDB can use the non-portable function `alloca' for the allocation of
> >   small temporary values (such as strings).
> 
> >So I use it to avoid cleanups.  OTOH, it occurs to me that
> >rs->remote_packet_size is a bit large; OTOOH, remote.c uses this idiom
> >all over the place already.
> >
> >I've used xmalloc instead, since the buf is used for getpkt and thus
> >must be remote_packet_size large.
> >
> >Here's what I am about to check in.
> 
> There are two probems:
> 
> - the buffer can get very very large and that can blow the stack
> - it isn't possible to audit this code (with out a deep understanding of 
> that value) and hence demonstrate that the sprintf won't smash the 
> stack/heap
> 
> You'll need to also change the sprintf to snprintf (parameterized with 
> remote_packet_size.

I don't see a point in doing that until someone expresses interest in
thread locking or some other feature which requires adding to the code. 
The maximum length of any generated vcont packet is the length of:
   vCont;C01:12341468;C02
The minimum possible buffer size is about twenty times that.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: RFA/RFC: vCont for the remote protocol [client]

[Andrew Cagney]

> There are two probems:
>> 
>> - the buffer can get very very large and that can blow the stack
>> - it isn't possible to audit this code (with out a deep understanding of 
>> that value) and hence demonstrate that the sprintf won't smash the 
>> stack/heap
>> 
>> You'll need to also change the sprintf to snprintf (parameterized with 
>> remote_packet_size.
> 
> 
> I don't see a point in doing that until someone expresses interest in
> thread locking or some other feature which requires adding to the code. 
> The maximum length of any generated vcont packet is the length of:
>    vCont;C01:12341468;C02
> The minimum possible buffer size is about twenty times that.

I wrote "it isn't possible to audit this code (with out a deep 
understanding of that [remote_packet_size] value)".  The code should be 
locally robust.

Andrew

Re: RFA/RFC: vCont for the remote protocol [client]

[Daniel Jacobowitz]

On Thu, Oct 16, 2003 at 06:27:07PM -0400, Andrew Cagney wrote:
> >There are two probems:
> >>
> >>- the buffer can get very very large and that can blow the stack
> >>- it isn't possible to audit this code (with out a deep understanding of 
> >>that value) and hence demonstrate that the sprintf won't smash the 
> >>stack/heap
> >>
> >>You'll need to also change the sprintf to snprintf (parameterized with 
> >>remote_packet_size.
> >
> >
> >I don't see a point in doing that until someone expresses interest in
> >thread locking or some other feature which requires adding to the code. 
> >The maximum length of any generated vcont packet is the length of:
> >   vCont;C01:12341468;C02
> >The minimum possible buffer size is about twenty times that.
> 
> I wrote "it isn't possible to audit this code (with out a deep 
> understanding of that [remote_packet_size] value)".  The code should be 
> locally robust.

I wouldn't call the minimum size a deep understanding.  It isn't
documented anywhere in the code but I think that it should be.

But I'll fix it next week.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

[RFA] Remove some sprintfs from vCont client support

[Daniel Jacobowitz]

On Thu, Oct 16, 2003 at 06:53:28PM -0400, Daniel Jacobowitz wrote:
> On Thu, Oct 16, 2003 at 06:27:07PM -0400, Andrew Cagney wrote:
> > >There are two probems:
> > >>
> > >>- the buffer can get very very large and that can blow the stack

If that concerns you then I suggest the alloca's in putpkt_binary,
which I found while fixing this.  That's a whole lot more worrisome,
since there's one as big as the packet plus one as big as the max
packet length.

> > >>- it isn't possible to audit this code (with out a deep understanding of 
> > >>that value) and hence demonstrate that the sprintf won't smash the 
> > >>stack/heap
> > >>
> > >>You'll need to also change the sprintf to snprintf (parameterized with 
> > >>remote_packet_size.
> > >
> > >
> > >I don't see a point in doing that until someone expresses interest in
> > >thread locking or some other feature which requires adding to the code. 
> > >The maximum length of any generated vcont packet is the length of:
> > >   vCont;C01:12341468;C02
> > >The minimum possible buffer size is about twenty times that.
> > 
> > I wrote "it isn't possible to audit this code (with out a deep 
> > understanding of that [remote_packet_size] value)".  The code should be 
> > locally robust.
> 
> I wouldn't call the minimum size a deep understanding.  It isn't
> documented anywhere in the code but I think that it should be.
> 
> But I'll fix it next week.

OK, so it wasn't the next week.  I took advantage of the new xstrprintf
function to get something I'm happy with.  How about you, Andrew - is
this patch OK?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-12-02  Daniel Jacobowitz  <drow@mvista.com>

	* remote.c (remote_vcont_resume): Use xstrprintf instead of sprintf.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.122
diff -u -p -r1.122 remote.c
--- remote.c	10 Nov 2003 21:20:44 -0000	1.122
+++ remote.c	2 Dec 2003 15:57:33 -0000
@@ -2578,7 +2578,7 @@ remote_vcont_resume (ptid_t ptid, int st
 {
   struct remote_state *rs = get_remote_state ();
   int pid = PIDGET (ptid);
-  char *buf = NULL;
+  char *buf = NULL, *outbuf;
   struct cleanup *old_cleanup;
 
   buf = xmalloc (rs->remote_packet_size);
@@ -2603,40 +2603,45 @@ remote_vcont_resume (ptid_t ptid, int st
 	 don't have any PID numbers the inferior will understand.  Make sure
 	 to only send forms that do not specify a PID.  */
       if (step && siggnal != TARGET_SIGNAL_0)
-	sprintf (buf, "vCont;S%02x", siggnal);
+	outbuf = xstrprintf ("vCont;S%02x", siggnal);
       else if (step)
-	sprintf (buf, "vCont;s");
+	outbuf = xstrprintf ("vCont;s");
       else if (siggnal != TARGET_SIGNAL_0)
-	sprintf (buf, "vCont;C%02x", siggnal);
+	outbuf = xstrprintf ("vCont;C%02x", siggnal);
       else
-	sprintf (buf, "vCont;c");
+	outbuf = xstrprintf ("vCont;c");
     }
   else if (pid == -1)
     {
       /* Resume all threads, with preference for INFERIOR_PTID.  */
       if (step && siggnal != TARGET_SIGNAL_0)
-	sprintf (buf, "vCont;S%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+	outbuf = xstrprintf ("vCont;S%02x:%x;c", siggnal,
+			     PIDGET (inferior_ptid));
       else if (step)
-	sprintf (buf, "vCont;s:%x;c", PIDGET (inferior_ptid));
+	outbuf = xstrprintf ("vCont;s:%x;c", PIDGET (inferior_ptid));
       else if (siggnal != TARGET_SIGNAL_0)
-	sprintf (buf, "vCont;C%02x:%x;c", siggnal, PIDGET (inferior_ptid));
+	outbuf = xstrprintf ("vCont;C%02x:%x;c", siggnal,
+			     PIDGET (inferior_ptid));
       else
-	sprintf (buf, "vCont;c");
+	outbuf = xstrprintf ("vCont;c");
     }
   else
     {
       /* Scheduler locking; resume only PTID.  */
       if (step && siggnal != TARGET_SIGNAL_0)
-	sprintf (buf, "vCont;S%02x:%x", siggnal, pid);
+	outbuf = xstrprintf ("vCont;S%02x:%x", siggnal, pid);
       else if (step)
-	sprintf (buf, "vCont;s:%x", pid);
+	outbuf = xstrprintf ("vCont;s:%x", pid);
       else if (siggnal != TARGET_SIGNAL_0)
-	sprintf (buf, "vCont;C%02x:%x", siggnal, pid);
+	outbuf = xstrprintf ("vCont;C%02x:%x", siggnal, pid);
       else
-	sprintf (buf, "vCont;c:%x", pid);
+	outbuf = xstrprintf ("vCont;c:%x", pid);
     }
 
-  putpkt (buf);
+  gdb_assert (outbuf && strlen (outbuf) < rs->remote_packet_size);
+  make_cleanup (xfree, outbuf);
+
+  putpkt (outbuf);
 
   do_cleanups (old_cleanup);
 

Re: [RFA] Remove some sprintfs from vCont client support

[Andrew Cagney]

yes, thanks

Re: [RFA] Remove some sprintfs from vCont client support

[Daniel Jacobowitz]

On Tue, Dec 02, 2003 at 11:07:04AM -0500, Daniel Jacobowitz wrote:
> 2003-12-02  Daniel Jacobowitz  <drow@mvista.com>
> 
> 	* remote.c (remote_vcont_resume): Use xstrprintf instead of sprintf.

Oops, forgot to commit this after Andrew approved it.  Done now, and
the below.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2004-01-17  Daniel Jacobowitz  <drow@mvista.com>

	* remote.c: Update copyright years.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.125
diff -u -p -r1.125 remote.c
--- remote.c	18 Jan 2004 03:37:03 -0000	1.125
+++ remote.c	18 Jan 2004 03:37:58 -0000
@@ -1,7 +1,8 @@
 /* Remote target communications for serial-line targets in custom GDB protocol
 
    Copyright 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995, 1996,
-   1997, 1998, 1999, 2000, 2001, 2002 Free Software Foundation, Inc.
+   1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004
+   Free Software Foundation, Inc.
 
    This file is part of GDB.