* [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
@ 2005-05-26 14:59 Wu Zhou
2005-05-28 18:53 ` Daniel Jacobowitz
0 siblings, 1 reply; 5+ messages in thread
From: Wu Zhou @ 2005-05-26 14:59 UTC (permalink / raw)
To: gdb-patches; +Cc: eli
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
2005-05-26 14:59 [RFC/Patch] replace sprintf calls in remote.c with xsnprintf Wu Zhou
@ 2005-05-28 18:53 ` Daniel Jacobowitz
2005-05-29 6:55 ` Wu Zhou
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Jacobowitz @ 2005-05-28 18:53 UTC (permalink / raw)
To: Wu Zhou; +Cc: gdb-patches, eliz
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
2005-05-28 18:53 ` Daniel Jacobowitz
@ 2005-05-29 6:55 ` Wu Zhou
2005-06-17 2:00 ` Daniel Jacobowitz
0 siblings, 1 reply; 5+ messages in thread
From: Wu Zhou @ 2005-05-29 6:55 UTC (permalink / raw)
To: Daniel Jacobowitz; +Cc: gdb-patches, eliz
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
2005-05-29 6:55 ` Wu Zhou
@ 2005-06-17 2:00 ` Daniel Jacobowitz
2005-06-17 4:38 ` Wu Zhou
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Jacobowitz @ 2005-06-17 2:00 UTC (permalink / raw)
To: Wu Zhou; +Cc: gdb-patches, eliz
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf
2005-06-17 2:00 ` Daniel Jacobowitz
@ 2005-06-17 4:38 ` Wu Zhou
0 siblings, 0 replies; 5+ messages in thread
From: Wu Zhou @ 2005-06-17 4:38 UTC (permalink / raw)
To: Daniel Jacobowitz; +Cc: gdb-patches, eliz
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2005-06-17 4:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2005-05-26 14:59 [RFC/Patch] replace sprintf calls in remote.c with xsnprintf Wu Zhou
2005-05-28 18:53 ` Daniel Jacobowitz
2005-05-29 6:55 ` Wu Zhou
2005-06-17 2:00 ` Daniel Jacobowitz
2005-06-17 4:38 ` Wu Zhou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox