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

[Joel Brobecker <brobecker@adacore.com>]

Hi Sergio,


On Fri, Feb 09, 2018 at 08:42:40PM -0500, 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.
> 
> gdb/ChangeLog:
> 2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* Makefile.in (SFILES): Add "common/pathstuff.c".
> 	(HFILES_NO_SRCDIR): Add "common/pathstuff.h".
> 	(COMMON_OBS): Add "pathstuff.o".
> 	* common/pathstuff.c: New file.
> 	* common/pathstuff.h: New file.
> 	* utils.c (gdb_realpath): Move to "common/pathstuff.c".
> 	(gdb_realpath_keepfile): Likewise.
> 	(gdb_abspath): Likewise.
> 	* utils.h: Include "common/pathstuff.h".
> 	(gdb_realpath): Move to "common/pathstuff.h".
> 	(gdb_realpath_keepfile): Likewise.
> 	(gdb_abspath): Likewise.
> 
> gdb/gdbserver/ChangeLog:
> 2018-02-09  Sergio Durigan Junior  <sergiodj@redhat.com>

Thanks for doing this work!

> 	* Makefile.in (SFILES): Add "$(srcdir)/common/pathstuff.c".
> 	(OBJS): Add "pathstuff.o".
> +++ b/gdb/common/pathstuff.c
> @@ -0,0 +1,144 @@
[...]
> +gdb::unique_xmalloc_ptr<char>
> +gdb_realpath (const char *filename)

I realize you are just moving the function, but the function's missing
some documentation. I think it would be useful to add.

What lead me to this is the fact that there is one particularly
important element, is that this function used the current working
directory to expand relative paths. In the case of GDB, I verified
that when the user does a "cd DIR", GDB updates both current_directory
and its actual current working directory (in other words, we always
maintain the property current_directory == getcwd ().

In GDBserver, however, it doesn't seem to be the case. So I think
we need to be explicit about that, because calls to gdb_realpath
and gdb_abspath with the same filename  might actually return
the path to two different files if the conditions are right!

Ideally, I think we would want gdb_realpath and gdb_abspath to
return the same value. But, if we are interested, I suggest we
discuss that separately from this thread. This is potentially
disruptive (and potentially in a good way ;-)).

> +/* Return PATH in absolute form, performing tilde-expansion if necessary.
> +   PATH cannot be NULL or the empty string.
> +   This does not resolve symlinks however, use gdb_realpath for that.  */
> +
> +extern gdb::unique_xmalloc_ptr<char> gdb_abspath (const char *path);

Similar to the above, I think we should be clear that the expansion
of relative path is done relative to the current_directory (or
the current working directory if NULL). Something like that.


-- 
Joel