Re: [PATCH v2 4/5] Implement "set cwd" command

[Pedro Alves <palves at redhat dot com>]

On 09/19/2017 05:28 AM, Sergio Durigan Junior wrote:

> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
> index 653dd56a64..f086f10aea 100644
> --- a/gdb/cli/cli-cmds.c
> +++ b/gdb/cli/cli-cmds.c
> @@ -1716,9 +1716,9 @@ The commands below can be used to select other frames by number or address."),
>  Print working directory.  This is used for your program as well."));
>  
>    c = add_cmd ("cd", class_files, cd_command, _("\
> -Set working directory to DIR for debugger and program being debugged.\n\
> -The change does not take effect for the program being debugged\n\
> -until the next time it is started."), &cmdlist);
> +Set working directory to DIR for debugger.\n\


I think it'd be nice to add a sentence here suggesting what the
current working directory affects.

> +In order to change the inferior's current working directory, the recommended\n\
> +way is to use the \"set cwd\" command."), &cmdlist);
>    set_cmd_completer (c, filename_completer);
>  
>    add_com ("echo", class_support, echo_command, _("\
> diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h
> index 87c13009ed..515a8c0f4e 100644
> --- a/gdb/common/common-inferior.h
> +++ b/gdb/common/common-inferior.h
> @@ -30,4 +30,8 @@ extern const char *get_exec_wrapper ();
>     otherwise return 0 in that case.  */
>  extern char *get_exec_file (int err);
>  
> +/* Return the inferior's current working directory.  If nothing has
> +   been set, then return NULL.  */
> +extern const char *get_inferior_cwd ();
> +
>  #endif /* ! COMMON_INFERIOR_H */
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index af5fe6f46c..c513a49c26 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -2057,8 +2057,9 @@ environment} to change parts of the environment that affect
>  your program.  @xref{Environment, ,Your Program's Environment}.
>  
>  @item The @emph{working directory.}
> -Your program inherits its working directory from @value{GDBN}.  You can set
> -the @value{GDBN} working directory with the @code{cd} command in @value{GDBN}.
> +You can set your program's working directory with the command
> +@code{set cwd}.  If you do not set any working directory with this
> +command, your program will inherit @value{GDBN}'s working directory.
>  @xref{Working Directory, ,Your Program's Working Directory}.
>  
>  @item The @emph{standard input and output.}
> @@ -2424,19 +2425,36 @@ variables to files that are only run when you sign on, such as
>  @section Your Program's Working Directory
>  
>  @cindex working directory (of your program)
> -Each time you start your program with @code{run}, it inherits its
> -working directory from the current working directory of @value{GDBN}.
> -The @value{GDBN} working directory is initially whatever it inherited
> -from its parent process (typically the shell), but you can specify a new
> -working directory in @value{GDBN} with the @code{cd} command.
> +Each time you start your program with @code{run}, the inferior will be
> +initialized with the current working directory specified by the
> +@code{set cwd} command.  If no directory has been specified by this
> +command, then the inferior will inherit @value{GDBN}'s current working
> +directory as its working directory.
> +
> +You can also change @value{GDBN}'s current working directory by using
> +the @code{cd} command.
>  
>  The @value{GDBN} working directory also serves as a default for the commands
>  that specify files for @value{GDBN} to operate on.  @xref{Files, ,Commands to
>  Specify Files}.
>  
>  @table @code
> +@kindex set cwd
> +@cindex change inferior's working directory
> +@item set cwd @r{[}@var{directory}@r{]}
> +Set the inferior's working directory to @var{directory}.  If not
> +given, @var{directory} uses @file{'~'}.  This has no effect on
> +@value{GDBN}'s working directory.


I think we're missing a sentence somewhere in the manual saying that 
this setting only takes effect the next time you start the program, 
like it is said in "help cd" currently.

> +
> +@kindex show cwd
> +@cindex show inferior's working directory
> +@item show cwd
> +Show the inferior's working directory.  If no directory has been
> +specified by @code{set cwd}, then the default inferior's working
> +directory is the same as @value{GDBN}'s working directory.
> +
>  @kindex cd
> -@cindex change working directory
> +@cindex change @value{GDBN}'s working directory
>  @item cd @r{[}@var{directory}@r{]}
>  Set the @value{GDBN} working directory to @var{directory}.  If not
>  given, @var{directory} uses @file{'~'}.
> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c
> index 72f0412757..e78ad4faf1 100644
> --- a/gdb/gdbserver/inferiors.c
> +++ b/gdb/gdbserver/inferiors.c
> @@ -29,6 +29,9 @@ struct thread_info *current_thread;
>  
>  #define get_thread(inf) ((struct thread_info *)(inf))
>  
> +/* The current working directory used to start the inferior.  */
> +static const char *current_inferior_cwd = NULL;
> +
>  void
>  add_inferior_to_list (struct inferior_list *list,
>  		      struct inferior_list_entry *new_inferior)
> @@ -445,3 +448,11 @@ switch_to_thread (ptid_t ptid)
>    if (!ptid_equal (ptid, minus_one_ptid))
>      current_thread = find_thread_ptid (ptid);
>  }
> +
> +/* See common/common-inferior.h.  */
> +
> +const char *
> +get_inferior_cwd ()
> +{
> +  return current_inferior_cwd;
> +}
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 25cf025426..178a3ca15f 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -59,6 +59,8 @@
>  #include "top.h"
>  #include "interps.h"
>  #include "common/gdb_optional.h"
> +#include "gdb_chdir.h"
> +#include "readline/tilde.h"
>  
>  /* Local functions: */
>  
> @@ -111,6 +113,10 @@ static void run_command (char *, int);
>  
>  static char *inferior_args_scratch;
>  
> +/* Scratch area where the new cwd will be stored by 'set cwd'.  */
> +
> +static char *inferior_cwd_scratch;
> +
>  /* Scratch area where 'set inferior-tty' will store user-provided value.
>     We'll immediate copy it into per-inferior storage.  */
>  
> @@ -246,6 +252,56 @@ show_args_command (struct ui_file *file, int from_tty,
>    deprecated_show_value_hack (file, from_tty, c, get_inferior_args ());
>  }
>  
> +/* Set the inferior current working directory.  This directory will be
> +   entered by GDB before executing the inferior.  */

The second sentence is talking about implementation details of Unix-like
targets, which doesn't really belong here.

> +
> +static void
> +set_inferior_cwd (const char *cwd)
> +{
> +  struct inferior *inf = current_inferior ();
> +
> +  gdb_assert (inf != NULL);
> +  xfree ((void *) inf->cwd);
> +  inf->cwd = tilde_expand (*cwd != '\0' ? cwd : "~");

So the inferior owns the string?  Wouldn't it be better if
inf->cwd were a unique_ptr?  I don't see anywhere releasing
the cwd string when an inferior is deleted.

> +
> +/* Handle the 'show cwd' command.  */
> +
> +static void
> +show_cwd_command (struct ui_file *file, int from_tty,
> +		  struct cmd_list_element *c, const char *value)
> +{
> +  const char *cwd = get_inferior_cwd ();
> +
> +  if (cwd == NULL)
> +    {
> +      /* To maintain backwards compatibility, we use
> +	 'current_directory' here, which is set by the "cd"
> +	 command.  */
> +      cwd = current_directory;
> +    }

This doesn't look like the right thing to show, to me.
It'll only make sense for native debugging, not remote debugging,
right?  I mean, when remote debugging, the inferior won't inherit
gdb's cwd.  Right?

> +
> +  fprintf_filtered (gdb_stdout,
> +		    _("Current working directory that will be used "
> +		      "when starting the inferior is \"%s\".\n"), cwd);
> +}
> +
>  

> index 6d020f73c4..bcd1e54a03 100644
> --- a/gdb/inferior.h
> +++ b/gdb/inferior.h
> @@ -355,6 +355,10 @@ public:
>       should never be freed.  */
>    char **argv = NULL;
>  
> +  /* The current working directory that will be used when starting
> +     this inferior.  */
> +  const char *cwd = NULL;

See comment above in set_inferior_cwd.  Who owns this string?
Looks like removing the inferior leaks this?
Can this be a unique_xmalloc_ptr ?

> @@ -376,6 +377,22 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>  	 UIs.  */
>        close_most_fds ();
>  
> +      const char *cwd = get_inferior_cwd ();
> +
> +      if (cwd != NULL)
> +	{
> +	  TRY
> +	    {
> +	      gdb_chdir (cwd);
> +	    }
> +	  CATCH (ex, RETURN_MASK_ERROR)
> +	    {
> +	      warning ("%s", ex.message);
> +	      _exit (0177);
> +	    }
> +	  END_CATCH
> +	}

This is the fork-child path, and as such we should only be calling
async-signal-safe code.  And throwing C++ exceptions is not async-signal-safe.
I think an easy solution is to call gdb_tilde_expand _before_ forking, and
then call plain old chdir in the child, and then call
trace_start_error_with_name instead of perror_with_name on failure:

  if (chdir (expanded_dir.c_str ()) < 0)
    trace_start_error_with_name (expanded_dir.c_str ());

With that, I'm not sure whether gdb_chdir is still very useful
compared to using gdb_tilde_expand + chdir in the "cd" command too.

> +++ b/gdb/testsuite/gdb.base/set-cwd.c
> @@ -0,0 +1,29 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2017 Free Software Foundation, Inc.
> +
> +   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 <stdio.h>
> +#include <unistd.h>
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  char dir[BUFSIZ];
> +
> +  getcwd (dir, BUFSIZ);

Hmm, BUFSIZ came back?  :-)  Please use something like PATH_MAX
with a fallback to 1024 or some such.

> +
> +  return 0; /* break-here */
> +}
> diff --git a/gdb/testsuite/gdb.base/set-cwd.exp b/gdb/testsuite/gdb.base/set-cwd.exp
> new file mode 100644
> index 0000000000..3a6ffd3862
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/set-cwd.exp
> @@ -0,0 +1,90 @@
> +# This testcase is part of GDB, the GNU debugger.
> +
> +# Copyright 2017 Free Software Foundation, Inc.
> +
> +# 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/>.
> +
> +if { ![isnative] || [is_remote target] || [is_remote host]
> +     || [target_info gdb_protocol] == "extended-remote" } then {
> +    untested "not implemented on gdbserver"

The skip on [is_remote host] surely has nothing to do with gdbserver, right?
Please split that up to a separate untested call with a meaningful gdb.sum
output message.  The rest of the checks don't look exactly right,
but I'll ignore that because surely you'll be changing it in the following
patch.

> +    return


> +}
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } {
> +    return -1
> +}
> +
> +# Test that tilde expansion works fine.
> +
> +proc test_tilde_expansion { } {
> +    if { [info exists ::env(HOME)] } {
> +	with_test_prefix "test tilde expansion" {
> +	    set home $::env(HOME)
> +
> +	    gdb_test_no_output "set cwd ~/test" "set cwd to ~/test dir"
> +
> +	    gdb_test "show cwd" \
> +		"Current working directory that will be used when starting the inferior is \"${home}/test\"\." \
> +		"show cwd shows expanded tilde"
> +	}
> +    }
> +}
> +
> +# Test that when we "set cwd" the inferior will be started under the
> +# correct working directory and GDB will not be affected by this.
> +
> +proc test_cd_into_dir { } {
> +    global decimal gdb_prompt
> +
> +    with_test_prefix "test cd into temp dir" {
> +	gdb_test_multiple "pwd" "pwd before run" {
> +	    -re "Working directory \(.*\)\.\r\n$gdb_prompt $" {
> +		set gdb_cwd_before_run $expect_out(1,string)
> +	    }
> +	}

Is the above fails, then gdb_cwd_before_run is left unset,
and the next reference causes a TCL error.

> +
> +	set tmpdir [standard_output_file ""]
> +
> +	gdb_test_no_output "set cwd $tmpdir" "set cwd to temp dir"
> +
> +	if { ![runto_main] } {
> +	    untested "could not run to main"
> +	    return -1
> +	}
> +
> +	gdb_breakpoint [gdb_get_line_number "break-here"]
> +	gdb_continue_to_breakpoint "break-here" ".* break-here .*"
> +
> +	gdb_test "print dir" "\\\$$decimal = \"$tmpdir\", .*" \
> +	    "inferior cwd is correctly set"
> +
> +	gdb_test_multiple "pwd" "pwd after run" {
> +	    -re "Working directory \(.*\)\.\r\n$gdb_prompt $" {
> +		set gdb_cwd_after_run $expect_out(1,string)
> +	    }
> +	}

Likewise.


> +
> +	set test "GDB cwd is unchanged after running inferior"
> +	if { [string equal $gdb_cwd_before_run $gdb_cwd_after_run] } {
> +	    pass $test
> +	} else {
> +	    fail $test
> +	}

You can use gdb_assert instead of this if/else pass/fail.

Thanks,
Pedro Alves