From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18944 invoked by alias); 29 May 2005 06:11:52 -0000 Mailing-List: contact gdb-patches-help@sources.redhat.com; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sources.redhat.com Received: (qmail 18923 invoked by uid 22791); 29 May 2005 06:11:47 -0000 Received: from ausmtp01.au.ibm.com (HELO ausmtp01.au.ibm.com) (202.81.18.186) by sourceware.org (qpsmtpd/0.30-dev) with ESMTP; Sun, 29 May 2005 06:11:47 +0000 Received: from sd0208e0.au.ibm.com (d23rh904.au.ibm.com [202.81.18.202]) by ausmtp01.au.ibm.com (8.12.10/8.12.10) with ESMTP id j4T6ECa7437048 for ; Sun, 29 May 2005 16:14:13 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.250.243]) by sd0208e0.au.ibm.com (8.12.10/NCO/VER6.6) with ESMTP id j4T6ETCZ142636 for ; Sun, 29 May 2005 16:14:35 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.12.11/8.13.3) with ESMTP id j4T6Bbm2003433 for ; Sun, 29 May 2005 16:11:37 +1000 Received: from plinuxt18.cn.ibm.com (plinuxt18.cn.ibm.com [9.181.140.28]) by d23av02.au.ibm.com (8.12.11/8.12.11) with ESMTP id j4T6BZ2H003391; Sun, 29 May 2005 16:11:35 +1000 Date: Sun, 29 May 2005 06:55:00 -0000 From: Wu Zhou To: Daniel Jacobowitz cc: gdb-patches@sources.redhat.com, eliz@gnu.org Subject: Re: [RFC/Patch] replace sprintf calls in remote.c with xsnprintf In-Reply-To: <20050528183924.GE26806@nevyn.them.org> Message-ID: References: <20050528183924.GE26806@nevyn.them.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2005-05/txt/msg00610.txt.bz2 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 * 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