[RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[Wu Zhou]

Hello all,

I coded a patch to replace sprintf calls (mainly these error-prone 
and easy-to-fix ones) in remote.c with xsnprintf.  The fixs for two
typos are also included.  Had tested it on ppc64.  No regression.

Please review and comment.  Thanks.

<2005-05-26>  Wu Zhou  <woodzltc@cn.ibm.com>

	* remote.c: Fix two comment typos.

	* remote.c (set_thread, remote_thread_alive): Replace sprintf 
	call with xsnprintf.
	(remote_threads_extra_info, extended_remote_restart)
	(remote_check_symbols, store_register_using_P)
	(compare_sections_command): Ditto.


Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.190
diff -c -p -r1.190 remote.c
*** remote.c	16 May 2005 16:36:24 -0000	1.190
--- remote.c	26 May 2005 05:56:50 -0000
*************** record_currthread (int currthread)
*** 1037,1042 ****
--- 1037,1043 ----
  }
  
  #define MAGIC_NULL_PID 42000
+ #define REMOTE_PACKET_SIZE (rs->remote_packet_size)
  
  static void
  set_thread (int th, int gen)
*************** set_thread (int th, int gen)
*** 1056,1064 ****
        buf[3] = '\0';
      }
    else if (th < 0)
!     sprintf (&buf[2], "-%x", -th);
    else
!     sprintf (&buf[2], "%x", th);
    putpkt (buf);
    getpkt (buf, (rs->remote_packet_size), 0);
    if (gen)
--- 1057,1065 ----
        buf[3] = '\0';
      }
    else if (th < 0)
!     xsnprintf (&buf[2], REMOTE_PACKET_SIZE - 2, "-%x", -th);
    else
!     xsnprintf (&buf[2], REMOTE_PACKET_SIZE - 2, "%x", th);
    putpkt (buf);
    getpkt (buf, (rs->remote_packet_size), 0);
    if (gen)
*************** remote_thread_alive (ptid_t ptid)
*** 1076,1084 ****
    char buf[16];
  
    if (tid < 0)
!     sprintf (buf, "T-%08x", -tid);
    else
!     sprintf (buf, "T%08x", tid);
    putpkt (buf);
    getpkt (buf, sizeof (buf), 0);
    return (buf[0] == 'O' && buf[1] == 'K');
--- 1077,1085 ----
    char buf[16];
  
    if (tid < 0)
!     xsnprintf (buf, sizeof (buf), "T-%08x", -tid);
    else
!     xsnprintf (buf, sizeof (buf), "T%08x", tid);
    putpkt (buf);
    getpkt (buf, sizeof (buf), 0);
    return (buf[0] == 'O' && buf[1] == 'K');
*************** struct gdb_ext_thread_info
*** 1114,1120 ****
      int active;			/* Has state interesting to GDB? 
  				   regs, stack.  */
      char display[256];		/* Brief state display, name, 
! 				   blocked/syspended.  */
      char shortname[32];		/* To be used to name threads.  */
      char more_display[256];	/* Long info, statistics, queue depth, 
  				   whatever.  */
--- 1115,1121 ----
      int active;			/* Has state interesting to GDB? 
  				   regs, stack.  */
      char display[256];		/* Brief state display, name, 
! 				   blocked/suspended.  */
      char shortname[32];		/* To be used to name threads.  */
      char more_display[256];	/* Long info, statistics, queue depth, 
  				   whatever.  */
*************** remote_unpack_thread_info_response (char
*** 1489,1495 ****
    int mask, length;
    unsigned int tag;
    threadref ref;
!   char *limit = pkt + (rs->remote_packet_size);	/* plausable parsing limit */
    int retval = 1;
  
    /* info->threadid = 0; FIXME: implement zero_threadref.  */
--- 1490,1496 ----
    int mask, length;
    unsigned int tag;
    threadref ref;
!   char *limit = pkt + (rs->remote_packet_size);	/* plausible parsing limit */
    int retval = 1;
  
    /* info->threadid = 0; FIXME: implement zero_threadref.  */
*************** remote_threads_extra_info (struct thread
*** 1859,1865 ****
    int set;
    threadref id;
    struct gdb_ext_thread_info threadinfo;
!   static char display_buf[100];	/* arbitrary...  */
    char *bufp = alloca (rs->remote_packet_size);
    int n = 0;                    /* position in display_buf */
  
--- 1860,1868 ----
    int set;
    threadref id;
    struct gdb_ext_thread_info threadinfo;
!   /* change the buffer size to 600 to hold shortname, dispaly
!      and more_display of threadinfo.  */
!   static char display_buf[600];	
    char *bufp = alloca (rs->remote_packet_size);
    int n = 0;                    /* position in display_buf */
  
*************** remote_threads_extra_info (struct thread
*** 1869,1875 ****
  
    if (use_threadextra_query)
      {
!       sprintf (bufp, "qThreadExtraInfo,%x", PIDGET (tp->ptid));
        putpkt (bufp);
        getpkt (bufp, (rs->remote_packet_size), 0);
        if (bufp[0] != 0)
--- 1872,1879 ----
  
    if (use_threadextra_query)
      {
!       xsnprintf (bufp, REMOTE_PACKET_SIZE, "qThreadExtraInfo,%x", 
! 		 PIDGET (tp->ptid));
        putpkt (bufp);
        getpkt (bufp, (rs->remote_packet_size), 0);
        if (bufp[0] != 0)
*************** remote_threads_extra_info (struct thread
*** 1890,1901 ****
      if (threadinfo.active)
        {
  	if (*threadinfo.shortname)
! 	  n += sprintf(&display_buf[0], " Name: %s,", threadinfo.shortname);
  	if (*threadinfo.display)
! 	  n += sprintf(&display_buf[n], " State: %s,", threadinfo.display);
  	if (*threadinfo.more_display)
! 	  n += sprintf(&display_buf[n], " Priority: %s",
! 		       threadinfo.more_display);
  
  	if (n > 0)
  	  {
--- 1894,1907 ----
      if (threadinfo.active)
        {
  	if (*threadinfo.shortname)
! 	  n += xsnprintf (&display_buf[0], sizeof (display_buf) - n, 
! 			  " Name: %s,", threadinfo.shortname);
  	if (*threadinfo.display)
! 	  n += xsnprintf (&display_buf[n], sizeof (display_buf) - n, 
! 			  " State: %s,", threadinfo.display);
  	if (*threadinfo.more_display)
! 	  n += xsnprintf (&display_buf[n], sizeof (display_buf) - n, 
! 			  " Priority: %s", threadinfo.more_display);
  
  	if (n > 0)
  	  {
*************** extended_remote_restart (void)
*** 1921,1927 ****
    /* Send the restart command; for reasons I don't understand the
       remote side really expects a number after the "R".  */
    buf[0] = 'R';
!   sprintf (&buf[1], "%x", 0);
    putpkt (buf);
  
    /* Now query for status so this looks just like we restarted
--- 1927,1933 ----
    /* Send the restart command; for reasons I don't understand the
       remote side really expects a number after the "R".  */
    buf[0] = 'R';
!   xsnprintf (&buf[1], REMOTE_PACKET_SIZE - 1, "%x", 0);
    putpkt (buf);
  
    /* Now query for status so this looks just like we restarted
*************** remote_check_symbols (struct objfile *ob
*** 2138,2148 ****
        msg[end] = '\0';
        sym = lookup_minimal_symbol (msg, NULL, NULL);
        if (sym == NULL)
! 	sprintf (msg, "qSymbol::%s", &reply[8]);
        else
! 	sprintf (msg, "qSymbol:%s:%s",
! 		 paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
! 		 &reply[8]);
        putpkt (msg);
        getpkt (reply, (rs->remote_packet_size), 0);
      }
--- 2144,2154 ----
        msg[end] = '\0';
        sym = lookup_minimal_symbol (msg, NULL, NULL);
        if (sym == NULL)
! 	xsnprintf (msg, REMOTE_PACKET_SIZE, "qSymbol::%s", &reply[8]);
        else
! 	xsnprintf (msg, REMOTE_PACKET_SIZE, "qSymbol:%s:%s",
! 		   paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
! 		   &reply[8]);
        putpkt (msg);
        getpkt (reply, (rs->remote_packet_size), 0);
      }
*************** store_register_using_P (int regnum)
*** 3435,3441 ****
    char regp[MAX_REGISTER_SIZE];
    char *p;
  
!   sprintf (buf, "P%s=", phex_nz (reg->pnum, 0));
    p = buf + strlen (buf);
    regcache_raw_collect (current_regcache, reg->regnum, regp);
    bin2hex (regp, p, register_size (current_gdbarch, reg->regnum));
--- 3441,3447 ----
    char regp[MAX_REGISTER_SIZE];
    char *p;
  
!   xsnprintf (buf, REMOTE_PACKET_SIZE, "P%s=", phex_nz (reg->pnum, 0));
    p = buf + strlen (buf);
    regcache_raw_collect (current_regcache, reg->regnum, regp);
    bin2hex (regp, p, register_size (current_gdbarch, reg->regnum));
*************** compare_sections_command (char *args, in
*** 4908,4914 ****
        matched = 1;		/* do this section */
        lma = s->lma;
        /* FIXME: assumes lma can fit into long.  */
!       sprintf (buf, "qCRC:%lx,%lx", (long) lma, (long) size);
        putpkt (buf);
  
        /* Be clever; compute the host_crc before waiting for target
--- 4914,4921 ----
        matched = 1;		/* do this section */
        lma = s->lma;
        /* FIXME: assumes lma can fit into long.  */
!       xsnprintf (buf, REMOTE_PACKET_SIZE, "qCRC:%lx,%lx", 
! 		 (long) lma, (long) size);
        putpkt (buf);
  
        /* Be clever; compute the host_crc before waiting for target

Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[Daniel Jacobowitz]

On Thu, May 26, 2005 at 07:00:16AM -0700, Wu Zhou wrote:
> Hello all,
> 
> I coded a patch to replace sprintf calls (mainly these error-prone 
> and easy-to-fix ones) in remote.c with xsnprintf.  The fixs for two
> typos are also included.  Had tested it on ppc64.  No regression.
> 
> Please review and comment.  Thanks.

Thanks for doing this.  It needs a couple of small changes.

> <2005-05-26>  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* remote.c: Fix two comment typos.

Please do this as a separate patch.

>   #define MAGIC_NULL_PID 42000
> + #define REMOTE_PACKET_SIZE (rs->remote_packet_size)

Please don't do this.  Macros which reference local variables are very
bad style, and confusing.

> *************** remote_threads_extra_info (struct thread
> *** 1859,1865 ****
>     int set;
>     threadref id;
>     struct gdb_ext_thread_info threadinfo;
> !   static char display_buf[100];	/* arbitrary...  */
>     char *bufp = alloca (rs->remote_packet_size);
>     int n = 0;                    /* position in display_buf */
>   
> --- 1860,1868 ----
>     int set;
>     threadref id;
>     struct gdb_ext_thread_info threadinfo;
> !   /* change the buffer size to 600 to hold shortname, dispaly
> !      and more_display of threadinfo.  */
> !   static char display_buf[600];	
>     char *bufp = alloca (rs->remote_packet_size);
>     int n = 0;                    /* position in display_buf */
>   

Comments are full sentences (and you spelled display wrong).  You
didn't mention in the changelog that you changed the size of the
buffer; I don't think it's necessary.  Remember that remote_packet_size
is target-dependent.  If 100 has been good enough so far, it probably
still is (though using xsnprintf is still a good thing).

> --- 1927,1933 ----
>     /* Send the restart command; for reasons I don't understand the
>        remote side really expects a number after the "R".  */
>     buf[0] = 'R';
> !   xsnprintf (&buf[1], REMOTE_PACKET_SIZE - 1, "%x", 0);
>     putpkt (buf);
>   
>     /* Now query for status so this looks just like we restarted

Might as well use "R%x" while you're here.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[Wu Zhou]

On Sat, 28 May 2005, Daniel Jacobowitz wrote:

> >   #define MAGIC_NULL_PID 42000
> > + #define REMOTE_PACKET_SIZE (rs->remote_packet_size)
> 
> Please don't do this.  Macros which reference local variables are very
> bad style, and confusing.
> 

OK.  I switch back to rs->remote_packet_size.  Thanks for pointing this 
out.

> > *************** remote_threads_extra_info (struct thread
> > *** 1859,1865 ****
> >     int set;
> >     threadref id;
> >     struct gdb_ext_thread_info threadinfo;
> > !   static char display_buf[100];	/* arbitrary...  */
> >     char *bufp = alloca (rs->remote_packet_size);
> >     int n = 0;                    /* position in display_buf */
> >   
> > --- 1860,1868 ----
> >     int set;
> >     threadref id;
> >     struct gdb_ext_thread_info threadinfo;
> > !   /* change the buffer size to 600 to hold shortname, dispaly
> > !      and more_display of threadinfo.  */
> > !   static char display_buf[600];	
> >     char *bufp = alloca (rs->remote_packet_size);
> >     int n = 0;                    /* position in display_buf */
> >   
> 
> Comments are full sentences (and you spelled display wrong).  You
> didn't mention in the changelog that you changed the size of the
> buffer; I don't think it's necessary.  Remember that remote_packet_size
> is target-dependent.  If 100 has been good enough so far, it probably
> still is (though using xsnprintf is still a good thing).
> 
Thanks.  I will keep that in mind.  As for the size of the buffer, I 
still think that 100 might be overflowed in some scenario.  But I can't 
find such a scenario at this moment.  I drop this change anyway.   

> > --- 1927,1933 ----
> >     /* Send the restart command; for reasons I don't understand the
> >        remote side really expects a number after the "R".  */
> >     buf[0] = 'R';
> > !   xsnprintf (&buf[1], REMOTE_PACKET_SIZE - 1, "%x", 0);
> >     putpkt (buf);
> >   
> >     /* Now query for status so this looks just like we restarted
> 
> Might as well use "R%x" while you're here.

Good point. 

Below is the revised patch.  Please review and comment.  TIA.

<2005-05-29>  Wu Zhou  <woodzltc@cn.ibm.com>

	* remote.c (set_thread, remote_thread_alive): Replace sprintf
	call with xsnprintf.
	(remote_threads_extra_info, extended_remote_restart)
	(remote_check_symbols, store_register_using_P)
	(compare_sections_command): Ditto.


Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.192
diff -c -p -r1.192 remote.c
*** remote.c	28 May 2005 18:57:13 -0000	1.192
--- remote.c	29 May 2005 05:35:44 -0000
*************** set_thread (int th, int gen)
*** 1056,1064 ****
        buf[3] = '\0';
      }
    else if (th < 0)
!     sprintf (&buf[2], "-%x", -th);
    else
!     sprintf (&buf[2], "%x", th);
    putpkt (buf);
    getpkt (buf, (rs->remote_packet_size), 0);
    if (gen)
--- 1056,1064 ----
        buf[3] = '\0';
      }
    else if (th < 0)
!     xsnprintf (&buf[2], rs->remote_packet_size - 2, "-%x", -th);
    else
!     xsnprintf (&buf[2], rs->remote_packet_size - 2, "%x", th);
    putpkt (buf);
    getpkt (buf, (rs->remote_packet_size), 0);
    if (gen)
*************** remote_thread_alive (ptid_t ptid)
*** 1076,1084 ****
    char buf[16];
  
    if (tid < 0)
!     sprintf (buf, "T-%08x", -tid);
    else
!     sprintf (buf, "T%08x", tid);
    putpkt (buf);
    getpkt (buf, sizeof (buf), 0);
    return (buf[0] == 'O' && buf[1] == 'K');
--- 1076,1084 ----
    char buf[16];
  
    if (tid < 0)
!     xsnprintf (buf, sizeof (buf), "T-%08x", -tid);
    else
!     xsnprintf (buf, sizeof (buf), "T%08x", tid);
    putpkt (buf);
    getpkt (buf, sizeof (buf), 0);
    return (buf[0] == 'O' && buf[1] == 'K');
*************** remote_threads_extra_info (struct thread
*** 1869,1875 ****
  
    if (use_threadextra_query)
      {
!       sprintf (bufp, "qThreadExtraInfo,%x", PIDGET (tp->ptid));
        putpkt (bufp);
        getpkt (bufp, (rs->remote_packet_size), 0);
        if (bufp[0] != 0)
--- 1869,1876 ----
  
    if (use_threadextra_query)
      {
!       xsnprintf (bufp, rs->remote_packet_size, "qThreadExtraInfo,%x", 
! 		 PIDGET (tp->ptid));
        putpkt (bufp);
        getpkt (bufp, (rs->remote_packet_size), 0);
        if (bufp[0] != 0)
*************** remote_threads_extra_info (struct thread
*** 1890,1901 ****
      if (threadinfo.active)
        {
  	if (*threadinfo.shortname)
! 	  n += sprintf(&display_buf[0], " Name: %s,", threadinfo.shortname);
  	if (*threadinfo.display)
! 	  n += sprintf(&display_buf[n], " State: %s,", threadinfo.display);
  	if (*threadinfo.more_display)
! 	  n += sprintf(&display_buf[n], " Priority: %s",
! 		       threadinfo.more_display);
  
  	if (n > 0)
  	  {
--- 1891,1904 ----
      if (threadinfo.active)
        {
  	if (*threadinfo.shortname)
! 	  n += xsnprintf (&display_buf[0], sizeof (display_buf) - n, 
! 			  " Name: %s,", threadinfo.shortname);
  	if (*threadinfo.display)
! 	  n += xsnprintf (&display_buf[n], sizeof (display_buf) - n, 
! 			  " State: %s,", threadinfo.display);
  	if (*threadinfo.more_display)
! 	  n += xsnprintf (&display_buf[n], sizeof (display_buf) - n, 
! 			  " Priority: %s", threadinfo.more_display);
  
  	if (n > 0)
  	  {
*************** extended_remote_restart (void)
*** 1920,1927 ****
  
    /* Send the restart command; for reasons I don't understand the
       remote side really expects a number after the "R".  */
!   buf[0] = 'R';
!   sprintf (&buf[1], "%x", 0);
    putpkt (buf);
  
    /* Now query for status so this looks just like we restarted
--- 1923,1929 ----
  
    /* Send the restart command; for reasons I don't understand the
       remote side really expects a number after the "R".  */
!   xsnprintf (&buf, rs->remote_packet_size, "R%x", 0);
    putpkt (buf);
  
    /* Now query for status so this looks just like we restarted
*************** remote_check_symbols (struct objfile *ob
*** 2138,2148 ****
        msg[end] = '\0';
        sym = lookup_minimal_symbol (msg, NULL, NULL);
        if (sym == NULL)
! 	sprintf (msg, "qSymbol::%s", &reply[8]);
        else
! 	sprintf (msg, "qSymbol:%s:%s",
! 		 paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
! 		 &reply[8]);
        putpkt (msg);
        getpkt (reply, (rs->remote_packet_size), 0);
      }
--- 2140,2150 ----
        msg[end] = '\0';
        sym = lookup_minimal_symbol (msg, NULL, NULL);
        if (sym == NULL)
! 	xsnprintf (msg, rs->remote_packet_size, "qSymbol::%s", &reply[8]);
        else
! 	xsnprintf (msg, rs->remote_packet_size, "qSymbol:%s:%s",
! 		   paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
! 		   &reply[8]);
        putpkt (msg);
        getpkt (reply, (rs->remote_packet_size), 0);
      }
*************** store_register_using_P (int regnum)
*** 3435,3441 ****
    char regp[MAX_REGISTER_SIZE];
    char *p;
  
!   sprintf (buf, "P%s=", phex_nz (reg->pnum, 0));
    p = buf + strlen (buf);
    regcache_raw_collect (current_regcache, reg->regnum, regp);
    bin2hex (regp, p, register_size (current_gdbarch, reg->regnum));
--- 3437,3443 ----
    char regp[MAX_REGISTER_SIZE];
    char *p;
  
!   xsnprintf (buf, rs->remote_packet_size, "P%s=", phex_nz (reg->pnum, 0));
    p = buf + strlen (buf);
    regcache_raw_collect (current_regcache, reg->regnum, regp);
    bin2hex (regp, p, register_size (current_gdbarch, reg->regnum));
*************** compare_sections_command (char *args, in
*** 4908,4914 ****
        matched = 1;		/* do this section */
        lma = s->lma;
        /* FIXME: assumes lma can fit into long.  */
!       sprintf (buf, "qCRC:%lx,%lx", (long) lma, (long) size);
        putpkt (buf);
  
        /* Be clever; compute the host_crc before waiting for target
--- 4910,4917 ----
        matched = 1;		/* do this section */
        lma = s->lma;
        /* FIXME: assumes lma can fit into long.  */
!       xsnprintf (buf, rs->remote_packet_size, "qCRC:%lx,%lx", 
! 		 (long) lma, (long) size);
        putpkt (buf);
  
        /* Be clever; compute the host_crc before waiting for target
 

Cheers
- Wu Zhou

Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[Daniel Jacobowitz]

Sorry - not sure how this slipped through the cracks.

On Sun, May 29, 2005 at 07:07:45AM -0700, Wu Zhou wrote:
> Below is the revised patch.  Please review and comment.  TIA.
> 
> <2005-05-29>  Wu Zhou  <woodzltc@cn.ibm.com>
> 
> 	* remote.c (set_thread, remote_thread_alive): Replace sprintf
> 	call with xsnprintf.
> 	(remote_threads_extra_info, extended_remote_restart)
> 	(remote_check_symbols, store_register_using_P)
> 	(compare_sections_command): Ditto.

This is OK.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf

[Wu Zhou]

On Thu, 16 Jun 2005, Daniel Jacobowitz wrote:

> Sorry - not sure how this slipped through the cracks.

No problem.  

> 
> On Sun, May 29, 2005 at 07:07:45AM -0700, Wu Zhou wrote:
> > Below is the revised patch.  Please review and comment.  TIA.
> > 
> > <2005-05-29>  Wu Zhou  <woodzltc@cn.ibm.com>
> > 
> > 	* remote.c (set_thread, remote_thread_alive): Replace sprintf
> > 	call with xsnprintf.
> > 	(remote_threads_extra_info, extended_remote_restart)
> > 	(remote_check_symbols, store_register_using_P)
> > 	(compare_sections_command): Ditto.
> 
> This is OK.

Commited.  Thanks.

Cheers
- Wu Zhou