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

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.

> 
> Thanks,
> Andrew
> 
> ---
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index b65124d807f..e92227d3803 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -43531,6 +43531,10 @@ Packets
>  use byte accesses, or not.  For this reason, this packet may not be
>  suitable for accessing memory-mapped I/O devices.
>  
> +If @var{length} is zero then the reply should contain the leading
> +@samp{b} prefix and not data.  @value{GDBN} sends a zero length
> +@samp{x} packet to probe for support.
> +
>  Reply:
>  @table @samp
>  @item b @var{XX@dots{}}
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 64622dbfcdf..a332fcd6f95 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -9683,17 +9683,55 @@ remote_target::remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr,
>    memaddr = remote_address_masked (memaddr);
>  
>    /* Construct "m/x"<memaddr>","<len>".  */
> -  auto send_request = [this, rs, memaddr, todo_units] (char format) -> void
> +  auto send_request = [this, rs, memaddr] (char format, int len) -> void
>      {
>        char *buffer = rs->buf.data ();
>        *buffer++ = format;
>        buffer += hexnumstr (buffer, (ULONGEST) memaddr);
>        *buffer++ = ',';
> -      buffer += hexnumstr (buffer, (ULONGEST) todo_units);
> +      buffer += hexnumstr (buffer, (ULONGEST) len);
>        *buffer = '\0';
>        putpkt (rs->buf);
>      };
>  
> +  /* Unfortunately, lldb and GDB disagree on the reply format for the 'x'
> +     packet.  GDB expects a 'b' as the first byte of a successful packet,
> +     while lldb does not.  There are good reasons why GDB uses the 'b'
> +     prefix; it's not possible to distinguish between an empty
> +     not-supported reply and a failed read empty reply, unless there is a
> +     prefix.
> +
> +     As lldb "got there first", and there are clients in the wild that
> +     support the lldb style reply, we probe with a zero length read.  If
> +     this returns a single 'b' then we know that the 'x' packet is
> +     supported.
> +
> +     If we get back an error packet (starts with 'E') then we cannot know
> +     if a successful read would start with a 'b' or not.  We'll probe again
> +     next time.  */
> +  if (m_features.packet_support (PACKET_x) == PACKET_SUPPORT_UNKNOWN)
> +    {
> +      send_request ('x', 0);
> +      int packet_len = getpkt (&rs->buf);
> +
> +      /* Catastrophic error fetching packet.  */
> +      if (packet_len < 0)
> +	return TARGET_XFER_E_IO;
> +
> +      /* If we get back a lone 'b' with no data then the remote replied as
> +	 expected.  Anything else and we just disable the 'x' packet.  */
> +      if (rs->buf[0] == 'b' && rs->buf[1] == '\0')
> +	{
> +	  remote_debug_printf ("binary memory read is supported by target");
> +	  m_features.m_protocol_packets[PACKET_x].support = PACKET_ENABLE;
> +	}
> +      else
> +	{
> +	  remote_debug_printf ("binary memory read NOT supported by target");
> +	  m_features.m_protocol_packets[PACKET_x].support = PACKET_DISABLE;
> +	}
> +    }
> +
>    /* Determine which packet format to use.  The target's support for
>       'x' may be unknown.  We just try.  If it doesn't work, we try
>       again using 'm'.  */
> @@ -9703,32 +9741,11 @@ remote_target::remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr,
>    else
>      packet_format = 'x';
>  
> -  send_request (packet_format);
> +  send_request (packet_format, todo_units);
>    int packet_len = getpkt (&rs->buf);
>    if (packet_len < 0)
>      return TARGET_XFER_E_IO;
>  
> -  if (m_features.packet_support (PACKET_x) == PACKET_SUPPORT_UNKNOWN)
> -    {
> -      if (rs->buf[0] == '\0')
> -	{
> -	  remote_debug_printf ("binary uploading NOT supported by target");
> -	  m_features.m_protocol_packets[PACKET_x].support = PACKET_DISABLE;
> -
> -	  /* Try again using 'm'.  */
> -	  packet_format = 'm';
> -	  send_request (packet_format);
> -	  packet_len = getpkt (&rs->buf);
> -	  if (packet_len < 0)
> -	    return TARGET_XFER_E_IO;
> -	}
> -      else
> -	{
> -	  remote_debug_printf ("binary uploading supported by target");
> -	  m_features.m_protocol_packets[PACKET_x].support = PACKET_ENABLE;
> -	}
> -    }
> -
>    packet_result result = packet_check_result (rs->buf);
>    if (result.status () == PACKET_ERROR)
>      return TARGET_XFER_E_IO;
>