[PATCH] remote protocol cleanups

[PATCH] remote protocol cleanups

[Nathan Sidwell]

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

Hi,
This patch fixes a couple of problems with the remote protcol handling

1) remote_fetch_registers checks to see things have not got out of
sync, but it does not deal with upper case hex characters and so can get
confused

2) remote_write_bytes attempts to honour get_memory_write_packet_size's
limit, but fails in two ways when that is a very small number.  a) it can
end up with a negative byte count. b) it deducts the number of chars in
the *maximal* length count, not the number of chars in the actual length
count.  This can result in packets sending 1 or 2 bytes fewer than they
are limited to.

built & tested on i686-pc-linux-gnu and an unreleased architecture. ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: remote.patch --]
[-- Type: text/plain, Size: 2405 bytes --]

2005-02-09  Nathan Sidwell  <nathan@codesourcery.com>

	* remote.c (remote_fetch_registers): Allow uppercase hex when
	resyncing.
	(remote_write_bytes): Robustify packet size calculation for very
	small packets.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.163
diff -c -3 -p -r1.163 remote.c
*** remote.c	19 Jan 2005 21:15:44 -0000	1.163
--- remote.c	9 Feb 2005 11:09:39 -0000
*************** remote_fetch_registers (int regnum)
*** 3307,3312 ****
--- 3307,3313 ----
       in the buffer is not a hex character, assume that has happened
       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.  */
      {
*************** remote_write_bytes (CORE_ADDR memaddr, c
*** 3640,3649 ****
    buf = alloca (sizeof_buf);
  
    /* Compute the size of the actual payload by subtracting out the
!      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".  */
!   payload_size = (get_memory_write_packet_size () - (strlen ("$M,:#NN")
! 						     + hexnumlen (memaddr)
! 						     + hexnumlen (len)));
  
    /* Construct the packet header: "[MX]<memaddr>,<len>:".   */
  
--- 3641,3651 ----
    buf = alloca (sizeof_buf);
  
    /* Compute the size of the actual payload by subtracting out the
!      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".
!      */
!   payload_size = get_memory_write_packet_size ();
!   payload_size -= strlen ("$M,:#NN");
!   payload_size -= hexnumlen (memaddr);
  
    /* Construct the packet header: "[MX]<memaddr>,<len>:".   */
  
*************** remote_write_bytes (CORE_ADDR memaddr, c
*** 3656,3666 ****
--- 3658,3673 ----
        *p++ = 'X';
        /* Best guess at number of bytes that will fit.  */
        todo = min (len, payload_size);
+       payload_size -= hexnumlen (todo);
+       todo = min (todo, payload_size);
        break;
      case PACKET_DISABLE:
        *p++ = 'M';
        /* Num bytes that will fit.  */
        todo = min (len, payload_size / 2);
+       payload_size -= hexnumlen (todo);
+       todo = min (todo, payload_size / 2);
+       todo = max (todo, 1);
        break;
      case PACKET_SUPPORT_UNKNOWN:
        internal_error (__FILE__, __LINE__,

Re: [PATCH] remote protocol cleanups

[Daniel Jacobowitz]

On Wed, Feb 09, 2005 at 11:53:50AM +0000, Nathan Sidwell wrote:
> Hi,
> This patch fixes a couple of problems with the remote protcol handling
> 
> 1) remote_fetch_registers checks to see things have not got out of
> sync, but it does not deal with upper case hex characters and so can get
> confused
> 
> 2) remote_write_bytes attempts to honour get_memory_write_packet_size's
> limit, but fails in two ways when that is a very small number.  a) it can
> end up with a negative byte count. b) it deducts the number of chars in
> the *maximal* length count, not the number of chars in the actual length
> count.  This can result in packets sending 1 or 2 bytes fewer than they
> are limited to.
> 
> built & tested on i686-pc-linux-gnu and an unreleased architecture. ok?

Mostly OK, but one question.  I don't get the "max (todo, 1)".  If no
bytes fit, aren't we hosed?  It seems like an error condition; we
shouldn't be violating the size limit.

>       case PACKET_DISABLE:
>         *p++ = 'M';
>         /* Num bytes that will fit.  */
>         todo = min (len, payload_size / 2);
> +       payload_size -= hexnumlen (todo);
> +       todo = min (todo, payload_size / 2);
> +       todo = max (todo, 1);
>         break;
>       case PACKET_SUPPORT_UNKNOWN:
>         internal_error (__FILE__, __LINE__,


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] remote protocol cleanups

[Nathan Sidwell]

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

Daniel Jacobowitz wrote:
> On Wed, Feb 09, 2005 at 11:53:50AM +0000, Nathan Sidwell wrote:

>>2) remote_write_bytes attempts to honour get_memory_write_packet_size's
>>limit, but fails in two ways when that is a very small number.  a) it can
>>end up with a negative byte count. b) it deducts the number of chars in
>>the *maximal* length count, not the number of chars in the actual length
>>count.  This can result in packets sending 1 or 2 bytes fewer than they
>>are limited to.

> 
> Mostly OK, but one question.  I don't get the "max (todo, 1)".  If no
> bytes fit, aren't we hosed?  It seems like an error condition; we
> shouldn't be violating the size limit.

It appears the minimum size setting is somewhat arbitrary, and incorrect.
/* NOTE: 16 is just chosen at random.  */
#ifndef MIN_REMOTE_PACKET_SIZE
#define MIN_REMOTE_PACKET_SIZE 16
#endif

the minumum packet size is actually
7 - strlen ("$M,:#NN")
8 - hexnumlen (memaddr) (32 bit host)
1 - hexnumlen (len)
2 - data
= 18 characters

This patch fixes that size calculation. ok?

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::     CodeSourcery LLC
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: remote-2.patch --]
[-- Type: text/plain, Size: 4072 bytes --]

2005-03-08  Nathan Sidwell  <nathan@codesourcery.com>

	* remote.c (MIN_REMOTE_PACKET_SIZE): Set to 20.
	(remote_fetch_registers): Allow uppercase hex when resyncing.
	(remote_write_bytes): Only call get_memory_write_packet_size once.
	Robustify packet size calculation for very small packets. Check
	that at least one byte will be written.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.176
diff -c -3 -p -r1.176 remote.c
*** remote.c	4 Mar 2005 17:52:53 -0000	1.176
--- remote.c	8 Mar 2005 11:03:32 -0000
*************** get_memory_packet_size (struct memory_pa
*** 413,421 ****
  #ifndef MAX_REMOTE_PACKET_SIZE
  #define MAX_REMOTE_PACKET_SIZE 16384
  #endif
!   /* NOTE: 16 is just chosen at random.  */
  #ifndef MIN_REMOTE_PACKET_SIZE
! #define MIN_REMOTE_PACKET_SIZE 16
  #endif
    long what_they_get;
    if (config->fixed_p)
--- 413,421 ----
  #ifndef MAX_REMOTE_PACKET_SIZE
  #define MAX_REMOTE_PACKET_SIZE 16384
  #endif
!   /* NOTE: 20 ensures we can write at least one byte.  */
  #ifndef MIN_REMOTE_PACKET_SIZE
! #define MIN_REMOTE_PACKET_SIZE 20
  #endif
    long what_they_get;
    if (config->fixed_p)
*************** remote_fetch_registers (int regnum)
*** 3332,3337 ****
--- 3332,3338 ----
       in the buffer is not a hex character, assume that has happened
       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.  */
      {
*************** remote_write_bytes (CORE_ADDR memaddr, c
*** 3659,3674 ****
    /* Verify that the target can support a binary download.  */
    check_binary_download (memaddr);
  
    /* Compute the size, and then allocate space for the largest
!      possible packet.  Include space for an extra trailing NULL.  */
!   sizeof_buf = get_memory_write_packet_size () + 1;
    buf = alloca (sizeof_buf);
  
    /* Compute the size of the actual payload by subtracting out the
!      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".  */
!   payload_size = (get_memory_write_packet_size () - (strlen ("$M,:#NN")
! 						     + hexnumlen (memaddr)
! 						     + hexnumlen (len)));
  
    /* Construct the packet header: "[MX]<memaddr>,<len>:".   */
  
--- 3660,3677 ----
    /* Verify that the target can support a binary download.  */
    check_binary_download (memaddr);
  
+   payload_size = get_memory_write_packet_size ();
+   
    /* Compute the size, and then allocate space for the largest
!      possible packet.  Include space for an extra trailing NUL.  */
!   sizeof_buf = payload_size + 1;
    buf = alloca (sizeof_buf);
  
    /* Compute the size of the actual payload by subtracting out the
!      packet header and footer overhead: "$M<memaddr>,<len>:...#nn".
!      */
!   payload_size -= strlen ("$M,:#NN");
!   payload_size -= hexnumlen (memaddr);
  
    /* Construct the packet header: "[MX]<memaddr>,<len>:".   */
  
*************** remote_write_bytes (CORE_ADDR memaddr, c
*** 3681,3691 ****
--- 3684,3698 ----
        *p++ = 'X';
        /* Best guess at number of bytes that will fit.  */
        todo = min (len, payload_size);
+       payload_size -= hexnumlen (todo);
+       todo = min (todo, payload_size);
        break;
      case PACKET_DISABLE:
        *p++ = 'M';
        /* Num bytes that will fit.  */
        todo = min (len, payload_size / 2);
+       payload_size -= hexnumlen (todo);
+       todo = min (todo, payload_size / 2);
        break;
      case PACKET_SUPPORT_UNKNOWN:
        internal_error (__FILE__, __LINE__,
*************** remote_write_bytes (CORE_ADDR memaddr, c
*** 3693,3698 ****
--- 3700,3708 ----
      default:
        internal_error (__FILE__, __LINE__, _("bad switch"));
      }
+   if (todo <= 0)
+     internal_error (__FILE__, __LINE__,
+ 		    _("minumum packet size too small to write data"));
  
    /* Append "<memaddr>".  */
    memaddr = remote_address_masked (memaddr);

Re: [PATCH] remote protocol cleanups

[Daniel Jacobowitz]

On Tue, Mar 08, 2005 at 11:09:39AM +0000, Nathan Sidwell wrote:
> Daniel Jacobowitz wrote:
> >On Wed, Feb 09, 2005 at 11:53:50AM +0000, Nathan Sidwell wrote:
> 
> >>2) remote_write_bytes attempts to honour get_memory_write_packet_size's
> >>limit, but fails in two ways when that is a very small number.  a) it can
> >>end up with a negative byte count. b) it deducts the number of chars in
> >>the *maximal* length count, not the number of chars in the actual length
> >>count.  This can result in packets sending 1 or 2 bytes fewer than they
> >>are limited to.
> 
> >
> >Mostly OK, but one question.  I don't get the "max (todo, 1)".  If no
> >bytes fit, aren't we hosed?  It seems like an error condition; we
> >shouldn't be violating the size limit.
> 
> It appears the minimum size setting is somewhat arbitrary, and incorrect.
> /* NOTE: 16 is just chosen at random.  */
> #ifndef MIN_REMOTE_PACKET_SIZE
> #define MIN_REMOTE_PACKET_SIZE 16
> #endif
> 
> the minumum packet size is actually
> 7 - strlen ("$M,:#NN")
> 8 - hexnumlen (memaddr) (32 bit host)
> 1 - hexnumlen (len)
> 2 - data
> = 18 characters
> 
> This patch fixes that size calculation. ok?

Yes, this is OK.


-- 
Daniel Jacobowitz
CodeSourcery, LLC