From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14726 invoked by alias); 29 May 2013 12:49:06 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 14716 invoked by uid 89); 29 May 2013 12:49:05 -0000 X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,SPF_PASS,TW_XS autolearn=ham version=3.3.1 Received: from mail-lb0-f182.google.com (HELO mail-lb0-f182.google.com) (209.85.217.182) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 29 May 2013 12:49:05 +0000 Received: by mail-lb0-f182.google.com with SMTP id z5so8814825lbh.41 for ; Wed, 29 May 2013 05:49:02 -0700 (PDT) X-Received: by 10.112.18.164 with SMTP id x4mr1479216lbd.127.1369831742494; Wed, 29 May 2013 05:49:02 -0700 (PDT) Received: from [192.168.1.12] (178-78-231-178.customers.ownit.se. [178.78.231.178]) by mx.google.com with ESMTPSA id c7sm3786840lag.3.2013.05.29.05.49.01 for (version=SSLv3 cipher=RC4-SHA bits=128/128); Wed, 29 May 2013 05:49:01 -0700 (PDT) Message-ID: <1369831755.30815.10.camel@G3620.my.own.domain> Subject: Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd From: Svante Signell Reply-To: svante.signell@gmail.com To: Joel Brobecker Cc: Sergio Durigan Junior , Pedro Alves , gdb-patches@sourceware.org Date: Wed, 29 May 2013 12:49:00 -0000 In-Reply-To: <20130529094242.GE5751@adacore.com> References: <519F2A7A.4050002@redhat.com> <1369386446.8127.51.camel@s1499.it.kth.se> <1369654913.8127.84.camel@s1499.it.kth.se> <20130527121028.GB5751@adacore.com> <1369657278.8127.90.camel@s1499.it.kth.se> <1369662500.8127.97.camel@s1499.it.kth.se> <1369663027.8127.99.camel@s1499.it.kth.se> <20130527143053.GC5751@adacore.com> <1369670598.8127.118.camel@s1499.it.kth.se> <20130529094242.GE5751@adacore.com> Content-Type: text/plain; charset="ISO-8859-15" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00995.txt.bz2 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 > > > > * 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) > >