Re: [PATCH v3 5/5] Extend "set cwd" to work on gdbserver

[Eli Zaretskii <eliz@gnu.org>]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Cc: Pedro Alves <palves@redhat.com>,	Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu, 21 Sep 2017 18:59:26 -0400
> 
> 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.
> 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.

This once again raises the issue of whether to send expanded or
unexpanded directory down the wire, and if unexpanded, then what is
the meaning of the default "~" in the inferior.

> 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.

This part is OK.

> 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

OK for this part.

> +  if (inferior_cwd != NULL)
> +    {
> +      size_t cwdlen = strlen (inferior_cwd);
> +
> +      wcwd = alloca ((cwdlen + 1) * sizeof (wchar_t));
> +      mbstowcs (wcwd, inferior_cwd, cwdlen + 1);
> +    }

no error checking of the mbstowcs conversion?

>    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 */

Once again, this feeds CreateProcess with an unexpanded directory,
which AFAIU will not work if the directory includes "~".

> +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)
> +	{
> +	  std::string hexpath = bin2hex ((const gdb_byte *) inferior_cwd,
> +					 strlen (inferior_cwd));
> +

Shouldn't this do some encoding conversion, from the GDB charset to
the target charset, before encoding in hex?

Thanks.