Mirror of the gdb mailing list
 help / color / mirror / Atom feed
* Incompatible implementat ion of 'x' packet in GDB vs LLDB
@ 2025-01-22 19:57 Robert O'Callahan
  2025-01-23  7:12 ` Robert O'Callahan
  0 siblings, 1 reply; 12+ messages in thread
From: Robert O'Callahan @ 2025-01-22 19:57 UTC (permalink / raw)
  To: gdb

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.

Thanks,
Robert O'Callahan

[1]
https://sourceware.org/gdb/current/onlinedocs/gdb.html/Packets.html#Packets
[2] https://github.com/bminor/binutils-gdb/blob/master/gdb/remote.c#L9739
[3] https://lldb.llvm.org/resources/lldbgdbremote.html#x-binary-memory-read
[4] https://github.com/rr-debugger/rr/pull/3902
-- 
Su ot deraeppa sah dna Rehtaf eht htiw saw hcihw, efil lanrete eht uoy ot
mialcorp ew dna, ti ot yfitset dna ti nees evah ew; deraeppa efil eht. Efil
fo Drow eht gninrecnoc mialcorp ew siht - dehcuot evah sdnah ruo dna ta
dekool evah ew hcihw, seye ruo htiw nees evah ew hcihw, draeh evah ew
hcihw, gninnigeb eht morf saw hcihw taht.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-22 19:57 Incompatible implementat ion of 'x' packet in GDB vs LLDB Robert O'Callahan
@ 2025-01-23  7:12 ` Robert O'Callahan
  2025-01-23  8:13   ` Aktemur, Tankut Baris via Gdb
  0 siblings, 1 reply; 12+ messages in thread
From: Robert O'Callahan @ 2025-01-23  7:12 UTC (permalink / raw)
  To: gdb

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?

Rob
-- 
Su ot deraeppa sah dna Rehtaf eht htiw saw hcihw, efil lanrete eht uoy ot
mialcorp ew dna, ti ot yfitset dna ti nees evah ew; deraeppa efil eht. Efil
fo Drow eht gninrecnoc mialcorp ew siht - dehcuot evah sdnah ruo dna ta
dekool evah ew hcihw, seye ruo htiw nees evah ew hcihw, draeh evah ew
hcihw, gninnigeb eht morf saw hcihw taht.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23  7:12 ` Robert O'Callahan
@ 2025-01-23  8:13   ` Aktemur, Tankut Baris via Gdb
  2025-01-23 11:23     ` Robert O'Callahan
  2025-01-23 16:14     ` Andrew Burgess via Gdb
  0 siblings, 2 replies; 12+ messages in thread
From: Aktemur, Tankut Baris via Gdb @ 2025-01-23  8:13 UTC (permalink / raw)
  To: robert, gdb

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/

Regards,
-Baris 


> Rob
> --
> Su ot deraeppa sah dna Rehtaf eht htiw saw hcihw, efil lanrete eht uoy ot
> mialcorp ew dna, ti ot yfitset dna ti nees evah ew; deraeppa efil eht. Efil
> fo Drow eht gninrecnoc mialcorp ew siht - dehcuot evah sdnah ruo dna ta
> dekool evah ew hcihw, seye ruo htiw nees evah ew hcihw, draeh evah ew
> hcihw, gninnigeb eht morf saw hcihw taht.


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23  8:13   ` Aktemur, Tankut Baris via Gdb
@ 2025-01-23 11:23     ` Robert O'Callahan
  2025-01-23 11:39       ` Luis Machado via Gdb
  2025-01-23 16:14     ` Andrew Burgess via Gdb
  1 sibling, 1 reply; 12+ messages in thread
From: Robert O'Callahan @ 2025-01-23 11:23 UTC (permalink / raw)
  To: Aktemur, Tankut Baris; +Cc: gdb

On Thu, 23 Jan 2025 at 21:13, Aktemur, Tankut Baris <
tankut.baris.aktemur@intel.com> wrote:

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

Yes, both on the sending side and the receiving side:
Send:
https://github.com/llvm/llvm-project/blob/cb714e74cc0efd5bfdb3e5e80978239425bd83d4/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L723
Receive:
https://github.com/llvm/llvm-project/blob/2e6cc79f816d942ab09d6a310cd925c1da148aa9/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp#L2517
Not ideal, although it gets the job done.

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/


Thanks. From the point of view of an implementer trying to serve both
debuggers, it's unfortunate that no-one raised the question of what LLDB
had done... maybe next time someone could skim the LLDB protocol doc (and
vice versa on their side).

The LLDB approach is a bit distasteful so I understand why you wouldn't
want to follow it. But rr users who upgrade to gdb 16.1 before they update
rr are going to have a bad time, especially because it manifests as rr+gdb
just being mysteriously broken. GDB 16.1 was only just released and already
three users have reported the bug to us [1].

The least hacky fix I can think of that you could do to help us would be to
do the "x0,0" query thing to detect if the packet is supported and if so,
whether it's LLDB or GDB flavour and use that.

Whatever you do or don't do, for the future in rr I think we'll have to
push "LLDB vs GDB mode" deeper into our protocol handling and make sure any
new features are only enabled for the client that we have tested them with.
It may be even more complicated than that because there are other clients
like delve, although hopefully they can stick to e.g. the GDB mode.

Rob

[1] https://github.com/rr-debugger/rr/issues/3901
-- 
Su ot deraeppa sah dna Rehtaf eht htiw saw hcihw, efil lanrete eht uoy ot
mialcorp ew dna, ti ot yfitset dna ti nees evah ew; deraeppa efil eht. Efil
fo Drow eht gninrecnoc mialcorp ew siht - dehcuot evah sdnah ruo dna ta
dekool evah ew hcihw, seye ruo htiw nees evah ew hcihw, draeh evah ew
hcihw, gninnigeb eht morf saw hcihw taht.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23 11:23     ` Robert O'Callahan
@ 2025-01-23 11:39       ` Luis Machado via Gdb
  0 siblings, 0 replies; 12+ messages in thread
From: Luis Machado via Gdb @ 2025-01-23 11:39 UTC (permalink / raw)
  To: robert, Aktemur, Tankut Baris; +Cc: gdb

On 1/23/25 11:23, Robert O'Callahan wrote:
> On Thu, 23 Jan 2025 at 21:13, Aktemur, Tankut Baris <
> tankut.baris.aktemur@intel.com> wrote:
> 
>> 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?
>>
> 
> Yes, both on the sending side and the receiving side:
> Send:
> https://github.com/llvm/llvm-project/blob/cb714e74cc0efd5bfdb3e5e80978239425bd83d4/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp#L723
> Receive:
> https://github.com/llvm/llvm-project/blob/2e6cc79f816d942ab09d6a310cd925c1da148aa9/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp#L2517
> Not ideal, although it gets the job done.
> 
> 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/
> 
> 
> Thanks. From the point of view of an implementer trying to serve both
> debuggers, it's unfortunate that no-one raised the question of what LLDB
> had done... maybe next time someone could skim the LLDB protocol doc (and
> vice versa on their side).

I understand the pain. Sorry this has caused a bit of a hiccup in terms of support.

We can probably look into ways to make this smoother to integrate (or a workaround
to check if we're talking to GDB or LLDB), as we're not actively trying to break
stuff. It is just an unfortunate lack of community cooperation/coordination.

With that said, at least some of us have been aware of the gap between LLDB's RSP
implementation and GDB's. I recall discussing some of this years ago and warning
about it. But at the time there was a bit of a "us x them" feeling going on between
GNU and LLVM (mostly clang x gcc). So very little cooperation went into it.

As a result, you have a significantly different RSP implementation for LLDB that
doesn't make sense for GDB to follow without proper coordination/justification.

Given GDB is probably deemed the source of the RSP implementation and documentation,
I think things created for the RSP should be contributed back to the protocol for the
sake of making the lives of debugging server implementors easier across the board.

Maybe moving forward we could plan to converge the two variations of the RSP, or
just call them different and have a way to tell them apart (I don't think that's
ideal though).

Really ideally, we should probably ditch the RSP mechanism and go for a better
protocol. One can dream.

> 
> The LLDB approach is a bit distasteful so I understand why you wouldn't
> want to follow it. But rr users who upgrade to gdb 16.1 before they update
> rr are going to have a bad time, especially because it manifests as rr+gdb
> just being mysteriously broken. GDB 16.1 was only just released and already
> three users have reported the bug to us [1].
> 
> The least hacky fix I can think of that you could do to help us would be to
> do the "x0,0" query thing to detect if the packet is supported and if so,
> whether it's LLDB or GDB flavour and use that.
> 
> Whatever you do or don't do, for the future in rr I think we'll have to
> push "LLDB vs GDB mode" deeper into our protocol handling and make sure any
> new features are only enabled for the client that we have tested them with.
> It may be even more complicated than that because there are other clients
> like delve, although hopefully they can stick to e.g. the GDB mode.
> 
> Rob
> 
> [1] https://github.com/rr-debugger/rr/issues/3901


^ permalink raw reply	[flat|nested] 12+ messages in thread

* RE: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23  8:13   ` Aktemur, Tankut Baris via Gdb
  2025-01-23 11:23     ` Robert O'Callahan
@ 2025-01-23 16:14     ` Andrew Burgess via Gdb
  2025-01-23 16:33       ` Luis Machado via Gdb
  1 sibling, 1 reply; 12+ messages in thread
From: Andrew Burgess via Gdb @ 2025-01-23 16:14 UTC (permalink / raw)
  To: Aktemur, Tankut Baris, robert, gdb

"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;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23 16:14     ` Andrew Burgess via Gdb
@ 2025-01-23 16:33       ` Luis Machado via Gdb
  2025-01-23 17:28         ` Andrew Burgess via Gdb
  0 siblings, 1 reply; 12+ messages in thread
From: Luis Machado via Gdb @ 2025-01-23 16:33 UTC (permalink / raw)
  To: Andrew Burgess, Aktemur, Tankut Baris, robert, gdb

On 1/23/25 16:14, Andrew Burgess via Gdb wrote:
> "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.

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23 16:33       ` Luis Machado via Gdb
@ 2025-01-23 17:28         ` Andrew Burgess via Gdb
  2025-01-23 19:38           ` Andrew Burgess via Gdb
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Burgess via Gdb @ 2025-01-23 17:28 UTC (permalink / raw)
  To: Luis Machado, Aktemur, Tankut Baris, robert, gdb

Luis Machado <luis.machado@arm.com> writes:

> On 1/23/25 16:14, Andrew Burgess via Gdb wrote:
>> "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.
>
> 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23 17:28         ` Andrew Burgess via Gdb
@ 2025-01-23 19:38           ` Andrew Burgess via Gdb
  2025-01-28  8:26             ` Pavel Labath via Gdb
  0 siblings, 1 reply; 12+ messages in thread
From: Andrew Burgess via Gdb @ 2025-01-23 19:38 UTC (permalink / raw)
  To: Luis Machado, Aktemur, Tankut Baris, robert, gdb

Andrew Burgess <aburgess@redhat.com> writes:

> Luis Machado <luis.machado@arm.com> writes:
>
>> On 1/23/25 16:14, Andrew Burgess via Gdb wrote:
>>> "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.
>>
>> 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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-23 19:38           ` Andrew Burgess via Gdb
@ 2025-01-28  8:26             ` Pavel Labath via Gdb
  2025-01-28  9:25               ` Pavel Labath via Gdb
  0 siblings, 1 reply; 12+ messages in thread
From: Pavel Labath via Gdb @ 2025-01-28  8:26 UTC (permalink / raw)
  To: Andrew Burgess, Luis Machado, Aktemur, Tankut Baris, robert, gdb

Hello everyone, an lldb dev here :)

I'm sorry for the trouble our implementation of 'x' has caused. I have 
to admit I was surprised to find that the packet was not in the gdb 
documentation already. It's been implemented in lldb for as long as I 
can remember (~10 years), and the new packets we're adding nowadays have 
much longer names, so I had assumed that it was always a part of the gdb 
spec.

For what it's worth, I think your definition of the packet makes much 
more sense. LLDB's definition is indeed ambiguous (and I didn't realize 
how ambiguous until now) -- it cannot distinguish between a (truncated?) 
memory read and an error. This behavior is not completely easy to 
trigger because lldb will by default round the memory reads to 512-byte 
boundaries (so truncation is unlikely), but with the right commands, I 
was able to get it to treat valid memory as an error.

For this reason, I am going to propose to migrate lldb to the gdb 
("official") definition of the packet. Since we have users which need 
(fairly long) windows of compatibility with old server, this is going to 
require method to detect the implementation in use, so I'd like to reuse 
the same mechanism that's going to be used in gdb (both the zero length 
probe and the qSupported method seem fine to me).

regards,
Pavel

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-28  8:26             ` Pavel Labath via Gdb
@ 2025-01-28  9:25               ` Pavel Labath via Gdb
  2025-01-28 10:15                 ` Luis Machado via Gdb
  0 siblings, 1 reply; 12+ messages in thread
From: Pavel Labath via Gdb @ 2025-01-28  9:25 UTC (permalink / raw)
  To: gdb

On 28/01/2025 09:26, Pavel Labath via Gdb wrote:
> Hello everyone, an lldb dev here :)
> 
> I'm sorry for the trouble our implementation of 'x' has caused. I have 
> to admit I was surprised to find that the packet was not in the gdb 
> documentation already. It's been implemented in lldb for as long as I 
> can remember (~10 years), and the new packets we're adding nowadays have 
> much longer names, so I had assumed that it was always a part of the gdb 
> spec.
> 
> For what it's worth, I think your definition of the packet makes much 
> more sense. LLDB's definition is indeed ambiguous (and I didn't realize 
> how ambiguous until now) -- it cannot distinguish between a (truncated?) 
> memory read and an error. This behavior is not completely easy to 
> trigger because lldb will by default round the memory reads to 512-byte 
> boundaries (so truncation is unlikely), but with the right commands, I 
> was able to get it to treat valid memory as an error.
> 
> For this reason, I am going to propose to migrate lldb to the gdb 
> ("official") definition of the packet. Since we have users which need 
> (fairly long) windows of compatibility with old server, this is going to 
> require method to detect the implementation in use, so I'd like to reuse 
> the same mechanism that's going to be used in gdb (both the zero length 
> probe and the qSupported method seem fine to me).
> 
> regards,
> Pavel

And this 
<https://discourse.llvm.org/t/rfc-fixing-incompatibilties-of-the-x-packet-w-r-t-gdb/84288/1> 
is the lldb thread for that.

pl

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: Incompatible implementat ion of 'x' packet in GDB vs LLDB
  2025-01-28  9:25               ` Pavel Labath via Gdb
@ 2025-01-28 10:15                 ` Luis Machado via Gdb
  0 siblings, 0 replies; 12+ messages in thread
From: Luis Machado via Gdb @ 2025-01-28 10:15 UTC (permalink / raw)
  To: Pavel Labath, gdb

On 1/28/25 09:25, Pavel Labath via Gdb wrote:
> On 28/01/2025 09:26, Pavel Labath via Gdb wrote:
>> Hello everyone, an lldb dev here :)
>>
>> I'm sorry for the trouble our implementation of 'x' has caused. I have to admit I was surprised to find that the packet was not in the gdb documentation already. It's been implemented in lldb for as long as I can remember (~10 years), and the new packets we're adding nowadays have much longer names, so I had assumed that it was always a part of the gdb spec.
>>
>> For what it's worth, I think your definition of the packet makes much more sense. LLDB's definition is indeed ambiguous (and I didn't realize how ambiguous until now) -- it cannot distinguish between a (truncated?) memory read and an error. This behavior is not completely easy to trigger because lldb will by default round the memory reads to 512-byte boundaries (so truncation is unlikely), but with the right commands, I was able to get it to treat valid memory as an error.
>>
>> For this reason, I am going to propose to migrate lldb to the gdb ("official") definition of the packet. Since we have users which need (fairly long) windows of compatibility with old server, this is going to require method to detect the implementation in use, so I'd like to reuse the same mechanism that's going to be used in gdb (both the zero length probe and the qSupported method seem fine to me).
>>
>> regards,
>> Pavel
> 
> And this <https://discourse.llvm.org/t/rfc-fixing-incompatibilties-of-the-x-packet-w-r-t-gdb/84288/1> is the lldb thread for that.
> 
> pl

Hi Pavel,

Thanks a lot for the input and for getting a thread going on lldb's side. Hopefully we can get something that works for both and users won't run into hiccups.

I suppose we will still need something on gdb's side to handle the existing lldb format, but moving forward we could potentially sync things for both debuggers.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2025-01-28 10:16 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-01-22 19:57 Incompatible implementat ion of 'x' packet in GDB vs LLDB Robert O'Callahan
2025-01-23  7:12 ` Robert O'Callahan
2025-01-23  8:13   ` Aktemur, Tankut Baris via Gdb
2025-01-23 11:23     ` Robert O'Callahan
2025-01-23 11:39       ` Luis Machado via Gdb
2025-01-23 16:14     ` Andrew Burgess via Gdb
2025-01-23 16:33       ` Luis Machado via Gdb
2025-01-23 17:28         ` Andrew Burgess via Gdb
2025-01-23 19:38           ` Andrew Burgess via Gdb
2025-01-28  8:26             ` Pavel Labath via Gdb
2025-01-28  9:25               ` Pavel Labath via Gdb
2025-01-28 10:15                 ` Luis Machado via Gdb

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox