From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61014 invoked by alias); 11 Feb 2018 22:14:04 -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 59554 invoked by uid 89); 11 Feb 2018 22:14:03 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 11 Feb 2018 22:14:01 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 8EE931E520; Sun, 11 Feb 2018 17:13:59 -0500 (EST) Subject: Re: [PATCH 1/2] Create new common/pathstuff.[ch] To: Sergio Durigan Junior , GDB Patches Cc: Simon Marchi References: <20180210014241.19278-1-sergiodj@redhat.com> <20180210014241.19278-2-sergiodj@redhat.com> From: Simon Marchi Message-ID: <084a12bf-6b54-9399-e4fe-887952a57bc1@simark.ca> Date: Sun, 11 Feb 2018 22:14:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180210014241.19278-2-sergiodj@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-SW-Source: 2018-02/txt/msg00164.txt.bz2 On 2018-02-09 08:42 PM, Sergio Durigan Junior wrote: > This commit moves the path manipulation routines found on utils.c to a > new common/pathstuff.c, and updates the Makefile.in's accordingly. > The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and > "gdb_abspath". > > This will be needed because gdbserver will have to call "gdb_abspath" > on my next patch, which implements a way to expand the path of the > inferior provided by the user in order to allow specifying just the > binary name when starting gdbserver, like: > > $ gdbserver :1234 a.out > > With the recent addition of the startup-with-shell feature on > gdbserver, this scenario doesn't work anymore if the user doesn't have > the current directory listed in the PATH variable. > > I had to do a minor adjustment on "gdb_abspath" because we don't have > access to "tilde_expand" on gdbserver, so now the function is using > "gdb_tilde_expand" instead. Otherwise, the code is the same. > > Regression tested on the BuildBot, without regressions. Hi Sergio, Thanks for looking into this! This commit does not build: /home/simark/src/binutils-gdb/gdb/common/pathstuff.c: In function ‘gdb::unique_xmalloc_ptr gdb_abspath(const char*)’: /home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: error: ‘current_directory’ was not declared in this scope (concat (current_directory, ^~~~~~~~~~~~~~~~~ /home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: note: suggested alternative: ‘read_direction’ (concat (current_directory, ^~~~~~~~~~~~~~~~~ read_direction I guess you need to move the declaration to common-defs.h in this commit instead of the next one. I also got this whitespace error from git am: .git/rebase-apply/patch:131: trailing whitespace. /* Extract the basename of filename, and return immediately I think it's in code you copied, but if you can remove the extra space while at it it would be nice. > +/* See common/pathstuff.h. */ > + > +gdb::unique_xmalloc_ptr > +gdb_abspath (const char *path) > +{ > + gdb_assert (path != NULL && path[0] != '\0'); > + > + if (path[0] == '~') > + { > + std::string new_path = gdb_tilde_expand (path); > + > + return gdb::unique_xmalloc_ptr (xstrdup (new_path.c_str ())); We should try to avoid unnecessary copies, when possible. Here, we could either make another version of gdb_tilde_expand (it would have to be another name) that returns a gdb::unique_xmalloc_ptr or make gdb_abspath return an std::string. I think the former would be better for now because some callers require a gdb::unique_xmalloc_ptr, and would have to do a copy themselves. So using a gdb::unique_xmalloc_ptr across the whole chain would give the least amount of copies. > diff --git a/gdb/utils.h b/gdb/utils.h > index b234762929..4f25be0968 100644 > --- a/gdb/utils.h > +++ b/gdb/utils.h > @@ -23,6 +23,7 @@ > > #include "exceptions.h" > #include "common/scoped_restore.h" > +#include "common/pathstuff.h" I don't think utils.h should be including common/pathstuff.h, because it is not using it. I understand why you did this (this ensures that every current user of these functions will automatically see the new declaration), but in the long term I think it's better if the users include "pathstuff.h". The files that use these functions are: $ grep -e gdb_realpath_keepfile -e gdb_realpath -e gdb_abspath *.c */*.c -l | sort auto-load.c common/pathstuff.c compile/compile.c dwarf2read.c exec.c guile/scm-safe-call.c linux-thread-db.c main.c nto-tdep.c objfiles.c source.c symtab.c utils.c They are all files included in a --enable-targets=all build, so it should be easy to find where #include "common/pathstuff.h" is missing. Thanks, Simon