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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Friday, September 22 2017, Eli Zaretskii wrote:

>> From: Sergio Durigan Junior <sergiodj@redhat.com>
>> Cc: gdb-patches@sourceware.org,  palves@redhat.com
>> Date: Fri, 22 Sep 2017 14:45:59 -0400
>> 
>> >> +  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?
>> 
>> Sorry, I am not a Windows programmer.  Other places in the code also
>> don't check for errors.
>
> Not checking for errors in these conversions can be worse on Windows
> than on other platforms, because the Windows' wchar_t supports only
> the BMP, so the chances of getting a conversion error are higher than
> on Unix.
>
>> I'd be happy to improve this code, but I refuse to touch a Windows
>> machine so I'm doing this all this without any testing.  But please,
>> feel absolutely free to point out how this code should look like.
>
> mbstowcs returns NULL if it fails, so I suggest to throw an error in
> that case.  I see no reason for anything fancier.

OK, that I can do :-).

>> >> +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?
>> 
>> I don't know.  There's nothing related to charset on gdb/gdbserver/, but
>> then again I don't know if we've ever encountered a case that demanded
>> conversions.  I can investigate this a bit more.
>
> GDB does know about target-charset and host-charset.  I'd expect file
> names that are sent to the target be in the target charset, but since
> what get_inferior_cwd returns is in host charset (it was typed by the
> user), I think a conversion might be in order.

I don't know.  We never seem to do that in other cases.  For example,
when we are starting the inferior remotely:

  static int
  extended_remote_run (const std::string &args)
  {
    ...
    if (strlen (remote_exec_file) * 2 + len >= get_remote_packet_size ())
      error (_("Remote file name too long for run packet"));
    len += 2 * bin2hex ((gdb_byte *) remote_exec_file, rs->buf + len,
                        strlen (remote_exec_file));


The "remote_exec_file" variable is also something that the user inputs
through the "set remote exec-file" command.  It doesn't seem like we
need to worry about charset conversion here.

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/