Re: [PATCH v3 4/5] Implement "set cwd" command on GDB

[Eli Zaretskii <eliz at gnu dot org>]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>,	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu, 21 Sep 2017 18:59:25 -0400
> 
> This is the actual implementation of the "set/show cwd" commands on
> GDB.  The way they work is:
> 
> - If the user sets the inferior's cwd by using "set cwd", then this
>   directory is saved into current_inferior ()->cwd and is used when
>   the inferior is started (see below).
> 
> - If the user doesn't set the inferior's cwd by using "set cwd", but
>   rather use the "cd" command as before, then this directory is
>   inherited by the inferior because GDB will have chdir'd into it.
> 
> On Unix-like hosts, the way the directory is changed before the
> inferior execution is by expanding the user set directory before the
> fork, and then "chdir" after the call to fork/vfork on
> "fork_inferior", but before the actual execution.  On Windows, the
> inferior cwd set by the user is passed directly to the CreateProcess
> call, which takes care of the actual chdir for us.

Thanks, I have some comments.

> diff --git a/gdb/NEWS b/gdb/NEWS
> index 549f511b29..c131713293 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -64,6 +64,9 @@ QStartupWithShell
>  
>  * New commands
>  
> +set|show cwd
> +  Set and show the current working directory for the inferior.
> +
>  set|show compile-gcc
>    Set and show compilation command used for compiling and injecting code
>    with the 'compile' commands.

This part is OK.

> +@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{'~'}.

I think we should document here what does "~" mean on MS-Windows,
especially since, when HOME is not in the environment, Gnulib's glob
module doesn't behave according to MS platform recommendations (which
say not to create files directly below %HOMEDRIVE%%HOMEPATH%).

More generally, I think we should say here that the argument is
glob-expanded, because this is user-visible behavior (right?).  Also,
how will TAB-completion react to input of this command? will it expand
the input typed so far?

> +@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.

Does this show the original value typed by the user, or the expanded
value?  E.g., if the user types "set cwd ~/foo", what will "show cwd"
display?  If it shows the unexpanded form, does that mean the actual
cwd will change if, say, HOME changes?

Should we store the cwd after tilde-expansion?

> @@ -2461,6 +2462,7 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>    BOOL ret;
>    DWORD flags = 0;
>    const char *inferior_io_terminal = get_inferior_io_terminal ();
> +  const char *inferior_cwd = get_inferior_cwd ();
>  
>    if (!exec_file)
>      error (_("No executable specified, use `target exec'."));
> @@ -2488,8 +2490,15 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>  	error (_("Error starting executable: %d"), errno);
>        cygallargs = (wchar_t *) alloca (len * sizeof (wchar_t));
>        mbstowcs (cygallargs, allargs, len);
> +
> +      len = mbstowcs (NULL, inferior_cwd, 0) + 1;
> +      if (len == (size_t) -1)
> +	error (_("Invalid cwd for inferior: %d"), errno);
> +      infcwd = (wchar_t *) alloca (len * sizeof (wchar_t));
> +      mbstowcs (infcwd, inferior_cwd, len);
>  #else  /* !__USEWIDE */
>        cygallargs = allargs;
> +      infcwd = (cygwin_buf_t *) inferior_cwd;
>  #endif
>      }
>    else
> @@ -2574,7 +2583,7 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>  		       TRUE,	/* inherit handles */
>  		       flags,	/* start flags */
>  		       w32_env,	/* environment */
> -		       NULL,	/* current directory */
> +		       infcwd,	/* current directory */
>  		       &si,
>  		       &pi);
>    if (w32_env)
> @@ -2697,7 +2706,7 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>  			TRUE,	/* inherit handles */
>  			flags,	/* start flags */
>  			w32env,	/* environment */
> -			NULL,	/* current directory */
> +			inferior_cwd,	/* current directory */
>  			&si,
>  			&pi);
>    if (tty != INVALID_HANDLE_VALUE)

This seems to pass the unexpanded cwd directly to CreateProcess.  I
don't think this will work on Windows, as this directory is not
interpreted by any shell, so "~" will cause errors.  I think we should
pass this via gdb_tilde_expand, like we do in the Unix case, and I
also think we should mirror all the slashes in the result, just in
case.