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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Sunday, February 11 2018, Simon Marchi wrote:

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

Hey, Simon,

> Thanks for looking into this!

My pleasure.

> 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

Ah.  Sorry about that; that's the problem of testing on BuildBot with
everything-in-one-patch.  I'll include the declaration of
current_directory in common-defs.h and also in gdbserver/server.c.

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

Sure.

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

OK.

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

Alright, no problem.

I'll send a v2 soon.

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/