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

[Andrew Burgess <aburgess@redhat.com>]

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.