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

[Andrew Burgess via Gdb <gdb@sourceware.org>]

"Aktemur, Tankut Baris via Gdb" <gdb@sourceware.org> writes:

> On Thursday, January 23, 2025 8:13 AM, Robert O'Callahan wrote:
>> On Thu, 23 Jan 2025 at 08:57, Robert O'Callahan <robert@ocallahan.org>
>> 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/cover.1710343840.git.tankut.baris.aktemur@intel.com/#r
>
> with the part specific to the 'b' marker in
>
>   https://inbox.sourceware.org/gdb-patches/87msq82ced.fsf@redhat.com/

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.

Questions:

  + Do people think we should change GDB to improve compatibility?  My
    thinking is yes, so long as the cost isn't too high.

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

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;