This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 09/28/2017 05:10 AM, Sergio Durigan Junior wrote:
> This is the actual implementation of the "set/show cwd" commands on
> GDB.  The way they work is:

(This is not a huge deal, but note that this is
assuming the reader first read the cover letter, but the
cover letter doesn't make it to git and won't be available
to whoever looks ends up looking at the commit while doing
some archaeology.)

> 
> - 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.
> 
> This way, we'll make sure that GDB's cwd is not affected by the user
> set cwd.
> 

> @@ -339,6 +342,13 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs,
>       the parent and child flushing the same data after the fork.  */
>    gdb_flush_out_err ();
>  
> +  /* Check if the user wants to set a different working directory for
> +     the inferior.  */
> +  inferior_cwd = get_inferior_cwd ();
> +
> +  if (inferior_cwd != NULL)
> +    expanded_inferior_cwd = gdb_tilde_expand (inferior_cwd);

Please add something like:

/* Expand before forking because between fork and exec, the child
   process may only execute async-signal-safe operations.  */

I think it'd look clearer to do:

  inferior_cwd = get_inferior_cwd ();

  if (inferior_cwd != NULL)
    {
      expanded_inferior_cwd = gdb_tilde_expand (inferior_cwd);
      inferior_cwd = expanded_inferior_cwd.c_str ();
    }

here and ...

> +
>    /* If there's any initialization of the target layers that must
>       happen to prepare to handle the child we're about fork, do it
>       now...  */
> @@ -374,6 +384,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 (expanded_inferior_cwd.c_str ()) < 0)
> +	    trace_start_error_with_name (expanded_inferior_cwd.c_str ());
> +	}

... then here do:

      if (inferior_cwd != NULL)
	{
	  if (chdir (inferior_cwd) < 0)
	    trace_start_error_with_name (inferior_cwd);
	}

Or even:

      if (inferior_cwd != NULL && chdir (inferior_cwd) < 0)
        trace_start_error_with_name (inferior_cwd);


> +++ b/gdb/testsuite/gdb.base/set-cwd.c
> @@ -0,0 +1,32 @@
> +/* 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>

This include doesn't look necessary.

> +#include <stdlib.h>
> +#include <unistd.h>
> +
> +static char dir[4096];
> +
> +int
> +main (int argc, char *argv[])
> +{
> +  const char *home = getenv ("HOME");
> +
> +  getcwd (dir, 4096);
> +
> +  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..3d666ba18d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/set-cwd.exp
> @@ -0,0 +1,206 @@
> +# 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 { [use_gdb_stub] || [target_info gdb_protocol] == "extended-remote" } {
> +    untested "not implemented on gdbserver"

The untested message still refers to gdbserver.  OK, I see that
you're changing it in the next 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 { } {
> +    global decimal gdb_prompt hex
> +
> +    with_test_prefix "test tilde expansion" {

Note you can use proc_with_prefix to avoid having to
write procs with nested scopes.

> +	gdb_test_no_output "set cwd ~/" "set inferior cwd to ~/ 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_multiple "print home" "print home var" {
> +	    -re "\\\$$decimal = $hex \"\(.+\)\"\r\n$gdb_prompt $" {
> +		set home $expect_out(1,string)
> +	    }
> +	    -re "$gdb_prompt $" {
> +		untested "could to retrieve home var"
> +		return

"could not" I assume.

> +	    }
> +	    default {
> +		untested "could to retrieve home var"
> +		return
> +	    }
> +	}

I'd rather this was written like this:

        set home ""
	set test "print home var"
	gdb_test_multiple "print home" $test {
	    -re "\\\$$decimal = $hex \"\(.+\)\"\r\n$gdb_prompt $" {
		set home $expect_out(1,string)
		pass $test
	    }
	}

        if {$home == ""} {
	  untested "could not retrieve home var"
          return
        }

Because this way, you automatically get the usual:
   FAIL: print home var (timeout)
or:
   FAIL: print home var (eof)

which your version suppresses, followed by:
   UNTESTED: could to retrieve home var

And in spirit of always having matching pass/fails, note
there's a pass call above.

Actually, you can probably just use get_valueof for this:

        set home [get_valueof "" "home" "" "retrieve home var"]
        if {$home == ""} {
	  untested "could not retrieve home var"
          return
        }

> +
> +	if { [string length $home] > 0 } {

This check can then go away.

> +	    gdb_test_multiple "print dir" "print dir var" {
> +		-re "\\\$$decimal = \"\(.+\)\"\(, .*repeats.*\)?\r\n$gdb_prompt $" {
> +		    set curdir $expect_out(1,string)
> +		}
> +		-re "$gdb_prompt $" {
> +		    fail "failed to retrieve dir var"
> +		    return -1
> +		}
> +		default {
> +		    fail "failed to retrieve dir var"
> +		    return -1
> +		}
> +	    }

And here you'd apply the same pattern.



> +
> +	    gdb_assert [string equal $curdir $home] \
> +		"successfully chdir'd into home"
> +	} else {
> +	    untested "could not determine value of HOME"
> +	    return
> +	}
> +    }
> +}
> +
> +# The temporary directory that we will use to start the inferior.
> +set tmpdir [standard_output_file ""]
> +
> +# 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 tmpdir
> +
> +    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)
> +	    }
> +	    -re ".*$gdb_prompt $" {
> +		fail "failed to obtain GDB cwd before run"
> +		return -1
> +	    }
> +	    default {
> +		fail "failed to obtain GDB cwd before run"
> +		return -1
> +	    }
> +	}

Ditto.  (This appears multiple times throughout.)

> +
> +	# This test only makes sense if $tmpdir != $gdb_cwd_before_run
> +	if { ![gdb_assert ![string equal $tmpdir $gdb_cwd_before_run] \
> +		   "make sure that tmpdir and GDB's cwd are different"] } {
> +	    return -1
> +	}
> +
> +	gdb_test_no_output "set cwd $tmpdir" "set inferior 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)
> +	    }
> +	    -re ".*$gdb_prompt $" {
> +		fail "failed to obtain GDB cwd after run"
> +		return -1
> +	    }
> +	    default {
> +		fail "failed to obtain GDB cwd after run"
> +		return -1
> +	    }
> +	}
> +
> +	gdb_assert [string equal $gdb_cwd_before_run $gdb_cwd_after_run] \
> +	    "GDB cwd is unchanged after running inferior"
> +    }
> +}
> +
> +# Test that executing "set cwd" without arguments will reset the
> +# inferior's cwd setting to its previous state.
> +
> +proc test_cwd_reset { } {
> +    global decimal gdb_prompt tmpdir
> +
> +    with_test_prefix "test inferior cwd reset" {
> +	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
> +	    }
> +	}
> +
> +	# This test only makes sense if $tmpdir != $gdb_cwd
> +	# This test only makes sense if $tmpdir != $gdb_cwd

Duplicate comment.  And missing period.

> +	if { ![gdb_assert ![string equal $tmpdir $gdb_cwd] \
> +		   "make sure that tmpdir and GDB's cwd are different"] } {
> +	    return -1
> +	}
> +
> +	gdb_test_no_output "set cwd $tmpdir" "set inferior 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"
> +
> +	# Reset the inferior's cwd.
> +	gdb_test_no_output "set cwd" "resetting inferior cwd"
> +
> +	if { ![runto_main] } {
> +	    untested "could not run to main"
> +	    return -1
> +	}

Wrap each runto_main invocation in its own with_test_prefix,
so that if any fails we get unique test names in gdb.sum.

> +
> +	gdb_breakpoint [gdb_get_line_number "break-here"]
> +	gdb_continue_to_breakpoint "break-here" ".* break-here .*"
> +
> +	gdb_test "print dir" "\\\$$decimal = \"$gdb_cwd\", .*" \
> +	    "inferior cwd got reset correctly"
> +    }
> +}
> +
> +test_cd_into_dir
> +clean_restart $binfile
> +test_tilde_expansion
> +clean_restart $binfile
> +test_cwd_reset
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 3e1894410d..5144e9f7a6 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -66,6 +66,7 @@
>  #include "x86-nat.h"
>  #include "complaints.h"
>  #include "inf-child.h"
> +#include "gdb_tilde_expand.h"
>  
>  #define AdjustTokenPrivileges		dyn_AdjustTokenPrivileges
>  #define DebugActiveProcessStop		dyn_DebugActiveProcessStop
> @@ -2432,6 +2433,7 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>    cygwin_buf_t *toexec;
>    cygwin_buf_t *cygallargs;
>    cygwin_buf_t *args;
> +  cygwin_buf_t *infcwd;
>    char **old_env = NULL;
>    PWCHAR w32_env;
>    size_t len;
> @@ -2461,6 +2463,8 @@ 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 ();
> +  std::string expanded_infcwd = gdb_tilde_expand (inferior_cwd);

Does gdb_tilde_expand work with NULL input ?

>  
>    if (!exec_file)
>      error (_("No executable specified, use `target exec'."));
> @@ -2514,6 +2518,10 @@ windows_create_inferior (struct target_ops *ops, const char *exec_file,
>        flags |= DEBUG_PROCESS;
>      }
>  
> +  if (cygwin_conv_path (CCP_POSIX_TO_WIN_W, expanded_infcwd.c_str (),
> +			infcwd, expanded_infcwd.size ()) < 0)
> +    error (_("Error converting inferior cwd: %d"), errno);

I don't see how this can work.  'infcwd' was never initialized to
point at some buffer?  It's garbage.  I think you're supposed to
call this twice, once to determine the needed size, and then
another to fill in a buffer of that size.

Thanks,
Pedro Alves


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]