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

Joel Brobecker
2013-05-29T09:42:53Z

> 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