From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61130 invoked by alias); 3 Oct 2017 16:39:30 -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 61119 invoked by uid 89); 3 Oct 2017 16:39:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-5.9 required=5.0 tests=BAYES_00,GIT_PATCH_1,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=swear 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; Tue, 03 Oct 2017 16:39:28 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C91BD82114; Tue, 3 Oct 2017 16:39:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C91BD82114 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 8C50E62684; Tue, 3 Oct 2017 16:39:25 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches , Eli Zaretskii Subject: Re: [PATCH v5 2/3] Implement "set cwd" command on GDB References: <20170912042325.14927-1-sergiodj@redhat.com> <20170929225852.21872-1-sergiodj@redhat.com> <20170929225852.21872-3-sergiodj@redhat.com> Date: Tue, 03 Oct 2017 16:39:00 -0000 In-Reply-To: (Pedro Alves's message of "Tue, 3 Oct 2017 16:15:15 +0100") Message-ID: <874lrg5goz.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-10/txt/msg00054.txt.bz2 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/