Small problem with Remote Protocol register fetching.

Small problem with Remote Protocol register fetching.

[Steven Johnson]

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

Registers in the remote protocol are Hex Encoded.  Hex encoded values 
can have (as far as I can tell, valid values of '0'-'9','a'-'f','A'-'F' 
and ('x' for registers). the problem is that register packets that have 
an upper case 'A'-'F' in the first location are junked as being bad 
packets, when their is nothing wrong.  And then GDB ends up in an 
infinite comms loop, trying to recover.

The attached patch allows Hex Encoded values to include upper case 
letters (in the case of fetching registers) without causing the packet 
handling to fail.

I wasnt sure if 'X' should also be allowable, seems like it should, but 
i dont know for sure, so havent changed it.

Steven Johnson



[-- Attachment #2: gdb-6.1-rsp-hexencodecasecheck.patch --]
[-- Type: text/x-patch, Size: 625 bytes --]

diff -p -u -r clean-src/insight-6.1/gdb/frame.c mod-src/insight-6.1/gdb/frame.c
diff -p -u -r clean-src/insight-6.1/gdb/remote.c mod-src/insight-6.1/gdb/remote.c
--- clean-src/insight-6.1/gdb/remote.c	2004-02-26 06:41:00.000000000 +1000
+++ mod-src/insight-6.1/gdb/remote.c	2004-06-15 16:36:53.000000000 +1000
@@ -3278,6 +3278,7 @@ remote_fetch_registers (int regnum)
      and try to fetch another packet to read.  */
   while ((buf[0] < '0' || buf[0] > '9')
 	 && (buf[0] < 'a' || buf[0] > 'f')
+	 && (buf[0] < 'A' || buf[0] > 'F')
 	 && buf[0] != 'x')	/* New: unavailable register value */
     {
       if (remote_debug)

Re: Small problem with Remote Protocol register fetching.

[Andrew Cagney]

> Registers in the remote protocol are Hex Encoded.  Hex encoded values can have (as far as I can tell, valid values of '0'-'9','a'-'f','A'-'F' and ('x' for registers). the problem is that register packets that have an upper case 'A'-'F' in the first location are junked as being bad packets, when their is nothing wrong.  And then GDB ends up in an infinite comms loop, trying to recover.
> 
> The attached patch allows Hex Encoded values to include upper case letters (in the case of fetching registers) without causing the packet handling to fail.
> 
> I wasnt sure if 'X' should also be allowable, seems like it should, but i dont know for sure, so havent changed it.

Hmm, the code's been that way since, well, forever and no one noticed! 
Perhaphs we should just clarify the spec and define it as lowercase 
@sc{hex}.

Andrew

Re: Small problem with Remote Protocol register fetching.

[Michael Snyder]

Steven Johnson wrote:
> Registers in the remote protocol are Hex Encoded.  Hex encoded values 
> can have (as far as I can tell, valid values of '0'-'9','a'-'f','A'-'F' 
> and ('x' for registers). the problem is that register packets that have 
> an upper case 'A'-'F' in the first location are junked as being bad 
> packets, when their is nothing wrong.  And then GDB ends up in an 
> infinite comms loop, trying to recover.
> 
> The attached patch allows Hex Encoded values to include upper case 
> letters (in the case of fetching registers) without causing the packet 
> handling to fail.
> 
> I wasnt sure if 'X' should also be allowable, seems like it should, but 
> i dont know for sure, so havent changed it.
> 
> Steven Johnson

What Andrew said, but OTOH, this is certainly not an intrusive change.
Steven, what remote target are you seeing this with?

[Someone will say "but then we'll have to make sure that we consistantly
accept upper case throughout, and I agree that's a little more intrusive]


> ------------------------------------------------------------------------
> 
> diff -p -u -r clean-src/insight-6.1/gdb/frame.c mod-src/insight-6.1/gdb/frame.c
> diff -p -u -r clean-src/insight-6.1/gdb/remote.c mod-src/insight-6.1/gdb/remote.c
> --- clean-src/insight-6.1/gdb/remote.c	2004-02-26 06:41:00.000000000 +1000
> +++ mod-src/insight-6.1/gdb/remote.c	2004-06-15 16:36:53.000000000 +1000
> @@ -3278,6 +3278,7 @@ remote_fetch_registers (int regnum)
>       and try to fetch another packet to read.  */
>    while ((buf[0] < '0' || buf[0] > '9')
>  	 && (buf[0] < 'a' || buf[0] > 'f')
> +	 && (buf[0] < 'A' || buf[0] > 'F')
>  	 && buf[0] != 'x')	/* New: unavailable register value */
>      {
>        if (remote_debug)

Re: Small problem with Remote Protocol register fetching.

[Steven Johnson]

Michael, Andrew,

I dont have a problem with it if it is defined to be lower case, but I 
checked the spec, and it wasnt defined anywhere (that I could find) 
exactly what Hex Encoding is, and what characters are valid.  So I went 
and looked for the function that interprets Hex Encoded characters.

remote-utils.c contains a fromhex function, that only supports lower case.
gdbtk-cmds.c contains a fromhex function, that supports upper and lower 
case.
monitor.c contains a fromhex function, that supports upper and lower case.
remote-sds.c contains a fromhex function, that only supports lower case.
remote.c contains a fromhex function, that supports upper and lower 
case. (This is the only one that seems to be used by the standard remote 
protocol)

There being more Hex Decoding functions supporting Upper and Lower case, 
I thought supporting both cases was the intention.  If the intention is 
that only lower case is the way it is encoded, I dont have any problem 
with this, but there needs to be a section added to the Remote Serial 
Protocol defining what Hex Encoding is, and what values are valid.  
Supporting Upper case in this one function didnt seem like a big change, 
if any others come up, where people have a problem with upper case, it 
will show itself in time, and exists now anyway.

If anyone does what I did and look for the "from_hex" function in 
remote.c, which is used to decode hex encoded values, and if lower case 
is all that is intended. The ability to decode upper case should be 
removed from remote.c from_hex.  I would suggest still doing it for a 
couple of versions, but print a warning suggesting Upper Case hex 
encoding is deprecated, in case there are targets out there that use 
upper case hex encoding for things like data read packets, etc.

BTW, I checked for all use of fromhex in remote.c, and the only place I 
can find any pre-checking of the Hex values is the one that is patched.  
So I am pretty confident it would fix the only place where upper case 
letters arent fully supported.

If the consensus is it should be lower case only, I will work up a patch 
to deprecate upper case from "fromhex" and an addition to the 
documentation to specify.  If upper case is allowed, I will work up 
another patch. A patch to the GDB documentation to describe what hex 
encoding is. (Unless there already is one that ive missed.)

So whatever the consenus view is, ill go that way. I dont care either 
way, it just seemed inconsistent currently.

Steven.



Michael Snyder wrote:

> Steven Johnson wrote:
>
>> Registers in the remote protocol are Hex Encoded.  Hex encoded values 
>> can have (as far as I can tell, valid values of 
>> '0'-'9','a'-'f','A'-'F' and ('x' for registers). the problem is that 
>> register packets that have an upper case 'A'-'F' in the first 
>> location are junked as being bad packets, when their is nothing 
>> wrong.  And then GDB ends up in an infinite comms loop, trying to 
>> recover.
>>
>> The attached patch allows Hex Encoded values to include upper case 
>> letters (in the case of fetching registers) without causing the 
>> packet handling to fail.
>>
>> I wasnt sure if 'X' should also be allowable, seems like it should, 
>> but i dont know for sure, so havent changed it.
>>
>> Steven Johnson
>
>
> What Andrew said, but OTOH, this is certainly not an intrusive change.
> Steven, what remote target are you seeing this with?
>
> [Someone will say "but then we'll have to make sure that we consistantly
> accept upper case throughout, and I agree that's a little more intrusive]
>
>
>> ------------------------------------------------------------------------
>>
>> diff -p -u -r clean-src/insight-6.1/gdb/frame.c 
>> mod-src/insight-6.1/gdb/frame.c
>> diff -p -u -r clean-src/insight-6.1/gdb/remote.c 
>> mod-src/insight-6.1/gdb/remote.c
>> --- clean-src/insight-6.1/gdb/remote.c    2004-02-26 
>> 06:41:00.000000000 +1000
>> +++ mod-src/insight-6.1/gdb/remote.c    2004-06-15 16:36:53.000000000 
>> +1000
>> @@ -3278,6 +3278,7 @@ remote_fetch_registers (int regnum)
>>       and try to fetch another packet to read.  */
>>    while ((buf[0] < '0' || buf[0] > '9')
>>       && (buf[0] < 'a' || buf[0] > 'f')
>> +     && (buf[0] < 'A' || buf[0] > 'F')
>>       && buf[0] != 'x')    /* New: unavailable register value */
>>      {
>>        if (remote_debug)
>
>
>
>

Re: Small problem with Remote Protocol register fetching.

[Andrew Cagney]

Hmm, you should be wanting to change your remote target :-) :-)

By doing that you ensure that the remote-target interoperates with any 
released GDBs.  If you don't you ensure that at best your target 
operates with only a not-yet-released mainline GDB.

As for remote.c and the protocol, yes need to think of something.  My 
opinion is that, regardless, we need to change the spec to ``lower-case 
@sc{hex}'' - to ensure that compatibility.

Andrew

Re: Small problem with Remote Protocol register fetching.

[Steven Johnson]

Yes, the target was changed before the original post.

It just pointed out a minor inconsistency, tis all.  One that should be 
easy to remove either way.

Steven

Andrew Cagney wrote:

> Hmm, you should be wanting to change your remote target :-) :-)
>
> By doing that you ensure that the remote-target interoperates with any 
> released GDBs.  If you don't you ensure that at best your target 
> operates with only a not-yet-released mainline GDB.
>
> As for remote.c and the protocol, yes need to think of something.  My 
> opinion is that, regardless, we need to change the spec to 
> ``lower-case @sc{hex}'' - to ensure that compatibility.
>
> Andrew
>
>
>

Re: Small problem with Remote Protocol register fetching.

[Andrew Cagney]

> Yes, the target was changed before the original post.

yo!

> It just pointed out a minor inconsistency, tis all.  One that should be easy to remove either way.

Yes, leaves us with two things:
- fix the doco so that it is explicit about lowercase - needed for 
backward compatibility.
- change GDB so it doesn't go so awol when it encounters ucase
On the second, I just wonder about "Enn", at present its detected as an 
upper case "e" and a length of three.

Andrew