Re: [PATCH] Improve gdb_tilde_expand logic

[Andrew Burgess <andrew.burgess@embecosm.com>]

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