[RFA] Restore leading zeros in remote_thread_alive

[RFA] Restore leading zeros in remote_thread_alive

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

Hi Pedro,

VMware has a remote target for which "info threads" stopped
working after you added your remote multi-process patch in
September.  I've finally got around to figuring out why.

The docs don't actually say whether this message should have
leading zeros, but it always used to (you can check out the
old sprintf spec).

If there's no compelling reason for removing them,
do you mind if we put them back?   ;-)


[-- Attachment #2: remote_thread_alive.txt --]
[-- Type: text/plain, Size: 1356 bytes --]

2008-10-21  Michael Snyder  <msnyder@vmware.com>

	* remote.c (write_ptid): Emit leading zeros to preserve 
	remote protocol behavior of remote_thread_alive.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.321
diff -u -p -r1.321 remote.c
--- remote.c	17 Oct 2008 19:43:47 -0000	1.321
+++ remote.c	22 Oct 2008 01:00:16 -0000
@@ -1433,15 +1433,15 @@ write_ptid (char *buf, const char *endbu
     {
       pid = ptid_get_pid (ptid);
       if (pid < 0)
-	buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
+	buf += xsnprintf (buf, endbuf - buf, "p-%08x.", -pid);
       else
-	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
+	buf += xsnprintf (buf, endbuf - buf, "p%08x.", pid);
     }
   tid = ptid_get_tid (ptid);
   if (tid < 0)
-    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
+    buf += xsnprintf (buf, endbuf - buf, "-%x08", -tid);
   else
-    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
+    buf += xsnprintf (buf, endbuf - buf, "%x08", tid);
 
   return buf;
 }
@@ -2097,6 +2097,10 @@ remote_threads_info (void)
 	    {
 	      do
 		{
+		  /* Skip spaces.  */
+		  while (*bufp == ' ')
+		    bufp++;
+
 		  new_thread = read_ptid (bufp, &bufp);
 		  if (!ptid_equal (new_thread, null_ptid)
 		      && (!in_thread_list (new_thread)

Re: [RFA] Restore leading zeros in remote_thread_alive

[Michael Snyder]

Damn!  Please ignore the "skip spaces" diff.
Intended to be submitted as a separate patch tomorrow...

Michael Snyder wrote:
> Hi Pedro,
> 
> VMware has a remote target for which "info threads" stopped
> working after you added your remote multi-process patch in
> September.  I've finally got around to figuring out why.
> 
> The docs don't actually say whether this message should have
> leading zeros, but it always used to (you can check out the
> old sprintf spec).
> 
> If there's no compelling reason for removing them,
> do you mind if we put them back?   ;-)
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 2008-10-21  Michael Snyder  <msnyder@vmware.com>
> 
> 	* remote.c (write_ptid): Emit leading zeros to preserve 
> 	remote protocol behavior of remote_thread_alive.
> 
> Index: remote.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/remote.c,v
> retrieving revision 1.321
> diff -u -p -r1.321 remote.c
> --- remote.c	17 Oct 2008 19:43:47 -0000	1.321
> +++ remote.c	22 Oct 2008 01:00:16 -0000
> @@ -1433,15 +1433,15 @@ write_ptid (char *buf, const char *endbu
>      {
>        pid = ptid_get_pid (ptid);
>        if (pid < 0)
> -	buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
> +	buf += xsnprintf (buf, endbuf - buf, "p-%08x.", -pid);
>        else
> -	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
> +	buf += xsnprintf (buf, endbuf - buf, "p%08x.", pid);
>      }
>    tid = ptid_get_tid (ptid);
>    if (tid < 0)
> -    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
> +    buf += xsnprintf (buf, endbuf - buf, "-%x08", -tid);
>    else
> -    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
> +    buf += xsnprintf (buf, endbuf - buf, "%x08", tid);
>  
>    return buf;
>  }
> @@ -2097,6 +2097,10 @@ remote_threads_info (void)
>  	    {
>  	      do
>  		{
> +		  /* Skip spaces.  */
> +		  while (*bufp == ' ')
> +		    bufp++;
> +
>  		  new_thread = read_ptid (bufp, &bufp);
>  		  if (!ptid_equal (new_thread, null_ptid)
>  		      && (!in_thread_list (new_thread)

Re: [RFA] Restore leading zeros in remote_thread_alive

[Pedro Alves]

On Wednesday 22 October 2008 02:02:19, Michael Snyder wrote:
> Hi Pedro,
> 
> VMware has a remote target for which "info threads" stopped
> working after you added your remote multi-process patch in
> September.  I've finally got around to figuring out why.
> 
> The docs don't actually say whether this message should have
> leading zeros, but it always used to (you can check out the
> old sprintf spec).
> 
> If there's no compelling reason for removing them,
> do you mind if we put them back?   ;-)

Urglh, bad stub!  I don't mind.  Sorry, and thanks.

-- 
Pedro Alves

Re: [RFA] Restore leading zeros in remote_thread_alive

[Daniel Jacobowitz]

On Tue, Oct 21, 2008 at 06:02:19PM -0700, Michael Snyder wrote:
>    if (tid < 0)
> -    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
> +    buf += xsnprintf (buf, endbuf - buf, "-%x08", -tid);
>    else
> -    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
> +    buf += xsnprintf (buf, endbuf - buf, "%x08", tid);

%x08?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Restore leading zeros in remote_thread_alive

[Michael Snyder]

Daniel Jacobowitz wrote:
> On Tue, Oct 21, 2008 at 06:02:19PM -0700, Michael Snyder wrote:
>>    if (tid < 0)
>> -    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
>> +    buf += xsnprintf (buf, endbuf - buf, "-%x08", -tid);
>>    else
>> -    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
>> +    buf += xsnprintf (buf, endbuf - buf, "%x08", tid);
> 
> %x08?

D'urr.  %08x...

Re: [RFA] Restore leading zeros in remote_thread_alive

[Pedro Alves]

On Wednesday 22 October 2008 02:02:19, Michael Snyder wrote:
> --- remote.c    17 Oct 2008 19:43:47 -0000      1.321
> +++ remote.c    22 Oct 2008 01:00:16 -0000
> @@ -1433,15 +1433,15 @@ write_ptid (char *buf, const char *endbu
>      {
>        pid = ptid_get_pid (ptid);
>        if (pid < 0)
> -       buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
> +       buf += xsnprintf (buf, endbuf - buf, "p-%08x.", -pid);
>        else
> -       buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
> +       buf += xsnprintf (buf, endbuf - buf, "p%08x.", pid);
>      }
>    tid = ptid_get_tid (ptid);
>    if (tid < 0)
> -    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
> +    buf += xsnprintf (buf, endbuf - buf, "-%x08", -tid);
>    else
> -    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
> +    buf += xsnprintf (buf, endbuf - buf, "%x08", tid);
>  
>    return buf;

( Now that I've slept a bit, :-) ) I've gone through an old version of the code
and documentation looking for places we use write_ptid now that used to
output %08x vs places we used to output "-1" or %x.  Indeed, I can only see
that in remote_thread_alive.  The change above can possibly lead to other
stubs out there (with similar assumptions to yours) not parsing e.g., Hg-000000001
correctly, because they were expecting Hg-1, or e.g., misparsing vCont, because
used to be "vCont;c:%x".  Also, we don't really need to use %08 when multi-process
is in effect.  There should be no multi-process stubs around that depend
on leading 0's.

I guess this means we get to add a new parameter to write_ptid
(leading_zeros ?), and pass 1 where needed...  Give me a sec to cook
something up.

-- 
Pedro Alves

Re: [RFA] Restore leading zeros in remote_thread_alive

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 2110 bytes --]

A Wednesday 22 October 2008 14:18:22, Pedro Alves escreveu:
> On Wednesday 22 October 2008 02:02:19, Michael Snyder wrote:
> > --- remote.c    17 Oct 2008 19:43:47 -0000      1.321
> > +++ remote.c    22 Oct 2008 01:00:16 -0000
> > @@ -1433,15 +1433,15 @@ write_ptid (char *buf, const char *endbu
> >      {
> >        pid = ptid_get_pid (ptid);
> >        if (pid < 0)
> > -       buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
> > +       buf += xsnprintf (buf, endbuf - buf, "p-%08x.", -pid);
> >        else
> > -       buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
> > +       buf += xsnprintf (buf, endbuf - buf, "p%08x.", pid);
> >      }
> >    tid = ptid_get_tid (ptid);
> >    if (tid < 0)
> > -    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
> > +    buf += xsnprintf (buf, endbuf - buf, "-%x08", -tid);
> >    else
> > -    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
> > +    buf += xsnprintf (buf, endbuf - buf, "%x08", tid);
> >  
> >    return buf;
> 
> ( Now that I've slept a bit, :-) ) I've gone through an old version of the code
> and documentation looking for places we use write_ptid now that used to
> output %08x vs places we used to output "-1" or %x.  Indeed, I can only see
> that in remote_thread_alive.  The change above can possibly lead to other
> stubs out there (with similar assumptions to yours) not parsing e.g., Hg-000000001
> correctly, because they were expecting Hg-1, or e.g., misparsing vCont, because
> used to be "vCont;c:%x".  Also, we don't really need to use %08 when multi-process
> is in effect.  There should be no multi-process stubs around that depend
> on leading 0's.
> 
> I guess this means we get to add a new parameter to write_ptid
> (leading_zeros ?), and pass 1 where needed...  Give me a sec to cook
> something up.
> 

Could you try this out please?  It works here against gdbserver
single|multi-process, and sends a `T000000tid' (tid alive) packet when
multi-process isn't in effect.

-- 
Pedro Alves

[-- Attachment #2: write_ptid_arg.diff --]
[-- Type: text/x-diff, Size: 3489 bytes --]

---
 gdb/remote.c |   62 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 49 insertions(+), 13 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-10-22 14:25:12.000000000 +0100
+++ src/gdb/remote.c	2008-10-22 14:58:28.000000000 +0100
@@ -209,6 +209,8 @@ static void show_remote_protocol_packet_
 					     const char *value);
 
 static char *write_ptid (char *buf, const char *endbuf, ptid_t ptid);
+static char *write_ptid_leading_zeros (char *buf, const char *endbuf,
+				       ptid_t ptid);
 static ptid_t read_ptid (char *buf, char **obuf);
 
 static void remote_query_supported (void);
@@ -1296,7 +1298,10 @@ remote_thread_alive (ptid_t ptid)
   endp = rs->buf + get_remote_packet_size ();
 
   *p++ = 'T';
-  write_ptid (p, endp, ptid);
+
+  /* Make sure we output an 8-char wide thread id (if the
+     multi-process extensions aren't in effect).  */
+  write_ptid_leading_zeros (p, endp, ptid);
 
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);
@@ -1420,32 +1425,63 @@ static int remote_newthread_step (thread
 
 
 /* Write a PTID to BUF.  ENDBUF points to one-passed-the-end of the
-   buffer we're allowed to write to.  Returns
-   BUF+CHARACTERS_WRITTEN.  */
+   buffer we're allowed to write to.  If LEADING_ZEROS, and the
+   multi-process extensions are in effect, always output an 8-char
+   long tid, padding with leading 0's if required to do so.
+   Returns BUF+CHARACTERS_WRITTEN.  */
 
 static char *
-write_ptid (char *buf, const char *endbuf, ptid_t ptid)
+write_ptid_1 (char *buf, const char *endbuf,
+	      ptid_t ptid, int leading_zeros)
 {
   int pid, tid;
   struct remote_state *rs = get_remote_state ();
 
-  if (remote_multi_process_p (rs))
+  tid = ptid_get_tid (ptid);
+
+  /* GDB has always output leading zeros in the thread id in some
+     packets.  Some stubs misparse those packets if the leading zeros
+     are removed.  GDB never sent leading zeros when multi-process
+     extensions are in effect, so it's safe to not send them in that
+     case (since it's just extra traffic).  */
+  if (leading_zeros && !remote_multi_process_p (rs))
     {
-      pid = ptid_get_pid (ptid);
-      if (pid < 0)
-	buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
+      if (tid < 0)
+	buf += xsnprintf (buf, endbuf - buf, "-%08x", -tid);
       else
-	buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
+	buf += xsnprintf (buf, endbuf - buf, "%08x", tid);
     }
-  tid = ptid_get_tid (ptid);
-  if (tid < 0)
-    buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
   else
-    buf += xsnprintf (buf, endbuf - buf, "%x", tid);
+    {
+      if (remote_multi_process_p (rs))
+	{
+	  pid = ptid_get_pid (ptid);
+	  if (pid < 0)
+	    buf += xsnprintf (buf, endbuf - buf, "p-%x.", -pid);
+	  else
+	    buf += xsnprintf (buf, endbuf - buf, "p%x.", pid);
+	}
+      if (tid < 0)
+	buf += xsnprintf (buf, endbuf - buf, "-%x", -tid);
+      else
+	buf += xsnprintf (buf, endbuf - buf, "%x", tid);
+    }
 
   return buf;
 }
 
+static char *
+write_ptid (char *buf, const char *endbuf, ptid_t ptid)
+{
+  return write_ptid_1 (buf, endbuf, ptid, 0);
+}
+
+static char *
+write_ptid_leading_zeros (char *buf, const char *endbuf, ptid_t ptid)
+{
+  return write_ptid_1 (buf, endbuf, ptid, 1);
+}
+
 /* Extract a PTID from BUF.  If non-null, OBUF is set to the to one
    passed the last parsed char.  Returns null_ptid on error.  */
 

Re: [RFA] Restore leading zeros in remote_thread_alive

[Michael Snyder]

Pedro Alves wrote:

> Could you try this out please?  It works here against gdbserver
> single|multi-process, and sends a `T000000tid' (tid alive) packet when
> multi-process isn't in effect.

Thanks for working up the patch -- it works for my target.

However, I must say that I'm not crazy about the idea that
we decide whether or not to send leading zeroes based on
whether the target is multi-process.  Seems orthogonal
and ad hoc.

If your targets will choke on the leading zeros, then
you have just as strong a case as I do.   ;-)

However, if you're just doing it to save a few bytes of
data over the serial line, I question whether it's worth it.

Michael

Re: [RFA] Restore leading zeros in remote_thread_alive

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

On Wednesday 22 October 2008 19:05:09, Michael Snyder wrote:
> Pedro Alves wrote:
> 
> > Could you try this out please?  It works here against gdbserver
> > single|multi-process, and sends a `T000000tid' (tid alive) packet when
> > multi-process isn't in effect.
> 
> Thanks for working up the patch -- it works for my target.

Thanks for testing.

> 
> However, I must say that I'm not crazy about the idea that
> we decide whether or not to send leading zeroes based on
> whether the target is multi-process.  Seems orthogonal
> and ad hoc.

I can't think of a valid reason why we'd have this
inconsistency:

 --> Hg p1.2
 ...

 --> vCont;s:p1.2
 <-- T05 thread:p1.2

 -->  qfThreadInfo
 <--  m p1.2
 -->  qfThreadInfo
 <--  m p1.3
 ...

 (others)

But:

 --> T p00000001.00000002

?

Especially since we centralized the thread-id definition for multi-process:

 "In addition, the remote protocol supports a multiprocess feature
 in which the thread-id syntax is extended to optionally include both
 process and thread ID fields, as `ppid.tid'. The pid (process) and
 tid (thread) components each have the format described above: a
 positive number with target-specific interpretation formatted as
 a big-endian hex string, literal `-1' to indicate all processes or
 threads (respectively), or `0' to indicate an arbitrary process or
 thread. Specifying just a process, as `ppid', is equivalent to
 `ppid.-1'. It is an error to specify all processes but a specific
 thread, such as `p-1.tid'. Note that the `p' prefix is not used
 for those packets and replies explicitly documented to include a
 process ID, rather than a thread-id. "

... and we nowhere hardcoded 64-bit hex numbers.

Perhaps the previous patch was too invasive for such a simple need.
Since this is only required in the thread alive packet, and since
we already have to special case... would you be happy with the
attached patch?  I know I would.  :-)

-- 
Pedro Alves

[-- Attachment #2: thread_alive.diff --]
[-- Type: text/x-diff, Size: 1810 bytes --]

2008-10-23  Michael Snyder  <msnyder@vmware.com>
	    Pedro Alves  <pedro@codesourcery.com>

	* remote.c (remote_thread_alive): Emit leading zeros in the
	single-process case to preserve remote protocol behavior.

---
 gdb/remote.c |   28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

Index: src/gdb/remote.c
===================================================================
--- src.orig/gdb/remote.c	2008-10-23 15:57:07.000000000 +0100
+++ src/gdb/remote.c	2008-10-23 16:30:19.000000000 +0100
@@ -1279,8 +1279,6 @@ static int
 remote_thread_alive (ptid_t ptid)
 {
   struct remote_state *rs = get_remote_state ();
-  int tid = ptid_get_tid (ptid);
-  char *p, *endp;
 
   if (ptid_equal (ptid, magic_null_ptid))
     /* The main thread is always alive.  */
@@ -1292,11 +1290,29 @@ remote_thread_alive (ptid_t ptid)
        multi-threading.  */
     return 1;
 
-  p = rs->buf;
-  endp = rs->buf + get_remote_packet_size ();
+  /* Note: We don't use write_ptid unconditionally here, since it
+     doesn't output leading zeros (%08x below).  Although nothing in
+     the docs suggests that the leading zeros are required, there's no
+     reason to break backwards compatibility, in case stubs are
+     relying on them being present.  */
+  if (!remote_multi_process_p (rs))
+    {
+      int tid = ptid_get_tid (ptid);
+
+      if (tid < 0)
+	xsnprintf (rs->buf, get_remote_packet_size (), "T-%08x", -tid);
+      else
+	xsnprintf (rs->buf, get_remote_packet_size (), "T%08x", tid);
+    }
+  else
+    {
+      char *p, *endp;
 
-  *p++ = 'T';
-  write_ptid (p, endp, ptid);
+      p = rs->buf;
+      endp = rs->buf + get_remote_packet_size ();
+      *p++ = 'T';
+      write_ptid (p, endp, ptid);
+    }
 
   putpkt (rs->buf);
   getpkt (&rs->buf, &rs->buf_size, 0);

Re: [RFA] Restore leading zeros in remote_thread_alive

[Michael Snyder]

Pedro Alves wrote:
> On Wednesday 22 October 2008 19:05:09, Michael Snyder wrote:
>> Pedro Alves wrote:
>>
>>> Could you try this out please?  It works here against gdbserver
>>> single|multi-process, and sends a `T000000tid' (tid alive) packet when
>>> multi-process isn't in effect.
>> Thanks for working up the patch -- it works for my target.
> 
> Thanks for testing.
> 
>> However, I must say that I'm not crazy about the idea that
>> we decide whether or not to send leading zeroes based on
>> whether the target is multi-process.  Seems orthogonal
>> and ad hoc.
> 
> I can't think of a valid reason why we'd have this
> inconsistency:

Yeah -- Pedro, I'm going to withdraw the request.
I decided to fix the client (breakage was worse than
I thought -- it was actually discarding the first byte
of data, which only succeeded because it happened to be
zero).

I don't think it's necessary to uglify remote.c to
handle this issue, unles someone else reports a problem
with it -- but I leave the decision up to you.  ;-)

Thanks for your trouble,
Michael