Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB

Luis Machado <[email protected]> writes:

> On 1/23/25 16:14, Andrew Burgess via Gdb wrote:
>> "Aktemur, Tankut Baris via Gdb" <[email protected]> writes:
>> 
>>> On Thursday, January 23, 2025 8:13 AM, Robert O'Callahan wrote:
>>>> On Thu, 23 Jan 2025 at 08:57, Robert O'Callahan <[email protected]>
>>>> wrote:
>>>>
>>>>> GDB (client) 16.1 started sending the gdbserver 'x' packet. It follows the
>>>>> documentation [1] and expects a leading 'b' in the response [2].
>>>>> Unfortunately LLDB has supported this packet for quite a long time [3] and
>>>>> does not expect a leading 'b' in the response. We added support for this
>>>>> packet to rr last year and followed LLDB's format because it was the only
>>>>> user of the packet at that time. So GDB 16.1 doesn't work with rr. [4]
>>>>>
>>>>> I realize that compatibility between GDB and LLDB flavoured gdbserver
>>>>> protocols is not a priority for either team, but until now it has actually
>>>>> worked in practice --- rr hasn't needed a client mode switch. We can add
>>>>> one, but it will be unfortunate if GDB 16.1 and later is incompatible for
>>>>> anyone who's installed the latest rr since May 2024.
>>>>>
>>>>> Could you make a GDB 16.1 point release that removes the 'b'? AFAIK it
>>>>> serves no purpose.
>>>>>
>>>>
>>>> It has been pointed out that if you want to return different error codes
>>>> then you need the 'b'. Is that the rationale?
>>>
>>> Hello Rob,
>>>
>>> Essentially, yes.  In case of an error, the server responds with an 'E' packet.
>>> To be able to distinguish an error packet from binary data, 'E' would have to be
>>> added to the list of escaped characters.  Having the 'b' marker avoids that.
>>>
>>> Additionally, when the response is empty, per RSP, it means the packet is unsupported.
>>> So, in case of a zero-length request, the 'b' marker could help us distinguish the
>>> unsupported case from an actual zero-response.
>>> LLDB doc says
>>>
>>>   To test if this packet is available, send a addr/len of 0:
>>>
>>>     x0,0
>>>
>>>   You will get an OK response if it is supported. 
>>>   The reply will be the data requested in 8-bit binary data format.
>>>
>>> How does LLDB distinguish an "OK" response, an empty binary data, an error,
>>> and an unsupported case?  These were not clear to me from the docs.
>>> Is the x0,0 query special-cased?
>>>
>>> For the record, the 'x' packet series were discussed in
>>>
>>>   https://inbox.sourceware.org/gdb-patches/[email protected]/#r
>>>
>>> with the part specific to the 'b' marker in
>>>
>>>   https://inbox.sourceware.org/gdb-patches/[email protected]/
>> 
>> I also am not entirely sure how lldb's implementation can work in all
>> cases given their description.  For now at least, I still think we made
>> the right choice with our implementation, though I guess if we'd known
>> we were diverging from llvm we might have picked a different letter for
>> packet in order to make it completely different.
>> 
>> I have two thoughts for things we could do to possibly help with
>> compatibility here though:
>> 
>>   1. Hide the 'x' packet behind a feature flag.  So instead of using the
>>   empty reply to indicate 'x' packet support, we ask remote targets to
>>   send 'gdb-x+' in their qSupported reply.  If a remote target sends
>>   this to GDB then we will assume that they support GDB's style of 'x'
>>   packet, and can make use of the packet.  Otherwise, it's 'm' packets
>>   all the way.
>> 
>> 
>>   2. The other option would be to probe for 'x' packet support as a
>>   separate action.  Right now probing is done the first time we send an
>>   'x' packet.  The packet is sent, and if we get back the empty packet,
>>   then we know 'x' packets are supported.
>> 
>>   But if instead, we sent a zero length read, then by GDB's rules we
>>   should get back a lone 'b'.  By lldb's rules we should get back
>>   .... maybe "OK" ... or maybe the empty packet?  It's not really
>>   clear.  But the important thing is that neither of those replies are
>>   'b'.
>> 
>>   So the first time we consider sending an 'x' packet, we send
>>   'xADDR,0', if we get back 'b', then we know that we have GDB style
>>   support, otherwise, we disable the 'x' packet, and fall back to the
>>   'm' packet.
>> 
>> Approach #1 is pretty straight forward, but I wanted to check what
>> approach #2 would look like, so I drafted the patch below.  This patch
>> is pretty rough right now, but can easily be cleaned up.
>> 
>> For testing this patch I hacked up gdbserver so it no longer added the
>> 'b' prefix, then I tried connecting with GDB.  I see GDB send the zero
>> length probe, then switch back to 'm' packets.
>
> Thanks for the input.
>
>> 
>> Questions:
>> 
>>   + Do people think we should change GDB to improve compatibility?  My
>>     thinking is yes, so long as the cost isn't too high.
>
> Sounds reasonable. I think we should too, as it seems to be good timing
> (just after the release of gdb 16) for us to try to improve this.
>
>> 
>>   + Do people prefer approach #1 or #2?  I'm leaning slightly more to #2
>>     right now, but not massively so.  If people prefer #1 I can write
>>     the patch for that instead.
>
> Approach #2 seems to be a bit better from your description, as it checks
> things from gdb's side as opposed to chaging the server side. At least
> that's my personal take on it.
>

I hadn't spotted it before, but we already have
remote_target::check_binary_download which checks the 'X' packet by
doing a zero length write.

So I'm now even more convinced that #2 is the way to go.  I'm reworking
my patch to add a remote_target::check_binary_upload for checking the
'x' packet, and I hope to post it soon for consideration.

Thanks,
Andrew