[PATCH v4 2/3] Implement "set cwd" command on GDB
Sergio Durigan Junior
sergiodj@redhat.com
Fri Sep 29 18:31:00 GMT 2017
On Friday, September 29 2017, Pedro Alves wrote:
> 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.)
I can improve the message here, and add more context.
>> - 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 ();
> }
Added the comment and assigned to inferior_cwd.
> 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);
> }
I prefer this way, so I updated the code to look like this.
> 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.
Hm. Removed.
>> +#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.
I'll change it to say "not implemented on remote servers".
>> + 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.
Didn't know about that. Thanks.
>> + 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.
Ops, yes. Fixed.
>> + }
>> + 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.
Hm, OK. Changed the code to reflect the proposed changes.
> 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
> }
I had tried using "get_valueof" even before I submitted the first
version of the patch, but it doesn't really work "out of the box". For
example, the output of "print home" (or "print/s home") is:
(gdb) print home
$1 = 0x7fffffffe703 "/home/sergio"
And "get_valueof" doesn't handle the address part very well, so $home
ends up with the value '0x7fffffffe703 "/home/sergio"'. A similar
situation happens with the "dir" variable, which is printed as:
(gdb) print dir
$1 = "/home/sergio/work/src/git/binutils-gdb/set-cwd-command/build-64", '\000' <repeats 4032 times>
I remember being able to workaround these limitations with
"get_valueof", but in the end I decided it was better to just use
"gdb_test_multiple".
>> +
>> + if { [string length $home] > 0 } {
>
> This check can then go away.
Done.
>> + 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.
Done.
>
>
>> +
>> + 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.)
Updated this and the other instance of this pattern.
>> +
>> + # 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.
Fixed.
>> + 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.
OK.
>> +
>> + 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 ?
You're right, I should check for NULL here as well.
>>
>> 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.
You're right. I see that there are other paths being declared in this
function:
...
cygwin_buf_t real_path[__PMAX];
cygwin_buf_t shell[__PMAX]; /* Path to shell */
...
So I think a better way is to follow the convention and declare "infcwd"
with "__PMAX" size. That's what I did.
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/
More information about the Gdb-patches
mailing list