[PATCH v3 5/5] Extend "set cwd" to work on gdbserver
Sergio Durigan Junior
sergiodj@redhat.com
Wed Sep 27 21:48:00 GMT 2017
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
>> <https://raw.githubusercontent.com/llvm-mirror/lldb/master/docs/lldb-gdb-remote.txt>),
>> 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
<https://raw.githubusercontent.com/llvm-mirror/lldb/master/docs/lldb-gdb-remote.txt>),
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 <sergiodj@redhat.com>
>>
>> * 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) <QSetWorkingDir>: 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 <sergiodj@redhat.com>
>>
>> * 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 <sergiodj@redhat.com>
>>
>> * gdb.base/set-cwd.exp: Make it available on gdbserver.
>>
>> gdb/doc/ChangeLog:
>> yyyy-mm-dd Sergio Durigan Junior <sergiodj@redhat.com>
>>
>> * gdb.texinfo (Starting your Program) <The working directory.>:
>> Mention remote debugging.
>> (Working Directory) <Your Program's Working Directory>:
>> Likewise.
>> (Connecting) <Remote Packet>: Add "set-working-dir"
>> and "QSetWorkingDir" to the table.
>> (Remote Protocol) <QSetWorkingDir>: 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 <http://www.gnu.org/licenses/>.
>>
>> -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/
More information about the Gdb-patches
mailing list