This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Wednesday, February 21 2018, Joel Brobecker wrote:

> Hi Sergio,

Hi Joel,

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

No problem.

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

Exactly.  GDB has different concepts for "inferior CWD" and "GDB CWD".
gdbserver, after my patch which implemented the inferior CWD handling,
just cares about the "inferior CWD".  But now, with the current patch
we're discussing, gdbserver will have a "current_directory" variable and
will be more aware of its own CWD.

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

Yes, that's a good point.  I will include a comment about this.

>> +/* 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.

Consider it done.

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]