This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] Create new common/pathstuff.[ch]
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/