regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

[Andrew Sutherland]

The use of read_ptid replaced use of strtoul.  strtoul eats leading 
whitespace, read_ptid does not.

The protocol documentation explicitly provides for the existence of 
whitespace: `m thread-id' and `m thread-id,thread-id...'

I believe the following patch solves the problem without any ill 
effects.  My fix, however, is localized to remote_threads_info and so 
would not deal with any other regressions.

Patch:
====

diff --git a/gdb/remote.c b/gdb/remote.c
index 4c58abb..310c8f7 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2239,6 +2239,9 @@ remote_threads_info (struct target_ops *ops)
         {
           while (*bufp++ == 'm')        /* reply contains one or more 
TID */
             {
+              if (*bufp == ' ')
+                bufp++;
+
               do
                 {
                   new_thread = read_ptid (bufp, &bufp);

===

Origin of the problem:

Offending commit: b34f362bd3708bb786d64dc819ad2b4b1f84f55f

gitweb:
http://sourceware.org/git/gitweb.cgi?p=archer.git;a=commit;h=b34f362bd3708bb786d64dc819ad2b4b1f84f55f

hunk before the patch:
http://sourceware.org/git/gitweb.cgi?p=archer.git;a=blob;f=gdb/remote.c;h=3433e9d2ffed5f77b0035722b76e77c234df376e;hb=3433e9d2ffed5f77b0035722b76e77c234df376e#l1971

hunk after the patch:
http://sourceware.org/git/gitweb.cgi?p=archer.git;a=blob;f=gdb/remote.c;h=dfae2f68386662a52e3c26b424596010ea229d52;hb=dfae2f68386662a52e3c26b424596010ea229d52#l2056

Andrew

Re: regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

[Pedro Alves]

On Thursday 10 September 2009 06:25:17, Andrew Sutherland wrote:
> The use of read_ptid replaced use of strtoul.  strtoul eats leading 
> whitespace, read_ptid does not.
> 
> The protocol documentation explicitly provides for the existence of 
> whitespace: `m thread-id' and `m thread-id,thread-id...'

Note that a bit above it reads:

"Like the descriptions of the other packets, each description here has a template showing the packet's
overall syntax, followed by an explanation of the packet's meaning. We include spaces in some of the
templates for clarity; these are not part of the packet's syntax. No GDB packet uses
spaces to separate its components."

-- 
Pedro Alves

Re: regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

[Andrew Sutherland]

On 09/10/2009 03:11 AM, Pedro Alves wrote:
> "Like the descriptions of the other packets, each description here has a template showing the packet's
> overall syntax, followed by an explanation of the packet's meaning. We include spaces in some of the
> templates for clarity; these are not part of the packet's syntax. No GDB packet uses
> spaces to separate its components."

I did miss that, thank you.  And it certainly does make sense to not 
include whitespace in a machine protocol...

Is there any form of policy on backwards compatibility for the remote 
debugging protocol?  The problematic gdb stub is the VMware stub (as 
found in Workstation 6.x), and when notifying them of this problem, it'd 
be good to know how much of a problem it in fact is.

Thanks,
Andrew

Re: regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

[Pedro Alves]

On Thursday 10 September 2009 22:52:12, Andrew Sutherland wrote:

> The problematic gdb stub is the VMware stub (as  
> found in Workstation 6.x), and when notifying them of this problem, it'd 
> be good to know how much of a problem it in fact is.

This isn't the first time I hear VMware stub bugs related to bad
thread id handling.  See here:

 http://sourceware.org/ml/gdb-patches/2008-10/msg00586.html

In that message, you see that Michael decided to fix the stub instead. I assume
he must have stumbled on this case you're reporting as well, since it sounds
related, but didn't bother to raise it.  So it seems that handling just
this case wouldn't fix your whole problem.

-- 
Pedro Alves

Re: regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

[Pedro Alves]

On Friday 11 September 2009 15:36:06, Pedro Alves wrote:

>  http://sourceware.org/ml/gdb-patches/2008-10/msg00586.html
> 
> In that message, you see that Michael decided to fix the stub instead. I assume
> he must have stumbled on this case you're reporting as well, since it sounds
> related, but didn't bother to raise it.  So it seems that handling just
> this case wouldn't fix your whole problem.

Correction, he did report the issue you're reporting, so it was
indeed related:

 http://sourceware.org/ml/gdb-patches/2008-10/msg00528.html

-- 
Pedro Alves

Re: regression in remote.c in remote_threads_info for qfThreadInfo/qsThreadInfo with the introduction of read_ptid

[Tom Tromey]

>>>>> "Andrew" == Andrew Sutherland <sombrero@alum.mit.edu> writes:

Andrew> The use of read_ptid replaced use of strtoul.  strtoul eats leading
Andrew> whitespace, read_ptid does not.

FWIW I think this patch (or any rewrite) would better be sent to
gdb-patches.

Tom