From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 63928 invoked by alias); 29 Sep 2017 18:31:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 63909 invoked by uid 89); 29 Sep 2017 18:31:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 29 Sep 2017 18:31:50 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D03D8820E3; Fri, 29 Sep 2017 18:31:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com D03D8820E3 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=sergiodj@redhat.com Received: from localhost (unused-10-15-17-193.yyz.redhat.com [10.15.17.193]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8A59898168; Fri, 29 Sep 2017 18:31:48 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Eli Zaretskii Subject: Re: [PATCH v4 2/3] Implement "set cwd" command on GDB References: <20170912042325.14927-1-sergiodj@redhat.com> <20170928041046.5468-1-sergiodj@redhat.com> <20170928041046.5468-3-sergiodj@redhat.com> <1d556986-1f95-c9d3-6fe5-0e8aeb9c9292@redhat.com> Date: Fri, 29 Sep 2017 18:31:00 -0000 In-Reply-To: <1d556986-1f95-c9d3-6fe5-0e8aeb9c9292@redhat.com> (Pedro Alves's message of "Fri, 29 Sep 2017 16:19:59 +0100") Message-ID: <878tgxjqzw.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00917.txt.bz2 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 . */ >> + >> +#include > > This include doesn't look necessary. Hm. Removed. >> +#include >> +#include >> + >> +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 . >> + >> +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' 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/