Re: [PATCH v2 3/5] Introduce gdb_chdir

[Pedro Alves <palves at redhat dot com>]

On 09/19/2017 05:28 AM, Sergio Durigan Junior wrote:
> In order to be able to change the inferior's directory before its
> execution, it is necessary to perform a tilde expansion of the
> directory provided by the user and then chdir into the resulting dir.
> This is what gdb_chdir does.
> 
> Unfortunately it is not possible to use "tilde_expand" from readline
> because this is common code and gdbserver doesn't use readline.  For
> that reason I decided to go with "glob" and its GNU extension,
> GLOB_TILDE.  With the import of the "glob" module from gnulib, this is
> a no-brainer.
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* Makefile.in (SFILES): Add gdb_chdir.c.
> 	(HFILES_NO_SRCDIR): Add gdb_chdir.h.
> 	(COMMON_OBS): Add gdb_chdir.o.
> 	* cli/cli-cmds.c: Include "gdb_chdir.h".
> 	(cd_command): Use "gdb_chdir" instead of "tilde_expand
> 	plus chdir".
> 	* common/gdb_chdir.c: New file.
> 	* common/gdb_chdir.h: Likewise.
> 
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* Makefile.in (SFILES): Add $(srcdir)/common/gdb_chdir.c.
> 	(OBS): Add gdb_chdir.o.
> ---
>  gdb/Makefile.in           |  3 ++
>  gdb/cli/cli-cmds.c        | 20 +++++-----
>  gdb/common/gdb_chdir.c    | 96 +++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/common/gdb_chdir.h    | 26 +++++++++++++
>  gdb/gdbserver/Makefile.in |  2 +
>  5 files changed, 136 insertions(+), 11 deletions(-)
>  create mode 100644 gdb/common/gdb_chdir.c
>  create mode 100644 gdb/common/gdb_chdir.h
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9dfc117b2f..1f093e6c0f 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -1245,6 +1245,7 @@ SFILES = \
>  	common/filestuff.c \
>  	common/format.c \
>  	common/job-control.c \
> +	common/gdb_chdir.c \
>  	common/gdb_vecs.c \
>  	common/new-op.c \
>  	common/print-utils.c \
> @@ -1529,6 +1530,7 @@ HFILES_NO_SRCDIR = \
>  	common/fileio.h \
>  	common/format.h \
>  	common/gdb_assert.h \
> +	common/gdb_chdir.h \
>  	common/gdb_locale.h \
>  	common/gdb_setjmp.h \
>  	common/gdb_signals.h \
> @@ -1734,6 +1736,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
>  	frame-unwind.o \
>  	gcore.o \
>  	gdb_bfd.o \
> +	gdb_chdir.o \
>  	gdb-dlfcn.o \
>  	gdb_obstack.o \
>  	gdb_regex.o \
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 85d6d21113..653dd56a64 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -54,6 +54,8 @@
>  #include "tui/tui.h"	/* For tui_active et.al.  */
>  #endif
>  
> +#include "gdb_chdir.h"
> +
>  #include <fcntl.h>
>  #include <algorithm>
>  #include <string>
> @@ -408,12 +410,7 @@ cd_command (char *dir, int from_tty)
>       repeat might be useful but is more likely to be a mistake.  */
>    dont_repeat ();
>  
> -  gdb::unique_xmalloc_ptr<char> dir_holder
> -    (tilde_expand (dir != NULL ? dir : "~"));
> -  dir = dir_holder.get ();
> -
> -  if (chdir (dir) < 0)
> -    perror_with_name (dir);
> +  gdb_chdir (dir != NULL ? dir : "~");
>  
>  #ifdef HAVE_DOS_BASED_FILE_SYSTEM
>    /* There's too much mess with DOSish names like "d:", "d:.",
> @@ -436,20 +433,21 @@ cd_command (char *dir, int from_tty)
>  	len--;
>      }
>  
> -  dir_holder.reset (savestring (dir, len));
> -  if (IS_ABSOLUTE_PATH (dir_holder.get ()))
> +  std::string newdir = std::string (dir, len);
> +  const char *newdir_str = newdir.c_str ();
> +  if (IS_ABSOLUTE_PATH (newdir_str))
>      {
>        xfree (current_directory);
> -      current_directory = dir_holder.release ();
> +      current_directory = xstrdup (newdir_str);

This introduces one extra deep string dup.  One to
construct the std::string, and another here in this xstrdup.

How about simply, above:

 -  dir_holder.reset (savestring (dir, len));
 +  gdb::unique_xmalloc_ptr<char> dir_holder (savestring (dir, len))


But more importantly, isn't there a behavior change here?
Before, current_directory would be a copy of the the expand
path, while after the patch, it's a copy of the input string,
before expansion.  Right?


>      }
>    else
>      {
>        if (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1]))
> -	current_directory = concat (current_directory, dir_holder.get (),
> +	current_directory = concat (current_directory, newdir_str,
>  				    (char *) NULL);
>        else
>  	current_directory = concat (current_directory, SLASH_STRING,
> -				    dir_holder.get (), (char *) NULL);
> +				    newdir_str, (char *) NULL);
>      }


> +  /* Destroy the object and free M_GLOB.  */
> +  ~gdb_glob ()
> +  {
> +    globfree (&m_glob);
> +  }
> +
> +  /* Return the GL_PATHC component of M_GLOB.  */
> +  int
> +  pathc () const
> +  {
> +    return m_glob.gl_pathc;
> +  }

We've been putting type and function name in the
same line in inline member functions.  That's the
de facto standard in GCC as well, AFAICS.

> +
> +  /* Return the GL_PATHV component of M_GLOB.  */
> +  char **
> +  pathv () const
> +  {
> +    return m_glob.gl_pathv;
> +  }

Ditto.

> +#ifndef HAVE_GDB_CHDIR_H
> +#define HAVE_GDB_CHDIR_H
> +
> +/* Perform a "chdir" to DIR, doing the proper tilde expansion before.  */
> +extern void gdb_chdir (const char *dir);

Nit, I'd drop "the proper", as it doesn't see to add value.
I.e., what's proper and what's not proper?

Thanks,
Pedro Alves