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: Svante Signell <svante dot signell at gmail dot com>
- To: Joel Brobecker <brobecker at adacore 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 14:49:15 +0200
- 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> <20130529094242 dot GE5751 at adacore dot com>
- Reply-to: svante dot signell at gmail dot 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)
>
>