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

Andrew Burgess <[email protected]> writes:

> 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.

I've posted my proposed fix for this issue here:

  https://inbox.sourceware.org/gdb-patches/3183bc4875167009274313529badbb1685042c76.1737660927.git.aburgess@redhat.com

I started a new thread to increase the visibility.

All feedback appreciated.

Thanks,
Andrew