From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126081 invoked by alias); 27 Sep 2017 21:48:41 -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 124339 invoked by uid 89); 27 Sep 2017 21:48:40 -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; Wed, 27 Sep 2017 21:48:37 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C6E5319638B for ; Wed, 27 Sep 2017 21:48:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com C6E5319638B 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 804988E83D; Wed, 27 Sep 2017 21:48:18 +0000 (UTC) From: Sergio Durigan Junior To: Pedro Alves Cc: GDB Patches Subject: Re: [PATCH v3 5/5] Extend "set cwd" to work on gdbserver References: <20170912042325.14927-1-sergiodj@redhat.com> <20170921225926.23132-1-sergiodj@redhat.com> <20170921225926.23132-6-sergiodj@redhat.com> Date: Wed, 27 Sep 2017 21:48:00 -0000 In-Reply-To: (Pedro Alves's message of "Wed, 27 Sep 2017 15:42:04 +0100") Message-ID: <877ewjak4d.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/msg00842.txt.bz2 On Wednesday, September 27 2017, Pedro Alves wrote: > On 09/21/2017 11:59 PM, Sergio Durigan Junior wrote: >> This is the "natural" extension necessary for the "set cwd" command >> (and the whole "set the inferior's cwd" logic) to work on gdbserver. >> >> The idea here is to have a new remote packet, QSetWorkingDir (name >> adopted from LLDB's extension to the RSP, as can be seen at >> ), >> which sends an hex-encoded string representing the working directory >> that gdbserver is supposed to cd into before executing the inferior. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Unix implementation detail. Rewrote as: [...] The idea here is to have a new remote packet, QSetWorkingDir (name adopted from LLDB's extension to the RSP, as can be seen at ), which sends an hex-encoded string representing the working directory that the remote inferior will use. For UNIX-like targets this feature is already implemented on nat/fork-inferior.c, and all gdbserver has to do is to basically implement "set_inferior_cwd" and call it whenever such packet arrives. For other targets, like Windows, it is possible to use the existing "get_inferior_cwd" function and do the necessary steps to make sure that the inferior will use the specified working directory. [...] >> The good thing is that since this feature is already implemented on >> nat/fork-inferior.c, all gdbserver has to do is to basically implement >> "set_inferior_cwd" and call it whenever such packet arrives. > > Unix implementation detail. That's not how it works for Windows. See above. >> >> Aside from that, the patch consists basically of updates to the >> testcase (making it available on remote targets) and the >> documentation. >> >> No regressions found. >> >> gdb/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> >> * NEWS (Changes since GDB 8.0): Add entry about new >> 'set-cwd-on-gdbserver' feature. >> (New remote packets): Add entry for QSetWorkingDir. >> * common/common-inferior.h (set_inferior_cwd): New prototype. >> * infcmd.c (set_inferior_cwd): Remove "static". >> (show_cwd_command): Expand text to include remote debugging. >> * remote.c: Add PACKET_QSetWorkingDir. >> (remote_protocol_features) : New entry for >> PACKET_QSetWorkingDir. >> (extended_remote_set_inferior_cwd): New function. >> (extended_remote_create_inferior): Call >> "extended_remote_set_inferior_cwd". >> (_initialize_remote): Call "add_packet_config_cmd" for >> QSetWorkingDir. >> >> gdb/gdbserver/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> >> * inferiors.c (set_inferior_cwd): New function. >> * server.c (handle_general_set): Handle QSetWorkingDir packet. >> (handle_query): Inform that QSetWorkingDir is supported. >> * win32-low.c (create_process): Pass the inferior's cwd to >> CreateProcess. >> >> gdb/testsuite/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> >> * gdb.base/set-cwd.exp: Make it available on gdbserver. >> >> gdb/doc/ChangeLog: >> yyyy-mm-dd Sergio Durigan Junior >> >> * gdb.texinfo (Starting your Program) : >> Mention remote debugging. >> (Working Directory) : >> Likewise. >> (Connecting) : Add "set-working-dir" >> and "QSetWorkingDir" to the table. >> (Remote Protocol) : New item, explaining the >> packet. >> --- >> gdb/NEWS | 12 ++++++++++++ >> gdb/common/common-inferior.h | 3 +++ >> gdb/doc/gdb.texinfo | 39 +++++++++++++++++++++++++++++++++++--- >> gdb/gdbserver/inferiors.c | 9 +++++++++ >> gdb/gdbserver/server.c | 18 +++++++++++++++++- >> gdb/gdbserver/win32-low.c | 15 ++++++++++++--- >> gdb/infcmd.c | 7 ++++--- >> gdb/remote.c | 37 ++++++++++++++++++++++++++++++++++++ >> gdb/testsuite/gdb.base/set-cwd.exp | 11 +++++++++-- >> 9 files changed, 139 insertions(+), 12 deletions(-) >> >> diff --git a/gdb/NEWS b/gdb/NEWS >> index c131713293..4ac340eeb5 100644 >> --- a/gdb/NEWS >> +++ b/gdb/NEWS >> @@ -23,6 +23,14 @@ >> >> * New features in the GDB remote stub, GDBserver >> >> + ** GDBserver is now able to set the inferior's current working >> + directory. >> + >> + The user can set the desired working directory to be used by the >> + remote inferior on GDB, using the new "set cwd" command, which >> + will instruct GDB to tell GDBserver about this directory change >> + the next time an inferior is run. >> + >> ** New "--selftest" command line option runs some GDBserver self >> tests. These self tests are disabled in releases. >> >> @@ -56,6 +64,10 @@ QEnvironmentReset >> QStartupWithShell >> Indicates whether the inferior must be started with a shell or not. >> >> +QSetWorkingDir >> + Tell GDBserver that the inferior to be started should use a specific >> + working directory. >> + >> * The "maintenance print c-tdesc" command now takes an optional >> argument which is the file name of XML target description. >> >> diff --git a/gdb/common/common-inferior.h b/gdb/common/common-inferior.h >> index 515a8c0f4e..26acddc84a 100644 >> --- a/gdb/common/common-inferior.h >> +++ b/gdb/common/common-inferior.h >> @@ -34,4 +34,7 @@ extern char *get_exec_file (int err); >> been set, then return NULL. */ >> extern const char *get_inferior_cwd (); >> >> +/* Set the inferior current working directory. */ >> +extern void set_inferior_cwd (const char *cwd); >> + >> #endif /* ! COMMON_INFERIOR_H */ >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo >> index 899afb92b6..7434de2a68 100644 >> --- a/gdb/doc/gdb.texinfo >> +++ b/gdb/doc/gdb.texinfo >> @@ -2059,8 +2059,10 @@ your program. @xref{Environment, ,Your Program's Environment}. >> @item The @emph{working directory.} >> 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}. >> +command, your program will inherit @value{GDBN}'s working directory if >> +native debugging, or @code{gdbserver}'s working directory if remote >> +debugging. @xref{Working Directory, ,Your Program's Working >> +Directory}. > > Elsewhere in the manual we say "the remote server" instead of @code{gdbserver}, > because not all remote debugging uses gdbserver. Updated. >> >> @item The @emph{standard input and output.} >> Your program normally uses the same device for standard input and >> @@ -2439,7 +2441,9 @@ 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. >> +directory as its working directory if native debugging, or it will >> +inherit @code{gdbserver}'s current working directory if remote >> +debugging. > > Ditto. Updated. >> >> You can also change @value{GDBN}'s current working directory by using >> the @code{cd} command. >> @@ -20993,6 +20997,10 @@ are: >> @tab @code{QEnvironmentReset} >> @tab @code{Reset the inferior environment (i.e., unset user-set variables)} >> >> +@item @code{set-working-dir} >> +@tab @code{QSetWorkingDir} >> +@tab @code{set cwd} >> + >> @item @code{conditional-breakpoints-packet} >> @tab @code{Z0 and Z1} >> @tab @code{Support for target-side breakpoint condition evaluation} >> @@ -36781,6 +36789,28 @@ Reply: >> @table @samp >> @item OK >> The request succeeded. >> + >> +@item QSetWorkingDir:@var{hex-value} > > I think it'd be clearer to say @var{directory}, and > then say below that @var{directory} is an hex-encoded > string, like you're already doing. That's what we do > elsewhere in the manual. Sure. Updated > >> +@anchor{QSetWorkingDir packet} >> +@cindex set working directory, remote request >> +@cindex @samp{QSetWorkingDir} packet >> +This packet is used to inform @command{gdbserver} of the intended > > Ditto re. "gdbserver". Updated. >> +current working directory for programs that are going to be executed. >> + >> +The packet is composed by @var{hex-value}, an hex encoded >> +representation of the directory to be entered by @command{gdbserver}. >> + >> +This packet is only transmitted when the user issues a @code{set cwd} >> +command in @value{GDBN} (@pxref{Working Directory, ,Your Program's >> +Working Directory}). > > "This packet is only transmitted when set cwd" suggests to me that > the packet is sent immediately when the user types "set cwd". > > This packet is only transmitted if the user explicitly > specifies a directory with the @kdb{set cwd} command. Rewrote it. > note: > s/when/if/. > s/@code/@kdb It's @kbd (from "keyboard"). But I know, our muscle memory wants us to type "db" :-P. Even though I slightly disagree here (commands are not keybindings), I found that this is indeed what GDB uses. Anyway, using @kbd now. >> diff --git a/gdb/gdbserver/inferiors.c b/gdb/gdbserver/inferiors.c >> index e78ad4faf1..326d01f4d9 100644 >> --- a/gdb/gdbserver/inferiors.c >> +++ b/gdb/gdbserver/inferiors.c >> @@ -456,3 +456,12 @@ get_inferior_cwd () >> { >> return current_inferior_cwd; >> } >> + >> +/* See common/common-inferior.h. */ >> + >> +void >> +set_inferior_cwd (const char *cwd) >> +{ >> + xfree ((void *) current_inferior_cwd); >> + current_inferior_cwd = xstrdup (cwd); >> +} >> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c >> index f3eee31052..14f8a732a7 100644 >> --- a/gdb/gdbserver/server.c >> +++ b/gdb/gdbserver/server.c >> @@ -869,6 +869,21 @@ handle_general_set (char *own_buf) >> return; >> } >> >> + if (startswith (own_buf, "QSetWorkingDir:")) >> + { >> + const char *p = own_buf + strlen ("QSetWorkingDir:"); >> + std::string path = hex2str (p); >> + >> + set_inferior_cwd (path.c_str ()); >> + >> + if (remote_debug) >> + debug_printf (_("[Changed current directory to %s]\n"), >> + path.c_str ()); > > Please tweak the debug string to be clear that this is the > inferior's cwd, not gdbserver's. OK, changed to: debug_printf (_("[Set the inferior's current directory to %s]\n"), path.c_str ()); >> + write_ok (own_buf); >> + >> + return; >> + } >> + >> /* Otherwise we didn't know what packet it was. Say we didn't >> understand it. */ >> own_buf[0] = 0; >> @@ -2355,7 +2370,8 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p) >> sprintf (own_buf, >> "PacketSize=%x;QPassSignals+;QProgramSignals+;" >> "QStartupWithShell+;QEnvironmentHexEncoded+;" >> - "QEnvironmentReset+;QEnvironmentUnset+", >> + "QEnvironmentReset+;QEnvironmentUnset+;" >> + "QSetWorkingDir+", >> PBUFSIZ - 1); >> >> if (target_supports_catch_syscall ()) >> diff --git a/gdb/gdbserver/win32-low.c b/gdb/gdbserver/win32-low.c >> index cc84d15c2f..62b222d2fa 100644 >> --- a/gdb/gdbserver/win32-low.c >> +++ b/gdb/gdbserver/win32-low.c >> @@ -562,10 +562,11 @@ static BOOL >> create_process (const char *program, char *args, >> DWORD flags, PROCESS_INFORMATION *pi) >> { >> + const char *inferior_cwd = get_inferior_cwd (); >> BOOL ret; >> >> #ifdef _WIN32_WCE >> - wchar_t *p, *wprogram, *wargs; >> + wchar_t *p, *wprogram, *wargs, *wcwd = NULL; >> size_t argslen; >> >> wprogram = alloca ((strlen (program) + 1) * sizeof (wchar_t)); >> @@ -579,6 +580,14 @@ create_process (const char *program, char *args, >> wargs = alloca ((argslen + 1) * sizeof (wchar_t)); >> mbstowcs (wargs, args, argslen + 1); >> >> + if (inferior_cwd != NULL) >> + { >> + size_t cwdlen = strlen (inferior_cwd); >> + >> + wcwd = alloca ((cwdlen + 1) * sizeof (wchar_t)); >> + mbstowcs (wcwd, inferior_cwd, cwdlen + 1); >> + } >> + >> ret = CreateProcessW (wprogram, /* image name */ >> wargs, /* command line */ >> NULL, /* security, not supported */ >> @@ -586,7 +595,7 @@ create_process (const char *program, char *args, >> FALSE, /* inherit handles, not supported */ >> flags, /* start flags */ >> NULL, /* environment, not supported */ >> - NULL, /* current directory, not supported */ >> + wcwd, /* current directory */ >> NULL, /* start info, not supported */ >> pi); /* proc info */ >> #else >> @@ -599,7 +608,7 @@ create_process (const char *program, char *args, >> TRUE, /* inherit handles */ >> flags, /* start flags */ >> NULL, /* environment */ >> - NULL, /* current directory */ >> + inferior_cwd, /* current directory */ >> &si, /* start info */ >> pi); /* proc info */ >> #endif >> diff --git a/gdb/infcmd.c b/gdb/infcmd.c >> index 4c5bbfdbf2..f483a33a44 100644 >> --- a/gdb/infcmd.c >> +++ b/gdb/infcmd.c >> @@ -250,9 +250,9 @@ 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. */ >> +/* See common/common-inferior.h. */ >> >> -static void >> +void >> set_inferior_cwd (const char *cwd) >> { >> if (*cwd == '\0') >> @@ -292,7 +292,8 @@ show_cwd_command (struct ui_file *file, int from_tty, >> fprintf_filtered (gdb_stdout, >> _("\ >> You have not set the inferior's current working directory.\n\ >> -The inferior will inherit GDB's cwd.\n")); >> +The inferior will inherit GDB's cwd if native debugging, or gdbserver's\n\ >> +cwd if remote debugging.\n")); > > gdbserver -> remote server. Updated. >> +static void >> +extended_remote_set_inferior_cwd (struct remote_state *rs) >> +{ >> + if (packet_support (PACKET_QSetWorkingDir) != PACKET_DISABLE) >> + { >> + const char *inferior_cwd = get_inferior_cwd (); >> + >> + if (inferior_cwd != NULL) >> + { > > What happens if you do: > > set cwd foo > run > kill > set cwd # clear > run > > Do we clear the cwd on the remote end or we do we fail the > NULL check above? In this specific version of the patch, which does not have the implementation to clear out the inferior's cwd, the empty "set cwd" would actually make the "~" as the current directory for the inferior. But in the next version (which I'll send), when "inferior_cwd == NULL" I will send an empty QSetWorkingDir packet (which means I'll extend the QSetWorkingDir packet to accept empty arguments), and that will clear things up in the server side. > >> +/* See common/common-inferior.h. */ >> + >> +void >> +set_inferior_cwd (const char *cwd) >> +{ >> + xfree ((void *) current_inferior_cwd); >> + current_inferior_cwd = xstrdup (cwd); >> +} > > This always sets to non-NULL. So is it really possible > to get back to the original clear state? Doesn't look like it? > Do we have a test for that? Not in this specific version, no. This was something decided *after* I had sent this version. In my local tree I have code implementing the "clear up" behaviour, and I'll add a test for that as well. > > >> + std::string hexpath = bin2hex ((const gdb_byte *) inferior_cwd, >> + strlen (inferior_cwd)); >> + >> + xsnprintf (rs->buf, get_remote_packet_size (), >> + "QSetWorkingDir:%s", hexpath.c_str ()); >> + putpkt (rs->buf); >> + getpkt (&rs->buf, &rs->buf_size, 0); >> + if (packet_ok (rs->buf, >> + &remote_protocol_packets[PACKET_QSetWorkingDir]) >> + != PACKET_OK) >> + error (_("\ >> +Remote replied unexpectedly while changing working directory: %s"), >> + rs->buf); > > Again "changing". Maybe say "setting the inferior's working directory" > instead. OK. > > >> + } >> + } >> +} >> + >> /* In the extended protocol we want to be able to do things like >> "run" and have them basically work as expected. So we need >> a special create_inferior function. We support changing the >> @@ -9686,6 +9718,8 @@ Remote replied unexpectedly while setting startup-with-shell: %s"), >> >> extended_remote_environment_support (rs); >> >> + extended_remote_set_inferior_cwd (rs); >> + >> /* Now restart the remote server. */ >> run_worked = extended_remote_run (args) != -1; >> if (!run_worked) >> @@ -14185,6 +14219,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL, >> add_packet_config_cmd (&remote_protocol_packets[PACKET_QProgramSignals], >> "QProgramSignals", "program-signals", 0); >> >> + add_packet_config_cmd (&remote_protocol_packets[PACKET_QSetWorkingDir], >> + "QSetWorkingDir", "set-working-dir", 0); >> + >> add_packet_config_cmd (&remote_protocol_packets[PACKET_QStartupWithShell], >> "QStartupWithShell", "startup-with-shell", 0); >> >> diff --git a/gdb/testsuite/gdb.base/set-cwd.exp b/gdb/testsuite/gdb.base/set-cwd.exp >> index f2700ec44d..32458e384e 100644 >> --- a/gdb/testsuite/gdb.base/set-cwd.exp >> +++ b/gdb/testsuite/gdb.base/set-cwd.exp >> @@ -15,11 +15,18 @@ >> # 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" >> +if { [use_gdb_stub] } { >> + untested "not valid on native-gdbserver" > > There are many other boards that set use_gdb_stub, not just > native-gdbserver. Other testcases say: > > untested "skipping tests due to use_gdb_stub" Hm, OK. Does this look correct now, though? >> return >> } >> >> +if { [target_info gdb_protocol] == "remote" } { >> + load_lib gdbserver-support.exp >> + if { [skip_gdbserver_tests] } { >> + return >> + } >> +} >> + > > Please remember to drop this part. Already did. Thanks. >> standard_testfile >> >> if { [prepare_for_testing "failed to prepare" $testfile $srcfile debug] } { >> > > > Thanks, > Pedro Alves 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/