[3/4] Introduce gdb_chdir

  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
  

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