[3/4] Introduce gdb_chdir
Commit Message
In order to be able to change the inferior's directory before its
execution, it is necessary to perform a tilde expansion of the
directory provided by the user and then chdir into the resulting dir.
This is what gdb_chdir does.
Unfortunately it is not possible to use "tilde_expand" from readline
because this is common code and gdbserver doesn't use readline. For
that reason I decided to go with "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_chdir.c.
(HFILES_NO_SRCDIR): Add gdb_chdir.h.
(COMMON_OBS): Add gdb_chdir.o.
* common/gdb_chdir.c: New file.
* common/gdb_chdir.h: Likewise.
gdb/gdbserver/ChangeLog:
yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
* Makefile.in (SFILES): Add $(srcdir)/common/gdb_chdir.c.
(OBS): Add gdb_chdir.o.
---
gdb/Makefile.in | 3 +++
gdb/common/gdb_chdir.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
gdb/common/gdb_chdir.h | 26 ++++++++++++++++++++
gdb/gdbserver/Makefile.in | 2 ++
4 files changed, 91 insertions(+)
create mode 100644 gdb/common/gdb_chdir.c
create mode 100644 gdb/common/gdb_chdir.h
Comments
> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>, Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 12 Sep 2017 00:23:24 -0400
>
> In order to be able to change the inferior's directory before its
> execution, it is necessary to perform a tilde expansion of the
> directory provided by the user and then chdir into the resulting dir.
> This is what gdb_chdir does.
>
> Unfortunately it is not possible to use "tilde_expand" from readline
> because this is common code and gdbserver doesn't use readline. For
> that reason I decided to go with "glob" and its GNU extension,
> GLOB_TILDE. With the import of the "glob" module from gnulib, this is
> a no-brainer.
Why not simply 'getenv("HOME")'?
On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote:
> +#include "common-defs.h"
> +#include <glob.h>
> +#include "gdb_chdir.h"
As per the guidelines, the module's own header should be
included first thing right after "common-defs.h". That
allows exposing unsatisfied dependencies (missing includes)
in the header, if any is missing.
> +/* Perform path expansion (i.e., tilde expansion) on DIR, and return
> + the full path. */
> +
> +static std::string
> +expand_path (const char *dir)
Since this is particularly about tilde expansion,
and a replacement for "tilde_expand", did you consider calling
it gdb_tilde_expand and using it throughout? If this were an
extern function, I'd press for having "tilde" in its name,
to make the call sites a bit more obvious.
> +{
> + glob_t g;
> + std::string expanded_dir;
Move declaration further below to the initialization line,
to avoid pointlessly default-constructing the string:
std::string expanded_dir = g.gl_pathv[0];
> + int err = glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR,
NULL, &g);
> +
> + if (err != 0)
> + {
> + if (err == GLOB_NOMATCH)
> + error (_("No such directory '%s'. Failure to set cwd."), dir);
The "Failure to set cwd" string doesn't seem like an error that
should be in "expand_path"? OK, not really an issue since this
is a single-use static function...
> +
> + error (_("Could not process directory '%s'."), dir);
> + }
> +
> + gdb_assert (g.gl_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. */
> + expanded_dir = g.gl_pathv[0];
> + globfree (&g);
Pedantically, this isn't strictly exception safe, since
either the gdb_assert or the expanded_dir initialization
(a string dup which can fail with out of memory) could throw.
I think I'd write a thin RAII wrapper around glob that'd have a
single "glob_t m_g;" field, that'd call glob in the ctor, and
globfree in the dtor.
> +
> + return expanded_dir;
> +}
> +
On Tuesday, September 12 2017, Eli Zaretskii wrote:
>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: Pedro Alves <palves@redhat.com>, Sergio Durigan Junior <sergiodj@redhat.com>
>> Date: Tue, 12 Sep 2017 00:23:24 -0400
>>
>> In order to be able to change the inferior's directory before its
>> execution, it is necessary to perform a tilde expansion of the
>> directory provided by the user and then chdir into the resulting dir.
>> This is what gdb_chdir does.
>>
>> Unfortunately it is not possible to use "tilde_expand" from readline
>> because this is common code and gdbserver doesn't use readline. For
>> that reason I decided to go with "glob" and its GNU extension,
>> GLOB_TILDE. With the import of the "glob" module from gnulib, this is
>> a no-brainer.
>
> Why not simply 'getenv("HOME")'?
Tilde expansion can be more complicated than that. If you're just using
"~/test", then yeah, you can use $HOME. However, the user can specify a
directory like "~someone/test", and in that case you have to where is
"someone's $HOME" before you can expand the tilde. glob takes care of
that for us.
On Wednesday, September 13 2017, Pedro Alves wrote:
> On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote:
>
>> +#include "common-defs.h"
>> +#include <glob.h>
>> +#include "gdb_chdir.h"
>
> As per the guidelines, the module's own header should be
> included first thing right after "common-defs.h". That
> allows exposing unsatisfied dependencies (missing includes)
> in the header, if any is missing.
Done.
>> +/* Perform path expansion (i.e., tilde expansion) on DIR, and return
>> + the full path. */
>> +
>> +static std::string
>> +expand_path (const char *dir)
>
> Since this is particularly about tilde expansion,
> and a replacement for "tilde_expand", did you consider calling
> it gdb_tilde_expand and using it throughout? If this were an
> extern function, I'd press for having "tilde" in its name,
> to make the call sites a bit more obvious.
Sure, no problem in renaming it. Just to clarify: when you mean "use it
throughout", are saying that this should be used to replace readline's
"tilde_expand" elsewhere on GDB?
>> +{
>> + glob_t g;
>> + std::string expanded_dir;
>
> Move declaration further below to the initialization line,
> to avoid pointlessly default-constructing the string:
>
> std::string expanded_dir = g.gl_pathv[0];
Done.
>> + int err = glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR,
> NULL, &g);
>> +
>> + if (err != 0)
>> + {
>> + if (err == GLOB_NOMATCH)
>> + error (_("No such directory '%s'. Failure to set cwd."), dir);
>
> The "Failure to set cwd" string doesn't seem like an error that
> should be in "expand_path"? OK, not really an issue since this
> is a single-use static function...
You're right. I'll make sure to display this error on "gdb_chdir"
instead.
>> +
>> + error (_("Could not process directory '%s'."), dir);
>> + }
>> +
>> + gdb_assert (g.gl_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. */
>> + expanded_dir = g.gl_pathv[0];
>> + globfree (&g);
>
> Pedantically, this isn't strictly exception safe, since
> either the gdb_assert or the expanded_dir initialization
> (a string dup which can fail with out of memory) could throw.
>
> I think I'd write a thin RAII wrapper around glob that'd have a
> single "glob_t m_g;" field, that'd call glob in the ctor, and
> globfree in the dtor.
OK, I can do that, no problem.
Thanks,
On 09/14/2017 04:14 PM, Sergio Durigan Junior wrote:
> On Wednesday, September 13 2017, Pedro Alves wrote:
>
>> On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote:
>>> +/* Perform path expansion (i.e., tilde expansion) on DIR, and return
>>> + the full path. */
>>> +
>>> +static std::string
>>> +expand_path (const char *dir)
>>
>> Since this is particularly about tilde expansion,
>> and a replacement for "tilde_expand", did you consider calling
>> it gdb_tilde_expand and using it throughout? If this were an
>> extern function, I'd press for having "tilde" in its name,
>> to make the call sites a bit more obvious.
>
> Sure, no problem in renaming it. Just to clarify: when you mean "use it
> throughout", are saying that this should be used to replace readline's
> "tilde_expand" elsewhere on GDB?
Yes, and no. Yes, by 'throughout' I meant elsewhere in GDB.
But no, I'm not _saying_ it should. I'm really asking if you
considered/thought about that.
I think what I'm really wondering is whether tilde_expand
and this new function behave exactly the same, or whether
glob behaves a little different in some cases. If it behaves
differently [and the differences are benign), then I get to
wonder whether we should use it throughout so that different
commands don't behave differently.
E.g., does "cd *" behave the same before/after ? Or does
'glob' expand '*' while tilde_expand didn't?
Thanks,
Pedro Alves
On Thursday, September 14 2017, Pedro Alves wrote:
> On 09/14/2017 04:14 PM, Sergio Durigan Junior wrote:
>> On Wednesday, September 13 2017, Pedro Alves wrote:
>>
>>> On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote:
>
>>>> +/* Perform path expansion (i.e., tilde expansion) on DIR, and return
>>>> + the full path. */
>>>> +
>>>> +static std::string
>>>> +expand_path (const char *dir)
>>>
>>> Since this is particularly about tilde expansion,
>>> and a replacement for "tilde_expand", did you consider calling
>>> it gdb_tilde_expand and using it throughout? If this were an
>>> extern function, I'd press for having "tilde" in its name,
>>> to make the call sites a bit more obvious.
>>
>> Sure, no problem in renaming it. Just to clarify: when you mean "use it
>> throughout", are saying that this should be used to replace readline's
>> "tilde_expand" elsewhere on GDB?
>
> Yes, and no. Yes, by 'throughout' I meant elsewhere in GDB.
> But no, I'm not _saying_ it should. I'm really asking if you
> considered/thought about that.
I thought about that while I was making the patch.
My very initial plan was to actually use 'wordexp', but I gave up on
that when I noticed that the function is not implemented in many targets
and gnulib doesn't provide a module for it. So I went ahead and decided
to use glob, but left the "cd_command" untouched just because it is
"already working" (TM).
> I think what I'm really wondering is whether tilde_expand
> and this new function behave exactly the same, or whether
> glob behaves a little different in some cases. If it behaves
> differently [and the differences are benign), then I get to
> wonder whether we should use it throughout so that different
> commands don't behave differently.
>
> E.g., does "cd *" behave the same before/after ? Or does
> 'glob' expand '*' while tilde_expand didn't?
From my initial tests I can see that "glob" and "tilde_expand" behave
the same, but I'll try to test more corner cases and see the results.
Apparently it'll be fine to just replace "tilde_expand".
@@ -1243,6 +1243,7 @@ SFILES = \
common/filestuff.c \
common/format.c \
common/job-control.c \
+ common/gdb_chdir.c \
common/gdb_vecs.c \
common/new-op.c \
common/print-utils.c \
@@ -1527,6 +1528,7 @@ HFILES_NO_SRCDIR = \
common/fileio.h \
common/format.h \
common/gdb_assert.h \
+ common/gdb_chdir.h \
common/gdb_locale.h \
common/gdb_setjmp.h \
common/gdb_signals.h \
@@ -1732,6 +1734,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
frame-unwind.o \
gcore.o \
gdb_bfd.o \
+ gdb_chdir.o \
gdb-dlfcn.o \
gdb_obstack.o \
gdb_regex.o \
new file mode 100644
@@ -0,0 +1,60 @@
+/* chdir(2) wrapper 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 <glob.h>
+#include "gdb_chdir.h"
+
+/* Perform path expansion (i.e., tilde expansion) on DIR, and return
+ the full path. */
+
+static std::string
+expand_path (const char *dir)
+{
+ glob_t g;
+ std::string expanded_dir;
+ int err = glob (dir, GLOB_TILDE | GLOB_TILDE_CHECK | GLOB_ONLYDIR, NULL, &g);
+
+ if (err != 0)
+ {
+ if (err == GLOB_NOMATCH)
+ error (_("No such directory '%s'. Failure to set cwd."), dir);
+
+ error (_("Could not process directory '%s'."), dir);
+ }
+
+ gdb_assert (g.gl_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. */
+ expanded_dir = g.gl_pathv[0];
+ globfree (&g);
+
+ return expanded_dir;
+}
+
+/* See gdb_chdir.h. */
+
+void
+gdb_chdir (const char *dir)
+{
+ std::string expanded_dir = expand_path (dir);
+
+ if (chdir (expanded_dir.c_str ()) < 0)
+ perror_with_name (expanded_dir.c_str ());
+}
new file mode 100644
@@ -0,0 +1,26 @@
+/* chdir(2) wrapper 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_CHDIR_H
+#define HAVE_GDB_CHDIR_H
+
+/* Perform a "chdir" to DIR, doing the proper tilde expansion before. */
+extern void gdb_chdir (const char *dir);
+
+#endif /* ! HAVE_GDB_CHDIR_H */
@@ -205,6 +205,7 @@ SFILES = \
$(srcdir)/common/fileio.c \
$(srcdir)/common/filestuff.c \
$(srcdir)/common/job-control.c \
+ $(srcdir)/common/gdb_chdir.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_chdir.o \
gdb_vecs.o \
hostio.o \
inferiors.o \