From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id nAy1C6qa+18JBQAAWB0awg (envelope-from ) for ; Sun, 10 Jan 2021 19:24:10 -0500 Received: by simark.ca (Postfix, from userid 112) id 233F21EE85; Sun, 10 Jan 2021 19:24:10 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 9116E1EE1B for ; Sun, 10 Jan 2021 19:24:09 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id CF65F3851C07; Mon, 11 Jan 2021 00:24:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org CF65F3851C07 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1610324648; bh=GtcKx3CLbfkxlnt4HohRr6zYiqRR6mJTpfDo0ip9qow=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=G1dWCz1P/+2qLpoMpOUgizHf0ND8/BTKCAm5OSV9EbPxRREmgScB8F7gu1Pt9VvAk wbm7CAWFmcZLpc1HQxpSqaSeT8QFhV4VGxnhBQ1Kvq++LMNwyFRNwxKx8l0kKSvzzA 22xjo6IpFGavN73/0Fb3tNwDF26IA/QdtMb5N/tY= Received: from beryx.lancelotsix.com (beryx.lancelotsix.com [IPv6:2001:41d0:401:3000::1ab3]) by sourceware.org (Postfix) with ESMTPS id DADE7385DC14 for ; Mon, 11 Jan 2021 00:24:05 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DADE7385DC14 Received: from gwenhwyvar (unknown [IPv6:2a02:390:8443:0:89c4:5cee:c803:8f4]) by beryx.lancelotsix.com (Postfix) with ESMTPSA id 2A51A2E070; Mon, 11 Jan 2021 01:24:04 +0100 (CET) Date: Mon, 11 Jan 2021 00:23:51 +0000 To: Sergio Durigan Junior Subject: Re: [PATCH v2] Improve gdb_tilde_expand logic Message-ID: <20210111002351.GA6684@gwenhwyvar> References: <20210108001337.29164-1-lsix@lancelotsix.com> <20210109182906.24570-1-lsix@lancelotsix.com> <87y2h0peek.fsf@paluero> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87y2h0peek.fsf@paluero> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (beryx.lancelotsix.com [0.0.0.0]); Mon, 11 Jan 2021 01:24:04 +0100 (CET) X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Lancelot SIX via Gdb-patches Reply-To: Lancelot SIX Cc: Lancelot SIX via Gdb-patches Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" On Sun, Jan 10, 2021 at 06:20:51PM -0500, Sergio Durigan Junior wrote: > On Saturday, January 09 2021, Lancelot SIX via Gdb-patches wrote: > > > Before this patch, gdb_tilde_expand would use glob(3) in order to expand > > tilde at the begining of a path. This implementation has limitation when > > expanding a tilde leading path to a non existing file since glob fails to > > expand. > > > > This patch proposes to use glob only to expand the tilde component of the > > path and leaves the rest of the path unchanged. > > > > This patch is a followup to the following discution: > > https://sourceware.org/pipermail/gdb-patches/2021-January/174776.html > > > > Before the patch: > > > > gdb_tilde_expand("~") -> "/home/lsix" > > gdb_tilde_expand("~/a/c/b") -> error() is called > > > > After the patch: > > > > gdb_tilde_expand("~") -> "/home/lsix" > > gdb_tilde_expand("~/a/c/b") -> "/home/lsix/a/c/b" > > > > Tested on x84_64 linux. > > > > New since V1: > > * Unittests added thanks to inputs and skeleton provided by > > Simon Marchi. > > * My copyright assignment has been signed! > > > > gdb/ChangeLog: > > > > * Makefile.in (SELFTESTS_SRCS): Add > > unittests/gdb_tilde_expand-selftests.c. > > * unittests/gdb_tilde_expand-selftests.c: New file. > > > > gdbsupport/ChangeLog: > > > > * gdb_tilde_expand.cc (gdb_tilde_expand): Improve > > implementation. > > (gdb_tilde_expand_up): Delegate logic to gdb_tilde_expand. > > * gdb_tilde_expand.h (gdb_tilde_expand): Update description. > > --- > > gdb/Makefile.in | 1 + > > gdb/unittests/gdb_tilde_expand-selftests.c | 62 ++++++++++++++++++++++ > > gdbsupport/gdb_tilde_expand.cc | 45 +++++++++++----- > > gdbsupport/gdb_tilde_expand.h | 3 +- > > 4 files changed, 97 insertions(+), 14 deletions(-) > > create mode 100644 gdb/unittests/gdb_tilde_expand-selftests.c > > > > diff --git a/gdb/Makefile.in b/gdb/Makefile.in > > index 9267bea7be..c8979fe276 100644 > > --- a/gdb/Makefile.in > > +++ b/gdb/Makefile.in > > @@ -446,6 +446,7 @@ SELFTESTS_SRCS = \ > > unittests/filtered_iterator-selftests.c \ > > unittests/format_pieces-selftests.c \ > > unittests/function-view-selftests.c \ > > + unittests/gdb_tilde_expand-selftests.c \ > > unittests/gmp-utils-selftests.c \ > > unittests/lookup_name_info-selftests.c \ > > unittests/memory-map-selftests.c \ > > diff --git a/gdb/unittests/gdb_tilde_expand-selftests.c b/gdb/unittests/gdb_tilde_expand-selftests.c > > new file mode 100644 > > index 0000000000..464e63b2e4 > > --- /dev/null > > +++ b/gdb/unittests/gdb_tilde_expand-selftests.c > > @@ -0,0 +1,62 @@ > > +/* Self tests for gdb_tilde_expand > > + > > + Copyright (C) 2021 Free Software Foundation, Inc. > > + > > + This file is part of GDB. > > + > > + This program is free software; you can redistribute it and/or modify > > + it under the terms of the GNU General Public License as published by > > + the Free Software Foundation; either version 3 of the License, or > > + (at your option) any later version. > > + > > + This program is distributed in the hope that it will be useful, > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + GNU General Public License for more details. > > + > > + You should have received a copy of the GNU General Public License > > + along with this program. If not, see . */ > > + > > +#include "gdbsupport/common-defs.h" > > +#include "filenames.h" > > +#include "gdbsupport/selftest.h" > > + > > +#include "gdbsupport/gdb_tilde_expand.h" > > + > > +namespace selftests { > > +namespace gdb_tilde_expand_tests { > > + > > +static void > > +do_test () > > +{ > > + /* Home directory of the user running the test */ > > Period at the end of the sentence. > > > + const std::string home = gdb_tilde_expand ("~"); > > I think it doesn't make much sense to rely on gdb_tilde_expand to test > gdb_tilde_expand. I think it's better to use $HOME, and just don't run > the test if it's not defined. > > Or (thinking aloud here) you could call glob(3) directly here. > > > + // The result should be a absolute path > > + SELF_CHECK (IS_ABSOLUTE_PATH (home)); > > IIRC // comments are not used on GDB. > > > + > > + /* When given a path that beging by a tilde and refers to a file that > > s/beging/begins/ > > > + does not exist, gdb_tilde expand must still be able to do the tilde > > + expansion. */ > > Two spaces after period (here and everywhere else). > > > + SELF_CHECK > > + (gdb_tilde_expand ("~/non/existent/directory") == > > + home + "/non/existent/directory"); > > + > > + /* gdb_tilde_expand only expands tilde and does not try to do any other > > + substitution. */ > > + SELF_CHECK (gdb_tilde_expand ("~/*/a.out") == home + "/*/a.out"); > > + > > + /* gdb_tilde_expand does no modification to a non tilde leading path. */ > > + SELF_CHECK (gdb_tilde_expand ("/some/abs/path") == "/some/abs/path"); > > + SELF_CHECK (gdb_tilde_expand("relative/path") == "relative/path"); > > Space before open parenthesis. > > > +} > > + > > +} /* namespace gdb_tilde_expand_tests */ > > I was going to say that I think this function should include tests for > the "~user/path" scenario, but I don't know offhand how you could do > that. The challenge here would be knowing the user. Maybe you can > write something that relies on $USER being defined, not sure. > > > +} /* namespace selftests */ > > + > > +void _initialize_gdb_tilde_expand_selftests (); > > +void > > +_initialize_gdb_tilde_expand_selftests () > > +{ > > + selftests::register_test > > + ("gdb_tilde_expand", selftests::gdb_tilde_expand_tests::do_test); > > +} > > diff --git a/gdbsupport/gdb_tilde_expand.cc b/gdbsupport/gdb_tilde_expand.cc > > index b31fc48446..d060e26f50 100644 > > --- a/gdbsupport/gdb_tilde_expand.cc > > +++ b/gdbsupport/gdb_tilde_expand.cc > > @@ -17,7 +17,9 @@ > > You should have received a copy of the GNU General Public License > > along with this program. If not, see . */ > > > > +#include > > "common-defs.h" should always be the first include in a file. > > > #include "common-defs.h" > > +#include "filenames.h" > > #include "gdb_tilde_expand.h" > > #include > > > > @@ -71,14 +73,37 @@ private: > > std::string > > gdb_tilde_expand (const char *dir) > > { > > - gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL); > > + if (dir[0] == '~') > > + { > > + /* This function uses glob in order to expand the ~. However, this > > + function will fail to expand if the actual dir we are looking > > + for does not exist. Given "~/does/not/exist", glob will fail. > > The two lines above are indented differently (the first one is using > only spaces, the second one is using TAB). > > > > > - gdb_assert (glob.pathc () > 0); > > - /* "glob" may return more than one match to the path provided by the > > - user, but we are only interested in the first match. */ > > - std::string expanded_dir = glob.pathv ()[0]; > > + In order to avoid such limitation, we only use > > + glob to expand "~" and keep "/does/not/exist" unchanged. > > > > - return expanded_dir; > > + Similarly, to expand ~gdb/might/not/exist, we only expand > > + "~gdb" using glob and leave "/might/not/exist" unchanged. */ > > + > > + const std::string d (dir); > > + > > + // Look for the first dir separator (if any) > > + const auto first_sep = > > The equal sign should be at the beginning of the next line. However, I > don't think you really need to break the line here. > > > + std::find_if (d.cbegin (), d.cend(), > > + [](const auto c) { return IS_DIR_SEPARATOR (c); }); > > Matter of personal taste, but I'm really not a fan of the "use auto > everywhere" idiom here. I do not mind that. I’ll just keep auto for the iterator type and remove it everywhere else. > > > + > > + // split d according to the first separator > > + const std::string to_expand (d.cbegin (), first_sep); > > + const std::string remainder (first_sep, d.cend ()); > > + > > + // Expand the first element > > + const gdb_glob glob (to_expand.c_str (), GLOB_TILDE_CHECK, nullptr); > > + gdb_assert (glob.pathc () == 1); > > You can move the comment you deleted earlier to here: > > /* "glob" may return more than one match to the path provided by the > user, but we are only interested in the first match. */ > Actually, since I only use glob for the first element of the path (until the first directory separator), I can have "~" or "~username" to expand and I do see how I can have multiple possible expansions for that. This is why the assert has been changed from '> 0' to '== 1'. Did I miss a corner case where I can expand to multiple options? I’ll also have the unittest use getenv and check for HOME and USER. If those env variable are not available, I’ll just skip sections of the test. Thanks for the review, I’ll post an updated version of the patch shortly. And sorry for the styling issues, the GNU guidelines are quite different from what I am used to so I tend to miss points easily. Lancelot. > > + > > + return std::string (glob.pathv ()[0]) + remainder; > > + } > > + else > > + return std::string (dir); > > You can get rid of the new indentation level you're adding by inverting > the check at the beginning of the function: > > if (dir[0] != '~') > return std::string (dir); > > ... > > > } > > > > /* See gdbsupport/gdb_tilde_expand.h. */ > > @@ -86,10 +111,6 @@ gdb_tilde_expand (const char *dir) > > gdb::unique_xmalloc_ptr > > gdb_tilde_expand_up (const char *dir) > > { > > - gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL); > > - > > - gdb_assert (glob.pathc () > 0); > > - /* "glob" may return more than one match to the path provided by the > > - user, but we are only interested in the first match. */ > > - return make_unique_xstrdup (glob.pathv ()[0]); > > + auto const expanded = gdb_tilde_expand (dir); > > Same comment here about "auto everywhere". > > > + return make_unique_xstrdup (expanded.c_str ()); > > } > > diff --git a/gdbsupport/gdb_tilde_expand.h b/gdbsupport/gdb_tilde_expand.h > > index e2d85cadad..a61f246329 100644 > > --- a/gdbsupport/gdb_tilde_expand.h > > +++ b/gdbsupport/gdb_tilde_expand.h > > @@ -20,8 +20,7 @@ > > #ifndef COMMON_GDB_TILDE_EXPAND_H > > #define COMMON_GDB_TILDE_EXPAND_H > > > > -/* Perform path expansion (i.e., tilde expansion) on DIR, and return > > - the full path. */ > > +/* Perform tilde expansion on DIR, and return the full path. */ > > extern std::string gdb_tilde_expand (const char *dir); > > > > /* Same as GDB_TILDE_EXPAND, but return the full path as a > > -- > > > > 2.29.2 > > > > > > -- > Sergio > GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 > Please send encrypted e-mail if possible > https://sergiodj.net/