Re: [PATCH v5 2/3] Implement "set cwd" command on GDB

[Pedro Alves <palves@redhat.com>]

Hi Sergio,

This looks largely OK to me now, though I still have a couple
comments.

On 09/29/2017 11:58 PM, Sergio Durigan Junior wrote:
> @@ -2434,19 +2435,43 @@ 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
> +@kbd{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}.

Rereading this, I think that this paragraph ("The GDB working directory
also serves...") would be better moved below, where "cd" is
documented (with the "also" probably dropped).  Maybe add an xref here taking
users there.

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

Above I meant, move that paragraph here, where "cd" is documented.
This is where I'd expect to see info about what does the command
affect.  Maybe also add an xref to "set cwd" here.

> @@ -374,6 +389,14 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>  	 UIs.  */
>        close_most_fds ();
>  
> +      /* Change to the requested working directory if the user
> +	 requested it.  */
> +      if (inferior_cwd != NULL)
> +	{
> +	  if (chdir (inferior_cwd) < 0)
> +	    trace_start_error_with_name (expanded_inferior_cwd.c_str ());

Please also update the trace_start... statement:

    trace_start_error_with_name (expanded_inferior);


> +proc_with_prefix test_cwd_reset { } {
> +    global decimal gdb_prompt tmpdir
> +
> +    gdb_test_multiple "pwd" "GDB cwd" {
> +	-re "Working directory \(.*\)\.\r\n$gdb_prompt $" {
> +	    set gdb_cwd $expect_out(1,string)
> +	}
> +	-re ".*$gdb_prompt $" {
> +	    fail "failed to obtain GDB cwd before run"
> +	    return -1
> +	}
> +	default {
> +	    fail "failed to obtain GDB cwd before run"
> +	    return -1
> +	}
> +    }

Any reason you didn't update this one?

>  
> +  const char *inferior_cwd = get_inferior_cwd ();
> +  std::string expanded_infcwd;
> +  if (inferior_cwd != NULL)
> +    {
> +      expanded_infcwd = gdb_tilde_expand (inferior_cwd);
> +      /* Mirror slashes on inferior's cwd.  */
> +      std::replace (expanded_infcwd.begin (), expanded_infcwd.end (),
> +		    '/', '\\');
> +      inferior_cwd = expanded_infcwd.c_str ();
> +    }
> +

But what if inferior_cwd _is_ NULL, when ...


>    memset (&si, 0, sizeof (si));
>    si.cb = sizeof (si);
>  
> @@ -2514,6 +2527,10 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>        flags |= DEBUG_PROCESS;
>      }
>  
> +  if (cygwin_conv_path (CCP_POSIX_TO_WIN_W, inferior_cwd,
> +			infcwd, strlen (inferior_cwd)) < 0)
> +    error (_("Error converting inferior cwd: %d"), errno);

... we get here?  It looks to me like this conversion should
skipped here then, and ...

> +
>  #ifdef __USEWIDE
>    args = (cygwin_buf_t *) alloca ((wcslen (toexec) + wcslen (cygallargs) + 2)
>  				  * sizeof (wchar_t));
> @@ -2574,7 +2591,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 */

... here still pass NULL.

>  		       &si,
>  		       &pi);
>    if (w32_env)
> @@ -2697,7 +2714,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)
> 

Thanks,
Pedro Alves