Re: [PATCH v2] Improve gdb_tilde_expand logic

[Sergio Durigan Junior via Gdb-patches <gdb-patches@sourceware.org>]

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 <http://www.gnu.org/licenses/>.  */
> +
> +#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 <http://www.gnu.org/licenses/>.  */
>  
> +#include <algorithm>

"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 <glob.h>
>  
> @@ -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.

> +
> +      // 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.  */

> +
> +      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<char>
>  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/