From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1385 invoked by alias); 12 Nov 2012 16:08:10 -0000 Received: (qmail 1331 invoked by uid 22791); 12 Nov 2012 16:08:06 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_XS X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 12 Nov 2012 16:07:58 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id ED7E71C7F72; Mon, 12 Nov 2012 11:07:57 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 65fibHMFaGad; Mon, 12 Nov 2012 11:07:57 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 7493E1C7F7B; Mon, 12 Nov 2012 11:07:57 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id D49C8C49E9; Mon, 12 Nov 2012 08:07:41 -0800 (PST) Date: Mon, 12 Nov 2012 16:08:00 -0000 From: Joel Brobecker To: Pierre Muller Cc: 'Tom Tromey' , 'Andreas Schwab' , gdb-patches@sourceware.org Subject: Re: [RFA-v2] ARI fixes: Remove some sprintf calls Message-ID: <20121112160741.GT5103@adacore.com> References: <4297.05661158568$1352387430@news.gmane.org> <87d2znhj7t.fsf@fleche.redhat.com> <000001cdc061$dad58740$908095c0$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000001cdc061$dad58740$908095c0$@muller@ics-cnrs.unistra.fr> User-Agent: Mutt/1.5.21 (2010-09-15) Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-11/txt/msg00286.txt.bz2 > 2012-11-11 Pierre Muller > > ARI fixes: Avoid sprintf function use rule. > * charset.c (convert_between_encodings): Use xsnprintf. > * cli-out.c (cli_field_int): Likewise. > * cp-namespace.c (cp_lookup_nested_symbol): Likewise. > * expprint.c (op_name_standard): Likewise. > * frv-tdep.c (set_variant_num_gprs): Likewise. > (set_variant_num_fprs): Likewise. > * m68hc11-tdep.c (m68hc11_initialize_register_info): Likewise. > * nto-tdep.c (nto_find_and_open_solib): Likewise. > (nto_init_solib_absolute_prefix): Likewise. > * source.c (init_source_path): Likewise. > (print_source_lines_base): Likewise. > * valprint.c (print_wchar): Likewise. > * mi/mi-out.c (mi_field_int): Likewise. > windows-nat.c (windows_pid_to_exec_file): Likewise. > (windows_create_inferior): Likewise. > (_initialize_check_for_gdb_ini): Likewise. This is a nice improvement. I'm wondering if there is any way to provoke an error if we ever use sprintf again... I am asking because I know that it's easy to ignore the ARI. I kind of remember not being able to do that, but perhaps wrong memory. #poison, maybe? A few nits... I think it's good to go, after the trivial nits are corrected. Thanks for doing this! > Index: nto-tdep.c > =================================================================== > RCS file: /cvs/src/src/gdb/nto-tdep.c,v > retrieving revision 1.46 > diff -u -p -r1.46 nto-tdep.c > --- nto-tdep.c 9 Nov 2012 19:58:00 -0000 1.46 > +++ nto-tdep.c 11 Nov 2012 22:42:47 -0000 > @@ -89,7 +89,7 @@ nto_find_and_open_solib (char *solib, un > char *buf, *arch_path, *nto_root, *endian; > const char *base; > const char *arch; > - int ret; > + int archlen, len, ret; Can we use "arch_len" instead if "archlen". This would be more in line with "arch_path", and since both are related... It's also more in line with the GNU Coding Style, I believe, where we use underscores to separate words in identifier names. > + archlen = strlen (nto_root) + strlen (arch) + strlen (endian) + 2 > + + strlen (solib); Another tiny nit. GCS require that we put the RHS expression inside parentheses (to help tools format it correctly). Thus: arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2 + strlen (solib)); > if (!noerror) > { > - char *name = alloca (strlen (s->filename) + 100); > - sprintf (name, "%d\t%s", line, s->filename); > + int len = strlen (s->filename) + 100; > + char *name = alloca (len); > + xsnprintf (name, len, "%d\t%s", line, s->filename); Can you add an empty line between variable defs and the rest of the code? > +++ windows-nat.c 11 Nov 2012 22:51:49 -0000 > @@ -1895,7 +1895,7 @@ windows_pid_to_exec_file (int pid) > /* Try to find exe name as symlink target of /proc//exe. */ > int nchars; > char procexe[sizeof ("/proc/4294967295/exe")]; > - sprintf (procexe, "/proc/%u/exe", pid); > + xsnprintf (procexe, sizeof (procexe), "/proc/%u/exe", pid); Same here, please? > #else > - cygallargs = (char *) > - alloca (sizeof (" -c 'exec '") + strlen (exec_file) > - + strlen (allargs) + 2); > - sprintf (cygallargs, " -c 'exec %s %s'", exec_file, allargs); > + len = sizeof (" -c 'exec '") + strlen (exec_file) > + + strlen (allargs) + 2; Can you enclose the RSH expression between parentheses? -- Joel