From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29117 invoked by alias); 29 May 2013 09:42:53 -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 29084 invoked by uid 89); 29 May 2013 09:42:52 -0000 X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RCVD_IN_HOSTKARMA_NO,TW_XS autolearn=ham version=3.3.1 Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 29 May 2013 09:42:52 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id AA03E2EDA0; Wed, 29 May 2013 05:42:50 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id ILAHhL+g8Z3W; Wed, 29 May 2013 05:42:50 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 005E42ED9F; Wed, 29 May 2013 05:42:49 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id B20EDC278A; Wed, 29 May 2013 13:42:42 +0400 (RET) Date: Wed, 29 May 2013 09:42:00 -0000 From: Joel Brobecker To: Svante Signell Cc: Sergio Durigan Junior , Pedro Alves , gdb-patches@sourceware.org Subject: Re: New patch: Re: Small patch to enable build of gdb-7.6 for GNU/Hurd Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1369670598.8127.118.camel@s1499.it.kth.se> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2013-05/txt/msg00986.txt.bz2 > 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 > > * 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