This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Svante Signell <svante dot signell at gmail dot com>
- Cc: Sergio Durigan Junior <sergiodj at redhat dot com>, Pedro Alves <palves at redhat dot com>, gdb-patches at sourceware dot org
- Date: Wed, 29 May 2013 13:42:42 +0400
- Subject: Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd
- References: <519F2A7A dot 4050002 at redhat dot com> <1369386446 dot 8127 dot 51 dot camel at s1499 dot it dot kth dot se> <m3hahslp0a dot fsf at redhat dot com> <1369654913 dot 8127 dot 84 dot camel at s1499 dot it dot kth dot se> <20130527121028 dot GB5751 at adacore dot com> <1369657278 dot 8127 dot 90 dot camel at s1499 dot it dot kth dot se> <1369662500 dot 8127 dot 97 dot camel at s1499 dot it dot kth dot se> <1369663027 dot 8127 dot 99 dot camel at s1499 dot it dot kth dot se> <20130527143053 dot GC5751 at adacore dot com> <1369670598 dot 8127 dot 118 dot camel at s1499 dot it dot kth dot se>
> 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