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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Tuesday, October 03 2017, Pedro Alves wrote:

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

Done.

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

Done.

>> @@ -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);
>

I could swear I had updated this.  Anyway, did that now.

>
>> +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?

That's strange.  I also could swear I updated this one.  I'm starting to
use a new tool to manage my git repos, so maybe I made a confusion and
reverted some changes.

Updated.

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

You're right, this should be skipped.

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

Here we pass NULL because that was the old behaviour when we did not
care about the inferior's cwd.  This is the same approach that we use on
fork_inferior: if the user hasn't provided any paths via "set cwd", then
we don't do anything.

>>  		       &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,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/