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

[Joel Brobecker <brobecker@adacore.com>]

> 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...

> 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...

>  
> -  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.

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).

> +  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)


-- 
Joel