Re: [PATCH v2] remote.c: Allow inferior to reply with an error

[Andrew Burgess <aburgess@redhat.com>]

AlexandraHájková@sourceware.org writes:

> From: Alexandra Hájková <ahajkova@redhat.com>
>
>     When gdb communicates with some kind of gdbserver or gdbserver
>     stub over the remote protocol, the only possible response to
>     the QSetWorkingDir packet is "OK".  If the remote will reply
>     with anything else, gdb will complain about the unexpected reply.
>
>     [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
>     [remote] Packet received: E00
>     Remote replied unexpectedly while setting the inferior's working
>     directory: E00
>     (gdb)
>
>     Allow remote to send an error message over as a QSetWorkingDir
>     packet reply.
>
>     [remote] Sending packet: $QSetWorkingDir:2f746d70#bb
>     [remote] Packet received: E.directory does not exist
>     Remote failed to set working directory: directory does not exist.

So, I started reviewing this patch, and wrote a whole bunch of stuff.
One thing I wrote was, you should really add a test for this that uses
the existing gdbserver.

So, I thought, I wonder what errors gdbserver currently sends back, so I
took a look at the QSetWorkingDir handling in gdbserver/server.cc.
Weird, I thought, it doesn't appear to send back any errors.

So then I took a look at set_inferior_cwd in gdbserver/inferiors.cc, and
all that it does is cache the requested directory.

So, then I went to the docs again, and re-read what we say about
QSetWorkingDir:

  This packet is used to inform the remote server of the intended
  current working directory for programs that are going to be executed.

Note the use of the word 'intended'.  This packet does not try to change
the directory NOW, it will try to change the directory LATER, when the
inferior is actually executed.

My current reading of the docs is that the QSetWorkingDir packet, is
either not supported, or should never fail.

If you specify an invalid directory then future attempts to start an
inferior will fail, but the QSetWorkingDir packet itself shouldn't
fail.

I guess we could decide that we want to extend QSetWorkingDir to allow
for it to send back error packets, I'm not sure that's entirely a good
idea though as that's not how the existing gdbserver works... but if we
did want to do that then we should probably improve the documentation to
explain that some gdbservers will cache the passed in directory and
return an error later, while others might choose to immediately change
directory and return an error now.

Because I already wrote it, I've left all my original review comments
inline below, but I'm not sure if you will want to roll a v3 or not.

Thanks,
Andrew

>
> ---
> V2 does not change the behaviour of gdb in a case it wasn't possible to
> set the inferior's working directory. It just allows to pass the error
> message to gdb.
>
>  gdb/doc/gdb.texinfo |  3 +++
>  gdb/remote.c        | 12 +++++++++---
>  2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 9c0018ea5c1..5df9a5a9178 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42517,6 +42517,9 @@ Reply:
>  @table @samp
>  @item OK
>  The request succeeded.
> +
> +@item E.errtext
> +An error occurred.  Reply with an error message.

I think that we should not limit the description here to just
'E.errtext', all the other error packets also support the 'Enn' format,
and for consistency, I think we should document that as supported here
too.

>  @end table
>  
>  @item qfThreadInfo
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 218bca30d04..ea89759e85a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -10397,8 +10397,10 @@ remote_target::extended_remote_set_inferior_cwd ()
>    if (packet_support (PACKET_QSetWorkingDir) != PACKET_DISABLE)
>      {
>        const std::string &inferior_cwd = current_inferior ()->cwd ();
> +      char *buf;
>        remote_state *rs = get_remote_state ();
>  
> +      buf = rs->buf.data ();

Given that BUF is only used later within the 'if (packet_ok (...))'
block, I'd move the declaration, and definition of BUF into that block.

Also, for C++, you don't need to declare the variable at the start of
the block, just 'char *buf = ....' is fine.

>        if (!inferior_cwd.empty ())
>  	{
>  	  std::string hexpath
> @@ -10420,11 +10422,15 @@ remote_target::extended_remote_set_inferior_cwd ()
>        getpkt (&rs->buf, 0);
>        if (packet_ok (rs->buf,
>  		     &remote_protocol_packets[PACKET_QSetWorkingDir])
> -	  != PACKET_OK)
> -	error (_("\
> +	  != PACKET_OK) {

A bunch of the following lines are indented entirely with spaces.
Unfortunately GNU style is a mix of tabs and spaces, so you need to
figure out how to configure your editor to indent lines correctly.  I'm
surprised that this doesn't get highlighted in a 'git diff' / 'git show'
output due to our .gitattributes file.

> +          if (buf[0] == 'E' && buf[1] == '.')
> +	    error (_("Remote failed to set working directory: %s"), buf + 2);
> +          else
> +              error (_("\
>  Remote replied unexpectedly while setting the inferior's working\n\

I think we should handle both error packet formats here.  Also, while
looking at this code, I noticed that we don't appear to check the
contents of the reply packet in the PACKET_OK case.  The docs say the
reply should be 'OK', but, we actually accept anything.  Maybe we should
"fix" that while we're here?

I'd suggest something like this:

      switch (packet_ok (rs->buf,
			 &remote_protocol_packets[PACKET_QSetWorkingDir]))
	{
	case PACKET_OK:
	  if (strcmp (rs->buf.data (), "OK") == 0)
	    break;
	  error (_("\
Remote replied unexpectedly while setting the inferior's working\n	\
directory: %s."),
		 rs->buf.data ());

	case PACKET_ERROR:
	  {
	    char *buf = rs->buf.data ();
	    if (buf[0] == 'E' && buf[1] == '.' && buf[2] != '\0')
	      error (_("Remote failed to set working directory: %s"), buf + 2);
	    else
	      error (_("Remote failed to set working directory: %s"), buf);
	  }

	case PACKET_UNKNOWN:
	  error (_("Remote does not support changing the working directory"));
	}

This includes support for the 'E.msg' style error that you're adding,
but also allows for the more generic Enn error that all the other
packets seem to support.

In the PACKET_OK case we check that the remote sent back and 'OK', and
if not we get to reuse the existing unexpected reply error.

And finally, we handle the PACKET_UNKNOWN case with an appropriate
error.

> -directory: %s"),
> +directory: %s."),
>  	       rs->buf.data ());

With the definition of BUF now in this block, I'd probably replace the
'rs->buf.data()' with a reuse of BUF, I think that's going to appear
more consistent.

> +      }
>  
>      }
>  }
> -- 
> 2.39.0