[v3,3/5] Introduce gdb_tilde_expand
Commit Message
Currently, whenever we want to handle paths provided by the user and
perform tilde expansion on GDB, we rely on "tilde_expand", which comes
from readline. This was enough for our use cases so far, but the
situation will change when we start dealing with paths on gdbserver as
well, which is what the next patches implement.
Unfortunately it is not possible to use "tilde_expand" in this case
because gdbserver doesn't use readline. For that reason I decided to
implement a new "gdb_tilde_expand" function, which is basically a
wrapper for "glob" and its GNU extension, GLOB_TILDE. With the import
of the "glob" module from gnulib, this is a no-brainer.
gdb/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* Makefile.in (SFILES): Add gdb_tilde_expand.c.
(HFILES_NO_SRCDIR): Add gdb_tilde_expand.h.
(COMMON_OBS): Add gdb_tilde_expand.o.
* cli/cli-cmds.c: Include "gdb_tilde_expand.h".
(cd_command): Use "gdb_tilde_expand" instead of "tilde_expand".
* common/gdb_tilde_expand.c: New file.
* common/gdb_tilde_expand.h: Likewise.
gdb/gdbserver/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* Makefile.in (SFILES): Add $(srcdir)/common/gdb_tilde_expand.c.
(OBS): Add gdb_tilde_expand.o.
---
gdb/Makefile.in | 3 ++
gdb/cli/cli-cmds.c | 10 +++---
gdb/common/gdb_tilde_expand.c | 82 +++++++++++++++++++++++++++++++++++++++++++
gdb/common/gdb_tilde_expand.h | 27 ++++++++++++++
gdb/gdbserver/Makefile.in | 2 ++
5 files changed, 119 insertions(+), 5 deletions(-)
create mode 100644 gdb/common/gdb_tilde_expand.c
create mode 100644 gdb/common/gdb_tilde_expand.h
Comments
On 09/21/2017 11:59 PM, Sergio Durigan Junior wrote:
> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index cbafb13837..507fdc4120 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -54,6 +54,8 @@
> #include "tui/tui.h" /* For tui_active et.al. */
> #endif
>
> +#include "gdb_tilde_expand.h"
> +
> #include <fcntl.h>
> #include <algorithm>
> #include <string>
> @@ -410,10 +412,8 @@ cd_command (char *dir, int from_tty)
> repeat might be useful but is more likely to be a mistake. */
> dont_repeat ();
>
> - gdb::unique_xmalloc_ptr<char> dir_holder
> - (tilde_expand (dir != NULL ? dir : "~"));
> - dir = dir_holder.get ();
> -
> + std::string expanded_path = gdb_tilde_expand (dir != NULL ? dir : "~");
> + dir = (char *) expanded_path.c_str ();
> if (chdir (dir) < 0)
> perror_with_name (dir);
>
> @@ -438,7 +438,7 @@ cd_command (char *dir, int from_tty)
> len--;
> }
>
> - dir_holder.reset (savestring (dir, len));
> + gdb::unique_xmalloc_ptr<char> dir_holder (savestring (dir, len));
> if (IS_ABSOLUTE_PATH (dir_holder.get ()))
> {
> xfree (current_directory);
I realized something: in light of the fact that "cd" is not what
is used to specify the inferior's cwd anymore since v1, patching
this particular use of tilde_expand, and not others seems arbitrary.
I.e., this now looks like kind of a spurious change to me, and
I think you should drop the changes to this file...
> +/* See common/gdb_tilde_expand.h. */
> +
> +std::string
> +gdb_tilde_expand (const char *dir)
> +{
> + gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.
I'm not sure whether GLOB_ONLYDIR is the right thing to do in
a function whose name doesn't suggest that it concern itself
with anything other than tilde expansion. E.g., if we replaced
tilde_expand with gdb_tilde_expand in place that expect to match
files (like e.g., gdb_print_filename), then would this work as is?
Maybe you should rename this to gdb_tilde_expand_dir (leaving
the file names as is).
Thanks,
Pedro Alves
On Friday, September 22 2017, Pedro Alves wrote:
> On 09/21/2017 11:59 PM, Sergio Durigan Junior wrote:
>
>> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
>> index cbafb13837..507fdc4120 100644
>> --- a/gdb/cli/cli-cmds.c
>> +++ b/gdb/cli/cli-cmds.c
>> @@ -54,6 +54,8 @@
>> #include "tui/tui.h" /* For tui_active et.al. */
>> #endif
>>
>> +#include "gdb_tilde_expand.h"
>> +
>> #include <fcntl.h>
>> #include <algorithm>
>> #include <string>
>> @@ -410,10 +412,8 @@ cd_command (char *dir, int from_tty)
>> repeat might be useful but is more likely to be a mistake. */
>> dont_repeat ();
>>
>> - gdb::unique_xmalloc_ptr<char> dir_holder
>> - (tilde_expand (dir != NULL ? dir : "~"));
>> - dir = dir_holder.get ();
>> -
>> + std::string expanded_path = gdb_tilde_expand (dir != NULL ? dir : "~");
>> + dir = (char *) expanded_path.c_str ();
>> if (chdir (dir) < 0)
>> perror_with_name (dir);
>>
>> @@ -438,7 +438,7 @@ cd_command (char *dir, int from_tty)
>> len--;
>> }
>>
>> - dir_holder.reset (savestring (dir, len));
>> + gdb::unique_xmalloc_ptr<char> dir_holder (savestring (dir, len));
>> if (IS_ABSOLUTE_PATH (dir_holder.get ()))
>> {
>> xfree (current_directory);
>
>
> I realized something: in light of the fact that "cd" is not what
> is used to specify the inferior's cwd anymore since v1, patching
> this particular use of tilde_expand, and not others seems arbitrary.
>
> I.e., this now looks like kind of a spurious change to me, and
> I think you should drop the changes to this file...
Yeah, you're right. I still intend to keep the cleanups, if that's OK
for you.
>> +/* See common/gdb_tilde_expand.h. */
>> +
>> +std::string
>> +gdb_tilde_expand (const char *dir)
>> +{
>> + gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
>
> By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.
Yes, but I think it pays to be explicit in this case.
> I'm not sure whether GLOB_ONLYDIR is the right thing to do in
> a function whose name doesn't suggest that it concern itself
> with anything other than tilde expansion. E.g., if we replaced
> tilde_expand with gdb_tilde_expand in place that expect to match
> files (like e.g., gdb_print_filename), then would this work as is?
>
> Maybe you should rename this to gdb_tilde_expand_dir (leaving
> the file names as is).
Or I could simply remove GLOB_ONLYDIR (which is a leftover from the
previous versions of the patch) and make this function generic. In
fact, I think I prefer this approach, because according to the manpage:
GLOB_ONLYDIR
This is a hint to glob() that the caller is interested
only in directories that match the pattern. If the
implementation can easily determine file-type
information, then nondirectory files are not returned
to the caller. However, the caller must still check
that returned files are directories. (The purpose of
this flag is merely to optimize performance when the
caller is interested only in directories.)
I.e., it's only a helper flag for "glob".
I'll remove GLOB_ONLYDIR and resubmit the patch.
Thanks,
On 09/22/2017 06:37 PM, Sergio Durigan Junior wrote:
> On Friday, September 22 2017, Pedro Alves wrote:
>> I realized something: in light of the fact that "cd" is not what
>> is used to specify the inferior's cwd anymore since v1, patching
>> this particular use of tilde_expand, and not others seems arbitrary.
>>
>> I.e., this now looks like kind of a spurious change to me, and
>> I think you should drop the changes to this file...
>
> Yeah, you're right. I still intend to keep the cleanups, if that's OK
> for you.
I don't know what you mean by that.
>
>>> +/* See common/gdb_tilde_expand.h. */
>>> +
>>> +std::string
>>> +gdb_tilde_expand (const char *dir)
>>> +{
>>> + gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
>>
>> By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.
>
> Yes, but I think it pays to be explicit in this case.
I think it only adds to confusion. I did "man glob",
saw that GLOB_TILDE_CHECK implies GLOB_TILDE and then
got to wonder why is GLOB_TILDE being passed explicitly.
> I'll remove GLOB_ONLYDIR and resubmit the patch.
Fine with me.
Thanks,
Pedro Alves
On Friday, September 22 2017, Pedro Alves wrote:
> On 09/22/2017 06:37 PM, Sergio Durigan Junior wrote:
>> On Friday, September 22 2017, Pedro Alves wrote:
>
>>> I realized something: in light of the fact that "cd" is not what
>>> is used to specify the inferior's cwd anymore since v1, patching
>>> this particular use of tilde_expand, and not others seems arbitrary.
>>>
>>> I.e., this now looks like kind of a spurious change to me, and
>>> I think you should drop the changes to this file...
>>
>> Yeah, you're right. I still intend to keep the cleanups, if that's OK
>> for you.
>
> I don't know what you mean by that.
Sorry, I hadn't really understood what you were saying. I will remove
the changes to gdb/cli/cli-cmds.c from the patch.
>>>> +/* See common/gdb_tilde_expand.h. */
>>>> +
>>>> +std::string
>>>> +gdb_tilde_expand (const char *dir)
>>>> +{
>>>> + gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
>>>
>>> By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.
>>
>> Yes, but I think it pays to be explicit in this case.
>
> I think it only adds to confusion. I did "man glob",
> saw that GLOB_TILDE_CHECK implies GLOB_TILDE and then
> got to wonder why is GLOB_TILDE being passed explicitly.
Well, I personally don't really see the reason for the confusion here
since you could have read GLOB_TILDE's entry as well and noticed that
there is nothing fancy being done here, but I respect your way of
thinking, so I will just remove GLOB_TILDE_CHECK.
Thanks,
On 09/22/2017 07:07 PM, Sergio Durigan Junior wrote:
> On Friday, September 22 2017, Pedro Alves wrote:
>
>>>>> +/* See common/gdb_tilde_expand.h. */
>>>>> +
>>>>> +std::string
>>>>> +gdb_tilde_expand (const char *dir)
>>>>> +{
>>>>> + gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
>>>>
>>>> By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.
>>>
>>> Yes, but I think it pays to be explicit in this case.
>>
>> I think it only adds to confusion. I did "man glob",
>> saw that GLOB_TILDE_CHECK implies GLOB_TILDE and then
>> got to wonder why is GLOB_TILDE being passed explicitly.
>
> Well, I personally don't really see the reason for the confusion here
> since you could have read GLOB_TILDE's entry as well and noticed that
> there is nothing fancy being done here, but I respect your way of
> thinking, so I will just remove GLOB_TILDE_CHECK.
I did not suggest to remove GLOB_TILDE_CHECK.
What I'm saying is that the man page looks like this:
GLOB_TILDE
Carry out tilde expansion. (...)
GLOB_TILDE_CHECK
This provides behavior similar to that of GLOB_TILDE. The difference is that (...)
And from reading this, my interpretation is that GLOB_TILDE
and GLOB_TILDE_CHECK are two different modes. From that
description, I'd think it reasonable for glob to reject
"GLOB_TILDE | GLOB_TILDE_CHECK" as ambiguous -- which mode
to you want with that?
Note that it says "provides behavior similar", not
"in combination with GLOB_TILDE".
With GLOB_TILDE | GLOB_TILDE_CHECK, I assumed that you
meant GLOB_TILDE_CHECK, but I have no idea whether that's
what all glob implementations actually end up interpreting
it as. But why leave that ambiguity? Just specify what
you want, exactly.
Thanks,
Pedro Alves
On Friday, September 22 2017, Pedro Alves wrote:
> On 09/22/2017 07:07 PM, Sergio Durigan Junior wrote:
>> On Friday, September 22 2017, Pedro Alves wrote:
>>
>>>>>> +/* See common/gdb_tilde_expand.h. */
>>>>>> +
>>>>>> +std::string
>>>>>> +gdb_tilde_expand (const char *dir)
>>>>>> +{
>>>>>> + gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
>>>>>
>>>>> By my reading of man glob, GLOB_TILDE_CHECK already implies GLOB_TILDE.
>>>>
>>>> Yes, but I think it pays to be explicit in this case.
>>>
>>> I think it only adds to confusion. I did "man glob",
>>> saw that GLOB_TILDE_CHECK implies GLOB_TILDE and then
>>> got to wonder why is GLOB_TILDE being passed explicitly.
>>
>> Well, I personally don't really see the reason for the confusion here
>> since you could have read GLOB_TILDE's entry as well and noticed that
>> there is nothing fancy being done here, but I respect your way of
>> thinking, so I will just remove GLOB_TILDE_CHECK.
>
> I did not suggest to remove GLOB_TILDE_CHECK.
Sorry, my bad, I meant "remove GLOB_TILDE".
@@ -1245,6 +1245,7 @@ SFILES = \
common/filestuff.c \
common/format.c \
common/job-control.c \
+ common/gdb_tilde_expand.c \
common/gdb_vecs.c \
common/new-op.c \
common/print-utils.c \
@@ -1529,6 +1530,7 @@ HFILES_NO_SRCDIR = \
common/fileio.h \
common/format.h \
common/gdb_assert.h \
+ common/gdb_tilde_expand.h \
common/gdb_locale.h \
common/gdb_setjmp.h \
common/gdb_signals.h \
@@ -1734,6 +1736,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
frame-unwind.o \
gcore.o \
gdb_bfd.o \
+ gdb_tilde_expand.o \
gdb-dlfcn.o \
gdb_obstack.o \
gdb_regex.o \
@@ -54,6 +54,8 @@
#include "tui/tui.h" /* For tui_active et.al. */
#endif
+#include "gdb_tilde_expand.h"
+
#include <fcntl.h>
#include <algorithm>
#include <string>
@@ -410,10 +412,8 @@ cd_command (char *dir, int from_tty)
repeat might be useful but is more likely to be a mistake. */
dont_repeat ();
- gdb::unique_xmalloc_ptr<char> dir_holder
- (tilde_expand (dir != NULL ? dir : "~"));
- dir = dir_holder.get ();
-
+ std::string expanded_path = gdb_tilde_expand (dir != NULL ? dir : "~");
+ dir = (char *) expanded_path.c_str ();
if (chdir (dir) < 0)
perror_with_name (dir);
@@ -438,7 +438,7 @@ cd_command (char *dir, int from_tty)
len--;
}
- dir_holder.reset (savestring (dir, len));
+ gdb::unique_xmalloc_ptr<char> dir_holder (savestring (dir, len));
if (IS_ABSOLUTE_PATH (dir_holder.get ()))
{
xfree (current_directory);
new file mode 100644
@@ -0,0 +1,82 @@
+/* Perform tilde expansion on paths for GDB and gdbserver.
+
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#include "common-defs.h"
+#include "gdb_tilde_expand.h"
+#include <glob.h>
+
+/* RAII-style class wrapping "glob". */
+
+class gdb_glob
+{
+public:
+ /* Construct a "gdb_glob" object by calling "glob" with the provided
+ parameters. This function can throw if "glob" fails. */
+ gdb_glob (const char *pattern, int flags,
+ int (*errfunc) (const char *epath, int eerrno))
+ {
+ int ret = glob (pattern, flags, errfunc, &m_glob);
+
+ if (ret != 0)
+ {
+ if (ret == GLOB_NOMATCH)
+ error (_("Could not find a match for '%s'."), pattern);
+ else
+ error (_("glob could not process pattern '%s'."),
+ pattern);
+ }
+ }
+
+ /* Destroy the object and free M_GLOB. */
+ ~gdb_glob ()
+ {
+ globfree (&m_glob);
+ }
+
+ /* Return the GL_PATHC component of M_GLOB. */
+ int pathc () const
+ {
+ return m_glob.gl_pathc;
+ }
+
+ /* Return the GL_PATHV component of M_GLOB. */
+ char **pathv () const
+ {
+ return m_glob.gl_pathv;
+ }
+
+private:
+ /* The actual glob object we're dealing with. */
+ glob_t m_glob;
+};
+
+/* See common/gdb_tilde_expand.h. */
+
+std::string
+gdb_tilde_expand (const char *dir)
+{
+ gdb_glob glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL);
+
+ gdb_assert (glob.pathc () > 0);
+ /* "glob" may return more than one match to the path provided by the
+ user, but we are only interested in the first match. */
+ std::string expanded_dir = glob.pathv ()[0];
+
+ return expanded_dir;
+}
new file mode 100644
@@ -0,0 +1,27 @@
+/* Perform tilde expansion on paths for GDB and gdbserver.
+
+ Copyright (C) 2017 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef HAVE_GDB_TILDE_EXPAND_H
+#define HAVE_GDB_TILDE_EXPAND_H
+
+/* Perform path expansion (i.e., tilde expansion) on DIR, and return
+ the full path. */
+extern std::string gdb_tilde_expand (const char *dir);
+
+#endif /* ! HAVE_GDB_TILDE_EXPAND_H */
@@ -205,6 +205,7 @@ SFILES = \
$(srcdir)/common/fileio.c \
$(srcdir)/common/filestuff.c \
$(srcdir)/common/job-control.c \
+ $(srcdir)/common/gdb_tilde_expand.c \
$(srcdir)/common/gdb_vecs.c \
$(srcdir)/common/new-op.c \
$(srcdir)/common/print-utils.c \
@@ -247,6 +248,7 @@ OBS = \
fileio.o \
filestuff.o \
format.o \
+ gdb_tilde_expand.o \
gdb_vecs.o \
hostio.o \
inferiors.o \