Re: [PATCH 1/2] Create new common/pathstuff.[ch]

[Simon Marchi <simark at simark dot ca>]

On 2018-02-09 08:42 PM, Sergio Durigan Junior wrote:
> This commit moves the path manipulation routines found on utils.c to a
> new common/pathstuff.c, and updates the Makefile.in's accordingly.
> The routines moved are "gdb_realpath", "gdb_realpath_keepfile" and
> "gdb_abspath".
> 
> This will be needed because gdbserver will have to call "gdb_abspath"
> on my next patch, which implements a way to expand the path of the
> inferior provided by the user in order to allow specifying just the
> binary name when starting gdbserver, like:
> 
>   $ gdbserver :1234 a.out
> 
> With the recent addition of the startup-with-shell feature on
> gdbserver, this scenario doesn't work anymore if the user doesn't have
> the current directory listed in the PATH variable.
> 
> I had to do a minor adjustment on "gdb_abspath" because we don't have
> access to "tilde_expand" on gdbserver, so now the function is using
> "gdb_tilde_expand" instead.  Otherwise, the code is the same.
> 
> Regression tested on the BuildBot, without regressions.

Hi Sergio,

Thanks for looking into this!

This commit does not build:

/home/simark/src/binutils-gdb/gdb/common/pathstuff.c: In function ‘gdb::unique_xmalloc_ptr<char> gdb_abspath(const char*)’:
/home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: error: ‘current_directory’ was not declared in this scope
     (concat (current_directory,
              ^~~~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/common/pathstuff.c:140:14: note: suggested alternative: ‘read_direction’
     (concat (current_directory,
              ^~~~~~~~~~~~~~~~~
              read_direction


I guess you need to move the declaration to common-defs.h in this commit instead of
the next one.  I also got this whitespace error from git am:

.git/rebase-apply/patch:131: trailing whitespace.
  /* Extract the basename of filename, and return immediately

I think it's in code you copied, but if you can remove the extra space while at it
it would be nice.

> +/* See common/pathstuff.h.  */
> +
> +gdb::unique_xmalloc_ptr<char>
> +gdb_abspath (const char *path)
> +{
> +  gdb_assert (path != NULL && path[0] != '\0');
> +
> +  if (path[0] == '~')
> +    {
> +      std::string new_path = gdb_tilde_expand (path);
> +
> +      return gdb::unique_xmalloc_ptr<char> (xstrdup (new_path.c_str ()));

We should try to avoid unnecessary copies, when possible.  Here, we could either make
another version of gdb_tilde_expand (it would have to be another name) that returns
a gdb::unique_xmalloc_ptr<char> or make gdb_abspath return an std::string.  I think the
former would be better for now because some callers require a gdb::unique_xmalloc_ptr<char>,
and would have to do a copy themselves.  So using a gdb::unique_xmalloc_ptr across the
whole chain would give the least amount of copies.

> diff --git a/gdb/utils.h b/gdb/utils.h
> index b234762929..4f25be0968 100644
> --- a/gdb/utils.h
> +++ b/gdb/utils.h
> @@ -23,6 +23,7 @@
>  
>  #include "exceptions.h"
>  #include "common/scoped_restore.h"
> +#include "common/pathstuff.h"

I don't think utils.h should be including common/pathstuff.h, because it is not using it.
I understand why you did this (this ensures that every current user of these functions
will automatically see the new declaration), but in the long term I think it's better if
the users include "pathstuff.h".  The files that use these functions are:

$ grep -e gdb_realpath_keepfile -e gdb_realpath -e gdb_abspath *.c */*.c -l | sort
auto-load.c
common/pathstuff.c
compile/compile.c
dwarf2read.c
exec.c
guile/scm-safe-call.c
linux-thread-db.c
main.c
nto-tdep.c
objfiles.c
source.c
symtab.c
utils.c

They are all files included in a --enable-targets=all build, so it should be easy to
find where #include "common/pathstuff.h" is missing.

Thanks,

Simon