[PATCH] Improve gdb_tilde_expand logic

[PATCH] Improve gdb_tilde_expand logic

[Lancelot SIX]

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 leave 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:

	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. I am unsure where to place unittests for this
function.

My copyright assignment is still pending, but should be signed soon.

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.
---
 gdbsupport/gdb_tilde_expand.cc | 45 +++++++++++++++++++++++++---------
 gdbsupport/gdb_tilde_expand.h  |  3 +--
 2 files changed, 34 insertions(+), 14 deletions(-)

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>
 #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.
 
-  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 =
+	std::find_if (d.cbegin (), d.cend(),
+		      [](const auto c) { return IS_DIR_SEPARATOR (c); });
+
+      // 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);
+
+      return std::string (glob.pathv ()[0]) + remainder;
+    }
+  else
+    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);
+  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

Re: [PATCH] Improve gdb_tilde_expand logic

[Andrew Burgess]

* Lancelot SIX via Gdb-patches <gdb-patches@sourceware.org> [2021-01-08 00:13:37 +0000]:

> 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 leave 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:
> 
> 	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. I am unsure where to place unittests for this
> function.

There's only very limited unit testing within GDB.  You should write a
test in gdb/testsuite/.... that triggers this behaviour and check the
functionality that way.

Thanks,
Andrew


> 
> My copyright assignment is still pending, but should be signed soon.
> 
> 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.
> ---
>  gdbsupport/gdb_tilde_expand.cc | 45 +++++++++++++++++++++++++---------
>  gdbsupport/gdb_tilde_expand.h  |  3 +--
>  2 files changed, 34 insertions(+), 14 deletions(-)
> 
> 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>
>  #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.
>  
> -  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 =
> +	std::find_if (d.cbegin (), d.cend(),
> +		      [](const auto c) { return IS_DIR_SEPARATOR (c); });
> +
> +      // 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);
> +
> +      return std::string (glob.pathv ()[0]) + remainder;
> +    }
> +  else
> +    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);
> +  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
> 

Re: [PATCH] Improve gdb_tilde_expand logic

[Simon Marchi]

On 2021-01-08 4:30 a.m., Andrew Burgess wrote:
> There's only very limited unit testing within GDB.  You should write a
> test in gdb/testsuite/.... that triggers this behaviour and check the
> functionality that way.

It might be difficult to write a .exp test that specifically targets this
function... I think a selftest for this could be useful (in addition to
something in a .exp file, if this ends up used for the index-cache
directory, as discussed).

The selftest could just call gdb_tilde_expand with
"~/a/non/existent/directory" and make some simple assertions with the
result.  That selftest could probably be written directly in
gdbsupport/gdb_tilde_expand.cc.  That would be the first time a
selftest is put in a gdbsupport file, but I don't think that will
cause a problem.  That test will be available in both GDB and GDBserver.

You can check the test for gdb_realpath in gdb/utils.c if you want
an example.

Simon

Re: [PATCH] Improve gdb_tilde_expand logic

[Lancelot SIX]

On 08/01/2021 15:59, Simon Marchi wrote:
> On 2021-01-08 4:30 a.m., Andrew Burgess wrote:
>> There's only very limited unit testing within GDB.  You should write a
>> test in gdb/testsuite/.... that triggers this behaviour and check the
>> functionality that way.
> It might be difficult to write a .exp test that specifically targets this
> function... I think a selftest for this could be useful (in addition to
> something in a .exp file, if this ends up used for the index-cache
> directory, as discussed).
>
> The selftest could just call gdb_tilde_expand with
> "~/a/non/existent/directory" and make some simple assertions with the
> result.  That selftest could probably be written directly in
> gdbsupport/gdb_tilde_expand.cc.  That would be the first time a
> selftest is put in a gdbsupport file, but I don't think that will
> cause a problem.  That test will be available in both GDB and GDBserver.
>
> You can check the test for gdb_realpath in gdb/utils.c if you want
> an example.
>
> Simon

The option I have in mind is inspired by gdb.gdb/selftest.exp (if I 
understand it correctly): once I have a gdb session debugging gdb, I can 
have something like:

     gdb_test {print gdb_tilde_expand ("~/most/probably/non/existing").c_str ()} \
              {= .*"/.*/most/probably/non/existing"}

or something similar. If I go for something like that, can it be added 
directly in selftest.expr (under gdb.gdb) or do it require an entire new 
testcase (probably in gdb.base)?  It might be an overkill approach to 
unit-testing, but it should work.

Lancelot.

Re: [PATCH] Improve gdb_tilde_expand logic

[Simon Marchi]

On 2021-01-08 8:33 p.m., Lancelot SIX wrote:
> The option I have in mind is inspired by gdb.gdb/selftest.exp (if I understand it correctly): once I have a gdb session debugging gdb, I can have something like:
> 
>     gdb_test {print gdb_tilde_expand ("~/most/probably/non/existing").c_str ()} \
>              {= .*"/.*/most/probably/non/existing"}
> 
> or something similar. If I go for something like that, can it be added directly in selftest.expr (under gdb.gdb) or do it require an entire new testcase (probably in gdb.base)?  It might be an overkill approach to unit-testing, but it should work.

Well, we now have a "selftest" framework integrated in the GDB binary
itself, which makes it much more convenient to unit test functions.
It's also called selftest, but it's not related to gdb.gdb/selftest.exp.

I mislead you in my previous message when I said you could place the
test in gdbsupport.  Well, the test could be in gdbsupport, but there
needs to be something that registers the test, which is usually done
in an _initialize_* function in GDB.  So there are already some tests
for gdbsupport stuff placed in gdb/unittest.

You can use the patch below as a starting point.

You can run it like this:

(gdb) maintenance selftest tilde
Running selftest gdb_tilde_expand.
Self test failed: Could not find a match for '~/non/existent/directory'.
Ran 1 unit tests, 1 failed

With your fix, there shouldn't be an exception thrown.  And it would be
a good idea to add a SELF_CHECK (i.e. an assert) to check the result,
for example we expect it to contain "/non/existent/directory" somewhere.

Simon

From 65013bf368ca127ff51d96c6efb053209afc0ad5 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 8 Jan 2021 21:12:19 -0500
Subject: [PATCH] gdb: add selftest for gdb_tilde_expand

Change-Id: Icfcbc761a9b39c9b66634985a23b92e9a65ca761
---
 gdb/Makefile.in                            |  1 +
 gdb/unittests/gdb_tilde_expand-selftests.c | 44 ++++++++++++++++++++++
 2 files changed, 45 insertions(+)
 create mode 100644 gdb/unittests/gdb_tilde_expand-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 9267bea7beb6..c8979fe2760b 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 000000000000..c68ea99dac83
--- /dev/null
+++ b/gdb/unittests/gdb_tilde_expand-selftests.c
@@ -0,0 +1,44 @@
+/* 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 "gdbsupport/selftest.h"
+
+#include "gdbsupport/gdb_tilde_expand.h"
+
+namespace selftests {
+namespace gdb_tilde_expand_tests {
+
+static void
+do_test ()
+{
+  std::string ret = gdb_tilde_expand ("~");
+  ret = gdb_tilde_expand ("~/non/existent/directory");
+}
+
+} /* namespace gdb_tilde_expand_tests */
+} /* 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);
+}
-- 
2.29.2

[PATCH v2] Improve gdb_tilde_expand logic

[Lancelot SIX]

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 */
+  const std::string home = gdb_tilde_expand ("~");
+  // The result should be a absolute path
+  SELF_CHECK (IS_ABSOLUTE_PATH (home));
+
+  /* When given a path that beging by a tilde and refers to a file that
+     does not exist, gdb_tilde expand must still be able to do the tilde
+     expansion. */
+  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");
+}
+
+} /* namespace gdb_tilde_expand_tests */
+} /* 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>
 #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.
 
-  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 =
+	std::find_if (d.cbegin (), d.cend(),
+		      [](const auto c) { return IS_DIR_SEPARATOR (c); });
+
+      // 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);
+
+      return std::string (glob.pathv ()[0]) + remainder;
+    }
+  else
+    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);
+  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

Re: [PATCH v2] Improve gdb_tilde_expand logic

[Sergio Durigan Junior]

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/

Re: [PATCH v2] Improve gdb_tilde_expand logic

[Lancelot SIX]

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 <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.

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

[PATCH v3] Improve gdb_tilde_expand logic

[Lancelot SIX]

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.

Since V1:
	* Unittests added thanks to inputs and skeleton provided by
	Simon Marchi.
	* My copyright assignment has been signed!

Since V2:
	* Fix coding style issuess and typos.
	* Use $HOME and $USER env variables to test the behavior of
	gdb_tilde_expand instead of testing the function against itself.

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 | 78 ++++++++++++++++++++++
 gdbsupport/gdb_tilde_expand.cc             | 39 +++++++----
 gdbsupport/gdb_tilde_expand.h              |  3 +-
 4 files changed, 107 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..d64b7c6384
--- /dev/null
+++ b/gdb/unittests/gdb_tilde_expand-selftests.c
@@ -0,0 +1,78 @@
+/* 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 "gdbsupport/selftest.h"
+#include <cstdlib>
+
+#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.  */
+  const char *c_home = std::getenv ("HOME");
+
+  /* Skip the test if $HOME is not available in the environment.  */
+  if (c_home == nullptr)
+    return;
+
+  const std::string home (c_home);
+
+  /* Basic situation.  */
+  SELF_CHECK (home == gdb_tilde_expand ("~"));
+
+  /* When given a path that begins by a tilde and refers to a file that
+     does not exist, gdb_tilde expand must still be able to do the tilde
+     expansion.  */
+  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");
+
+  /* If $USER is available in the env variables, Check the '~user'
+     expansion, otherwise skip the rest of the test.  */
+  const char *c_user = std::getenv ("USER");
+  if (c_user == nullptr)
+    return;
+
+  const std::string user (c_user);
+  SELF_CHECK (gdb_tilde_expand (("~" + user).c_str ()) == home);
+  SELF_CHECK (gdb_tilde_expand (("~" + user + "/a/b").c_str ()) == home + "/a/b");
+}
+
+} /* namespace gdb_tilde_expand_tests */
+} /* 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..29224972aa 100644
--- a/gdbsupport/gdb_tilde_expand.cc
+++ b/gdbsupport/gdb_tilde_expand.cc
@@ -18,6 +18,8 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "common-defs.h"
+#include <algorithm>
+#include "filenames.h"
 #include "gdb_tilde_expand.h"
 #include <glob.h>
 
@@ -71,14 +73,31 @@ private:
 std::string
 gdb_tilde_expand (const char *dir)
 {
-  gdb_glob glob (dir, GLOB_TILDE_CHECK, NULL);
+  if (dir[0] != '~')
+    return std::string (dir);
 
-  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];
+  /* 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.
 
-  return expanded_dir;
+     In order to avoid such limitation, we only use glob to expand "~" and keep
+     "/does/not/exist" unchanged.
+
+     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) and split d around it.  */
+  const auto first_sep
+    = std::find_if (d.cbegin (), d.cend(),
+                    [](const char c) -> bool { return IS_DIR_SEPARATOR (c); });
+  const std::string to_expand (d.cbegin (), first_sep);
+  const std::string remainder (first_sep, d.cend ());
+
+  const gdb_glob glob (to_expand.c_str (), GLOB_TILDE_CHECK, nullptr);
+
+  gdb_assert (glob.pathc () == 1);
+  return std::string (glob.pathv ()[0]) + remainder;
 }
 
 /* See gdbsupport/gdb_tilde_expand.h.  */
@@ -86,10 +105,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]);
+  const std::string expanded = gdb_tilde_expand (dir);
+  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

Re: [PATCH v3] Improve gdb_tilde_expand logic

[Simon Marchi]

On 2021-01-10 8:00 p.m., 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.
> 
> Since V1:
> 	* Unittests added thanks to inputs and skeleton provided by
> 	Simon Marchi.
> 	* My copyright assignment has been signed!
> 
> Since V2:
> 	* Fix coding style issuess and typos.
> 	* Use $HOME and $USER env variables to test the behavior of
> 	gdb_tilde_expand instead of testing the function against itself.
> 
> 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 | 78 ++++++++++++++++++++++
>  gdbsupport/gdb_tilde_expand.cc             | 39 +++++++----
>  gdbsupport/gdb_tilde_expand.h              |  3 +-
>  4 files changed, 107 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..d64b7c6384
> --- /dev/null
> +++ b/gdb/unittests/gdb_tilde_expand-selftests.c
> @@ -0,0 +1,78 @@
> +/* 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 "gdbsupport/selftest.h"
> +#include <cstdlib>
> +
> +#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.  */
> +  const char *c_home = std::getenv ("HOME");
> +
> +  /* Skip the test if $HOME is not available in the environment.  */
> +  if (c_home == nullptr)
> +    return;
> +
> +  const std::string home (c_home);
> +
> +  /* Basic situation.  */
> +  SELF_CHECK (home == gdb_tilde_expand ("~"));
> +
> +  /* When given a path that begins by a tilde and refers to a file that
> +     does not exist, gdb_tilde expand must still be able to do the tilde
> +     expansion.  */
> +  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");
> +
> +  /* If $USER is available in the env variables, Check the '~user'
> +     expansion, otherwise skip the rest of the test.  */
> +  const char *c_user = std::getenv ("USER");
> +  if (c_user == nullptr)
> +    return;
> +
> +  const std::string user (c_user);
> +  SELF_CHECK (gdb_tilde_expand (("~" + user).c_str ()) == home);
> +  SELF_CHECK (gdb_tilde_expand (("~" + user + "/a/b").c_str ()) == home + "/a/b");

Thanks for the extensive testing.

One more thing we could test is a tilde with an unknown user after it
(gdb_tilde_expand ("~some-non-existent-user/foo/bar")).  The current behavior is
that an error is thrown by glob, so we could assert that an error is thrown.

> +  /* Look for the first dir separator (if any) and split d around it.  */
> +  const auto first_sep
> +    = std::find_if (d.cbegin (), d.cend(),
> +                    [](const char c) -> bool { return IS_DIR_SEPARATOR (c); });

We'd typically put a space before the parenthesis here, like we put one when defining
a standard function.

Simon

[PATCH v4] Improve gdb_tilde_expand logic

[Lancelot SIX]

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.

Since V1:
	* Unittests added thanks to inputs and skeleton provided by
	Simon Marchi.
	* My copyright assignment has been signed!

Since V2:
	* Fix coding style issuess and typos.
	* Use $HOME and $USER env variables to test the behavior of
	gdb_tilde_expand instead of testing the function against itself.

Since V3:
	* Add tests for erroneous cases.
	* Fix coding style of lambda function.

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 | 94 ++++++++++++++++++++++
 gdbsupport/gdb_tilde_expand.cc             | 46 +++++++----
 gdbsupport/gdb_tilde_expand.h              |  3 +-
 4 files changed, 128 insertions(+), 16 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..b57f7bed36
--- /dev/null
+++ b/gdb/unittests/gdb_tilde_expand-selftests.c
@@ -0,0 +1,94 @@
+/* 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 "gdbsupport/selftest.h"
+#include <cstdlib>
+
+#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.  */
+  const char *c_home = std::getenv ("HOME");
+
+  /* Skip the test if $HOME is not available in the environment.  */
+  if (c_home == nullptr)
+    return;
+
+  const std::string home (c_home);
+
+  /* Basic situation.  */
+  SELF_CHECK (home == gdb_tilde_expand ("~"));
+
+  /* When given a path that begins by a tilde and refers to a file that
+     does not exist, gdb_tilde expand must still be able to do the tilde
+     expansion.  */
+  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");
+
+  /* If $USER is available in the env variables, check the '~user'
+     expansion.  */
+  const char *c_user = std::getenv ("USER");
+  if (c_user != nullptr)
+    {
+      const std::string user (c_user);
+      SELF_CHECK (gdb_tilde_expand (("~" + user).c_str ()) == home);
+      SELF_CHECK (gdb_tilde_expand (("~" + user + "/a/b").c_str ())
+                  == home + "/a/b");
+    }
+
+  /* Check that an error is thrown when trying to expand home of a unknown
+     user.  */
+  try
+    {
+      gdb_tilde_expand ("~no_one_should_have_that_login/a");
+      SELF_CHECK (false);
+    }
+  catch (const gdb_exception_error &e)
+    {
+      SELF_CHECK (e.error == GENERIC_ERROR);
+      SELF_CHECK
+        (*e.message
+         == "Could not find a match for '~no_one_should_have_that_login'.");
+    }
+}
+
+} /* namespace gdb_tilde_expand_tests */
+} /* 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..d9fb115f47 100644
--- a/gdbsupport/gdb_tilde_expand.cc
+++ b/gdbsupport/gdb_tilde_expand.cc
@@ -18,6 +18,8 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "common-defs.h"
+#include <algorithm>
+#include "filenames.h"
 #include "gdb_tilde_expand.h"
 #include <glob.h>
 
@@ -71,14 +73,34 @@ private:
 std::string
 gdb_tilde_expand (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.  */
-  std::string expanded_dir = glob.pathv ()[0];
-
-  return expanded_dir;
+  if (dir[0] != '~')
+    return std::string (dir);
+
+  /* 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.
+
+     In order to avoid such limitation, we only use glob to expand "~" and keep
+     "/does/not/exist" unchanged.
+
+     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) and split d around it.  */
+  const auto first_sep
+    = std::find_if (d.cbegin (), d.cend(),
+                    [] (const char c) -> bool
+                    {
+                      return IS_DIR_SEPARATOR (c);
+                    });
+  const std::string to_expand (d.cbegin (), first_sep);
+  const std::string remainder (first_sep, d.cend ());
+
+  const gdb_glob glob (to_expand.c_str (), GLOB_TILDE_CHECK, nullptr);
+
+  gdb_assert (glob.pathc () == 1);
+  return std::string (glob.pathv ()[0]) + remainder;
 }
 
 /* See gdbsupport/gdb_tilde_expand.h.  */
@@ -86,10 +108,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]);
+  const std::string expanded = gdb_tilde_expand (dir);
+  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

Re: [PATCH v4] Improve gdb_tilde_expand logic

[Simon Marchi]

On 2021-01-11 5:40 p.m., 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.
> 
> Since V1:
> 	* Unittests added thanks to inputs and skeleton provided by
> 	Simon Marchi.
> 	* My copyright assignment has been signed!
> 
> Since V2:
> 	* Fix coding style issuess and typos.
> 	* Use $HOME and $USER env variables to test the behavior of
> 	gdb_tilde_expand instead of testing the function against itself.
> 
> Since V3:
> 	* Add tests for erroneous cases.
> 	* Fix coding style of lambda function.
> 
> 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.

This is ok.  Did you get your sourceware account?  Once you do, you can push
a commit to add yourself to gdb/MAINTAINERS (just look up previous commit that
touch this file for examples).

Simon

Re: [PATCH v4] Improve gdb_tilde_expand logic

[Lancelot SIX]

> 
> This is ok.  Did you get your sourceware account?  Once you do, you can push
> a commit to add yourself to gdb/MAINTAINERS (just look up previous commit that
> touch this file for examples).
> 
> Simon

Done! I’ve added myself in gdb/MAINTAINERS.

Thanks,
Lancelot.

Re: [PATCH v4] Improve gdb_tilde_expand logic

[Lancelot SIX]

Le Mon, Jan 11, 2021 at 06:36:32PM -0500, Simon Marchi a écrit :
> On 2021-01-11 5:40 p.m., Lancelot SIX via Gdb-patches wrote:
> This is ok.
>
> Simon

Pushed.

Lancelot.