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

  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.

---
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(-)
  

> From: AlexandraHájková@sourceware.org
> Date: Fri, 13 Jan 2023 12:59:10 +0100
> 
> 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.
> 
> ---
> 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(-)

OK for the documentation part.

Thanks.
  

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
  

Hi Andrew,

On Tue, 2023-01-17 at 16:30 +0000, Andrew Burgess wrote:
> 
> 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.

This is part of nicer integration of the valgrind gdbserver and gdb.
One of the issues we have been struggling with is making this more user
friendly. Although the reply to some packets, like vRun, can include an
error value ('Enn', An error occurred. The error number nn is given as
hex digits), gdb seems to not use the error number at all (and the
remote protocol doesn't seem to define meaning to the error numbers).

But a lot of different things can go wrong with vRun. But all gdb will
say is "Running on the remote target failed". So that is why we like
the different vRun "setup" packets that currently don't take an Error
return to explain to the user what exactly goes wrong.

So if you think things like QSetWorkingDir shouldn't return an error,
then how do we get what is going wrong when handling the vRun packet
back to the gdb user?

Thanks,

Mark
  

>
> 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.
>
 I haven't added one because I wasn't sure this patch would be accepted.
I'll add one if we agree this work makes sense.

>
> 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 think I misinterpreted this part. My understanding was the remote
gdbserver (or the stub) will change the directory and then it's weird
to not be able to send back errors. Your interpretation makes perfect
sense.

In this case, if this behaviour is the preferred way to handle the
QSetWOrkingDir,
I propose to change the documentation to make it better understandable.


> 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.
>
> What is your preference and does anybody else have an opinion on
QSetWorkingDir
(and similar packets) behaviour?

Another confusing thing in remote protocol documentation (
https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets)
is the error handling. Various packets can reply with an error:

‘E NN’

Indicate a badly formed request. The error number NN is given as hex
digits.
But packet_check_result in remote.c just checks if it's an error and does
not handle the 'NN'. And I haven't found the meaning of 'NN'.

Thank you very much for the in-depth review.
  

Mark Wielaard <mark@klomp.org> writes:

> Hi Andrew,
>
> On Tue, 2023-01-17 at 16:30 +0000, Andrew Burgess wrote:
>> 
>> 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.
>
> This is part of nicer integration of the valgrind gdbserver and gdb.
> One of the issues we have been struggling with is making this more user
> friendly. Although the reply to some packets, like vRun, can include an
> error value ('Enn', An error occurred. The error number nn is given as
> hex digits), gdb seems to not use the error number at all (and the
> remote protocol doesn't seem to define meaning to the error numbers).
>
> But a lot of different things can go wrong with vRun. But all gdb will
> say is "Running on the remote target failed". So that is why we like
> the different vRun "setup" packets that currently don't take an Error
> return to explain to the user what exactly goes wrong.
>
> So if you think things like QSetWorkingDir shouldn't return an error,
> then how do we get what is going wrong when handling the vRun packet
> back to the gdb user?

Off the top of my head, we could start defining some meanings for the
various 'Enn' values.

If it doesn't already, we could update vRun to accept the 'E.msg' style
error packets.

Or for this particular case, we could, as I left open in my reply,
extend QSetWorkingDir to allow errors.  My only concern with that is
it's not just an extension of QSetWorkingDir, but a change to how
QSetWorkingDir is to be handled.  We'd certainly need a more extensive
doc update than Alexandra initially suggested.

Having spent most of the last few years doing remote debug, I share your
frustration at the poor error reporting in the remote protocol, so
anything we can do to improve that is a good thing, I'm 100% on board
with improving error reporting.

My concern here was just that the initial proposal, as laid out, seemed
to be based on a misunderstanding of what the QSetWorkingDir does.

Thanks,
Andrew
  

Alexandra Petlanova Hajkova <ahajkova@redhat.com> writes:

>>
>> 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.
>>
>  I haven't added one because I wasn't sure this patch would be accepted.
> I'll add one if we agree this work makes sense.
>
>>
>> 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 think I misinterpreted this part. My understanding was the remote
> gdbserver (or the stub) will change the directory and then it's weird
> to not be able to send back errors. Your interpretation makes perfect
> sense.
>
> In this case, if this behaviour is the preferred way to handle the
> QSetWOrkingDir,
> I propose to change the documentation to make it better understandable.
>
>
>> 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.
>>
>> What is your preference and does anybody else have an opinion on
> QSetWorkingDir
> (and similar packets) behaviour?
>
> Another confusing thing in remote protocol documentation (
> https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets)
> is the error handling. Various packets can reply with an error:
>
> ‘E NN’

Indeed, and laying this out at 'E NN' seems really bad to me as there's
no space in the packet.  Though looking through the docs we seem to do
this everywhere, so I guess anyone working on this will figure that bit
our pretty quick.

>
> Indicate a badly formed request. The error number NN is given as hex
> digits.
> But packet_check_result in remote.c just checks if it's an error and does
> not handle the 'NN'. And I haven't found the meaning of 'NN'.

Just throwing out an idea here, maybe packet_check_result should return
an object, not an enum.  Maybe a packet_result.  Properties of this
object would be:

  struct packet_result
  {
     /* Return the type of packet we encountered.  */
     enum packet_result status () const;

     /* Return a string representing the error, only valid if status()
        is PACKET_ERROR.  */
     std::string error_message () const;
  };

Then packet_check_result could parse the packet just like it currently
does.  If it sees 'Enn' then the error message becomes 'Unknown error:
nn', if it sees 'E.msg' then the error message becomes the 'msg' part of
the packet.

We could even imagine a future where callers of packet_check_result
would pass in a error code translation table that maps values of 'nn' to
defined error messages ... maybe?

I feel this is drifting away from your original patch, but is an
interesting idea of better error reporting.

Thanks,
Andrew


>
> Thank you very much for the in-depth review.