Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd

[Svante Signell <svante.signell@gmail.com>]

On Wed, 2013-05-29 at 13:42 +0400, Joel Brobecker wrote:
> > Attaching a patch and Chengelog entry for the function
> > nto_find_and_open_solib preceding nto_init_solib_absolute_prefix. There
> > I did not see any use of the make_cleanup/do_cleanups functions
> > (assuming open and openp always return). Therefore xfree is used
> > directly.
> 
> In this case, I would KISS, and go with cleanups.  Otherwise,
> you have to remember when you can free. With cleanups, you just
> mark it right after allocation and do_cleanups before returning.
> And we are putting more and more tools in place to make sure we
> do not forget to do the cleanups...

OK, I will use the *_cleanup(s) functions instead.

> > 2013-05-27  Svante Signell  <svante.signell@gmail.com>
> > 
> > 	* nto-dep.c (nto_find_and_open_solib): Use xstrprintf followed by
> > 	a cleanup instead of alloca to be consistent with function
> > 	(nto_init_solib_absolute_prefix).
> 
> My comments below...
> 
> > --- a/gdb/nto-tdep.c	2013-05-27 15:58:07.000000000 +0200
> > +++ b/gdb/nto-tdep.c	2013-05-27 16:23:50.000000000 +0200
> > @@ -89,9 +89,7 @@ nto_find_and_open_solib (char *solib, un
> >    char *buf, *arch_path, *nto_root, *endian;
> >    const char *base;
> >    const char *arch;
> > -  int arch_len, len, ret;
> > -#define PATH_FMT \
> > -  "%s/lib:%s/usr/lib:%s/usr/photon/lib:%s/usr/photon/dll:%s/lib/dll"
> > +  int ret;
> >  
> >    nto_root = nto_target ();
> >    if (strcmp (gdbarch_bfd_arch_info (target_gdbarch ())->arch_name, "i386") == 0)
> > @@ -117,22 +115,20 @@ nto_find_and_open_solib (char *solib, un
> >    /* In case nto_root is short, add strlen(solib)
> >       so we can reuse arch_path below.  */
> >  
> > -  arch_len = (strlen (nto_root) + strlen (arch) + strlen (endian) + 2
> > -	      + strlen (solib));
> > -  arch_path = alloca (arch_len);
> > -  xsnprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
> > +  arch_path = xstrprintf (arch_path, arch_len, "%s/%s%s", nto_root, arch, endian);
> 
> You forgot to remove "arch_path, arch_len" from the arguments of
> the call above. As a result, i suspect that this patch does not
> compile, and that's the minimum we need to do...

I have updated my patch and it built. I copied the patch to a directory,
attached it to the mail, and checked by opening the attachment with
gedit. Then updated the patch to that directory and rechecked with gedit
again and the updated patch was there (not deleting the old attachment)
Still the old version was attached :( Maybe an evolution issue.

> >  
> > -  len = strlen (PATH_FMT) + strlen (arch_path) * 5 + 1;
> > -  buf = alloca (len);
> > -  xsnprintf (buf, len, PATH_FMT, arch_path, arch_path, arch_path, arch_path,
> > +  buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib:\"%s\"/usr/photon/dll:\"%s\"/lib/dll", arch_path, arch_path, arch_path, arch_path,
> >  	     arch_path);
> 
> Same as in the other patch in the same file: Your patch is adding
> double-quoting of the arg_path, which we may want if you have
> a compelling argument, but should be discussed independently of
> this patch.

OK, an updated patch will remove these quotes.

> I do not know of any absolute guideline whether to split the format
> string or not, but I probably would, like so:
> 
>   buf = xstrprintf ("\"%s\"/lib:\"%s\"/usr/lib:\"%s\"/usr/photon/lib"
>                       ":\"%s\"/usr/photon/dll:\"%s\"/lib/dll",
>                     arch_path, arch_path, arch_path, arch_path,
>                     arch_path);
> 
> Be sure, also, to try to restrict your line length to 70 characters if
> you can. That's our soft limit, with 80 being the absolute limit (under
> normal circumstances).

OK, will do.

> > +  xfree (arch_path);
> >  
> >    base = lbasename (solib);
> >    ret = openp (buf, 1, base, o_flags, temp_pathname);
> > +  xfree (buf);
> >    if (ret < 0 && base != solib)
> >      {
> > -      xsnprintf (arch_path, arch_len, "/%s", solib);
> > +      arch_path = xstrprintf ("/%s", solib);
> >        ret = open (arch_path, o_flags, 0);
> > +      xfree (arch_path);
> >        if (temp_pathname)
> >  	{
> >  	  if (ret >= 0)
> 
>