From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117050 invoked by alias); 20 Sep 2017 14:01:31 -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 117030 invoked by uid 89); 20 Sep 2017 14:01:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=owns, TCL, HOME, TRY 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; Wed, 20 Sep 2017 14:01:19 +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 08428C00DB8E for ; Wed, 20 Sep 2017 14:01:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 08428C00DB8E Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id ECA875E1A5; Wed, 20 Sep 2017 14:01:13 +0000 (UTC) Subject: Re: [PATCH v2 4/5] Implement "set cwd" command To: Sergio Durigan Junior , GDB Patches References: <20170912042325.14927-1-sergiodj@redhat.com> <20170919042842.9210-1-sergiodj@redhat.com> <20170919042842.9210-5-sergiodj@redhat.com> From: Pedro Alves Message-ID: <30f117dc-8bda-67f5-341b-a50b23cfb7a2@redhat.com> Date: Wed, 20 Sep 2017 14:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20170919042842.9210-5-sergiodj@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-09/txt/msg00488.txt.bz2 On 09/19/2017 05:28 AM, Sergio Durigan Junior wrote: > diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c > index 653dd56a64..f086f10aea 100644 > --- a/gdb/cli/cli-cmds.c > +++ b/gdb/cli/cli-cmds.c > @@ -1716,9 +1716,9 @@ The commands below can be used to select other frames by number or address."), > Print working directory. This is used for your program as well.")); > > c = add_cmd ("cd", class_files, cd_command, _("\ > -Set working directory to DIR for debugger and program being debugged.\n\ > -The change does not take effect for the program being debugged\n\ > -until the next time it is started."), &cmdlist); > +Set working directory to DIR for debugger.\n\ I think it'd be nice to add a sentence here suggesting what the current working directory affects. > +In order to change the inferior's current working directory, the recommended\n\ > +way is to use the \"set cwd\" command."), &cmdlist); > set_cmd_completer (c, filename_completer); > > add_com ("echo", class_support, echo_command, _("\ > diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h > index 87c13009ed..515a8c0f4e 100644 > --- a/gdb/common/common-inferior.h > +++ b/gdb/common/common-inferior.h > @@ -30,4 +30,8 @@ extern const char *get_exec_wrapper (); > otherwise return 0 in that case. */ > extern char *get_exec_file (int err); > > +/* Return the inferior's current working directory. If nothing has > + been set, then return NULL. */ > +extern const char *get_inferior_cwd (); > + > #endif /* ! COMMON_INFERIOR_H */ > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index af5fe6f46c..c513a49c26 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -2057,8 +2057,9 @@ environment} to change parts of the environment that affect > your program. @xref{Environment, ,Your Program's Environment}. > > @item The @emph{working directory.} > -Your program inherits its working directory from @value{GDBN}. You can set > -the @value{GDBN} working directory with the @code{cd} command in @value{GDBN}. > +You can set your program's working directory with the command > +@code{set cwd}. If you do not set any working directory with this > +command, your program will inherit @value{GDBN}'s working directory. > @xref{Working Directory, ,Your Program's Working Directory}. > > @item The @emph{standard input and output.} > @@ -2424,19 +2425,36 @@ 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 > +@code{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}. > > @table @code > +@kindex set cwd > +@cindex change inferior's working directory > +@item set cwd @r{[}@var{directory}@r{]} > +Set the inferior's working directory to @var{directory}. If not > +given, @var{directory} uses @file{'~'}. This has no effect on > +@value{GDBN}'s working directory. I think we're missing a sentence somewhere in the manual saying that this setting only takes effect the next time you start the program, like it is said in "help cd" currently. > + > +@kindex show cwd > +@cindex show inferior's working directory > +@item show cwd > +Show the inferior's working directory. If no directory has been > +specified by @code{set cwd}, then the default inferior's working > +directory is the same as @value{GDBN}'s working directory. > + > @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{'~'}. > diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c > index 72f0412757..e78ad4faf1 100644 > --- a/gdb/gdbserver/inferiors.c > +++ b/gdb/gdbserver/inferiors.c > @@ -29,6 +29,9 @@ struct thread_info *current_thread; > > #define get_thread(inf) ((struct thread_info *)(inf)) > > +/* The current working directory used to start the inferior. */ > +static const char *current_inferior_cwd = NULL; > + > void > add_inferior_to_list (struct inferior_list *list, > struct inferior_list_entry *new_inferior) > @@ -445,3 +448,11 @@ switch_to_thread (ptid_t ptid) > if (!ptid_equal (ptid, minus_one_ptid)) > current_thread = find_thread_ptid (ptid); > } > + > +/* See common/common-inferior.h. */ > + > +const char * > +get_inferior_cwd () > +{ > + return current_inferior_cwd; > +} > diff --git a/gdb/infcmd.c b/gdb/infcmd.c > index 25cf025426..178a3ca15f 100644 > --- a/gdb/infcmd.c > +++ b/gdb/infcmd.c > @@ -59,6 +59,8 @@ > #include "top.h" > #include "interps.h" > #include "common/gdb_optional.h" > +#include "gdb_chdir.h" > +#include "readline/tilde.h" > > /* Local functions: */ > > @@ -111,6 +113,10 @@ static void run_command (char *, int); > > static char *inferior_args_scratch; > > +/* Scratch area where the new cwd will be stored by 'set cwd'. */ > + > +static char *inferior_cwd_scratch; > + > /* Scratch area where 'set inferior-tty' will store user-provided value. > We'll immediate copy it into per-inferior storage. */ > > @@ -246,6 +252,56 @@ show_args_command (struct ui_file *file, int from_tty, > deprecated_show_value_hack (file, from_tty, c, get_inferior_args ()); > } > > +/* Set the inferior current working directory. This directory will be > + entered by GDB before executing the inferior. */ The second sentence is talking about implementation details of Unix-like targets, which doesn't really belong here. > + > +static void > +set_inferior_cwd (const char *cwd) > +{ > + struct inferior *inf = current_inferior (); > + > + gdb_assert (inf != NULL); > + xfree ((void *) inf->cwd); > + inf->cwd = tilde_expand (*cwd != '\0' ? cwd : "~"); So the inferior owns the string? Wouldn't it be better if inf->cwd were a unique_ptr? I don't see anywhere releasing the cwd string when an inferior is deleted. > + > +/* Handle the 'show cwd' command. */ > + > +static void > +show_cwd_command (struct ui_file *file, int from_tty, > + struct cmd_list_element *c, const char *value) > +{ > + const char *cwd = get_inferior_cwd (); > + > + if (cwd == NULL) > + { > + /* To maintain backwards compatibility, we use > + 'current_directory' here, which is set by the "cd" > + command. */ > + cwd = current_directory; > + } This doesn't look like the right thing to show, to me. It'll only make sense for native debugging, not remote debugging, right? I mean, when remote debugging, the inferior won't inherit gdb's cwd. Right? > + > + fprintf_filtered (gdb_stdout, > + _("Current working directory that will be used " > + "when starting the inferior is \"%s\".\n"), cwd); > +} > + > > index 6d020f73c4..bcd1e54a03 100644 > --- a/gdb/inferior.h > +++ b/gdb/inferior.h > @@ -355,6 +355,10 @@ public: > should never be freed. */ > char **argv = NULL; > > + /* The current working directory that will be used when starting > + this inferior. */ > + const char *cwd = NULL; See comment above in set_inferior_cwd. Who owns this string? Looks like removing the inferior leaks this? Can this be a unique_xmalloc_ptr ? > @@ -376,6 +377,22 @@ fork_inferior (const char *exec_file_arg, const std::string &allargs, > UIs. */ > close_most_fds (); > > + const char *cwd = get_inferior_cwd (); > + > + if (cwd != NULL) > + { > + TRY > + { > + gdb_chdir (cwd); > + } > + CATCH (ex, RETURN_MASK_ERROR) > + { > + warning ("%s", ex.message); > + _exit (0177); > + } > + END_CATCH > + } This is the fork-child path, and as such we should only be calling async-signal-safe code. And throwing C++ exceptions is not async-signal-safe. I think an easy solution is to call gdb_tilde_expand _before_ forking, and then call plain old chdir in the child, and then call trace_start_error_with_name instead of perror_with_name on failure: if (chdir (expanded_dir.c_str ()) < 0) trace_start_error_with_name (expanded_dir.c_str ()); With that, I'm not sure whether gdb_chdir is still very useful compared to using gdb_tilde_expand + chdir in the "cd" command too. > +++ b/gdb/testsuite/gdb.base/set-cwd.c > @@ -0,0 +1,29 @@ > +/* 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 > +#include > + > +int > +main (int argc, char *argv[]) > +{ > + char dir[BUFSIZ]; > + > + getcwd (dir, BUFSIZ); Hmm, BUFSIZ came back? :-) Please use something like PATH_MAX with a fallback to 1024 or some such. > + > + 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..3a6ffd3862 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/set-cwd.exp > @@ -0,0 +1,90 @@ > +# 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 { ![isnative] || [is_remote target] || [is_remote host] > + || [target_info gdb_protocol] == "extended-remote" } then { > + untested "not implemented on gdbserver" The skip on [is_remote host] surely has nothing to do with gdbserver, right? Please split that up to a separate untested call with a meaningful gdb.sum output message. The rest of the checks don't look exactly right, but I'll ignore that because surely you'll be changing it in the following 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 { } { > + if { [info exists ::env(HOME)] } { > + with_test_prefix "test tilde expansion" { > + set home $::env(HOME) > + > + gdb_test_no_output "set cwd ~/test" "set cwd to ~/test dir" > + > + gdb_test "show cwd" \ > + "Current working directory that will be used when starting the inferior is \"${home}/test\"\." \ > + "show cwd shows expanded tilde" > + } > + } > +} > + > +# 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 > + > + 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) > + } > + } Is the above fails, then gdb_cwd_before_run is left unset, and the next reference causes a TCL error. > + > + set tmpdir [standard_output_file ""] > + > + gdb_test_no_output "set cwd $tmpdir" "set 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) > + } > + } Likewise. > + > + set test "GDB cwd is unchanged after running inferior" > + if { [string equal $gdb_cwd_before_run $gdb_cwd_after_run] } { > + pass $test > + } else { > + fail $test > + } You can use gdb_assert instead of this if/else pass/fail. Thanks, Pedro Alves