[RFA-v2] ARI fixes: Remove some sprintf calls

[Pierre Muller]

> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Joel Brobecker
> Envoyé : lundi 12 novembre 2012 17:08
> À : Pierre Muller
> Cc : 'Tom Tromey'; 'Andreas Schwab'; gdb-patches@sourceware.org
> Objet : Re: [RFA-v2] ARI fixes: Remove some sprintf calls
> 
> > 2012-11-11  Pierre Muller  <muller@sourceware.org>
> >
> >         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.
  Thanks.
 
> 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?

  As long as we can get also compilers that don't know about #poison
to also generate an error if sprint is used, everything is fine.

 
> A few nits... I think it's good to go, after the trivial nits are
> corrected.

  I committed the patch after the modification you
suggested below.
 
> 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.
  Done. 
> > +  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:

  I hope to correctly understand that you mean
that this is for multi-line expressions only...

 
>   arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
>               + strlen (solib));
 Done. 
> 
> >        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?

  I didn't add them if they were not present...
There are of course lots of other places were those are missing,
but you are right that we should correct those error
if we modify the corresponding 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/<pid>/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?
Done. 
> >  #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?

  Final committed patch is attached.
ChangeLog is unchanged.


Thanks for the approval,

Pierre Muller
as ARI maintainer
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sprintf-v2.patch
Type: application/octet-stream
Size: 12218 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20121113/5cee0ce3/attachment.obj>