gdb/remote: probe for 'x' packet support with a zero length request

Message ID 3183bc4875167009274313529badbb1685042c76.1737660927.git.aburgess@redhat.com
State New
Headers
Series gdb/remote: probe for 'x' packet support with a zero length request |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Andrew Burgess Jan. 23, 2025, 7:36 p.m. UTC
  This mailing list discussion:

  https://inbox.sourceware.org/gdb-patches/CAOp6jLYD0g-GUsx7jhO3g8H_4pHkB6dkh51cbyDT-5yMfQwu+A@mail.gmail.com

highlighted the following issue with GDB's 'x' packet implementation.

Unfortunately, LLDB also has an 'x' packet, but their implementation
is different to GDB's and so targets that have implemented LLDB's 'x'
packet are incompatible with GDB.

The above thread is specifically about the 'rr' tool, but there could
be other remote targets out there that have this problem.

The difference between LLDB and GDB is that GDB expects a 'b' prefix
on the reply data, while LLDB does not.  The 'b' is important as it
allows GDB to distinguish between an empty reply (which will be a 'b'
prefix with no trailing data) and an unsupported packet (which will be
a completely empty packet).  It is not clear to me how LLDB
distinguishes these two cases.

See for discussion of the 'x' packet:

  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 propose that we start probing for 'x' packet support by sending a
zero length 'x' packet read.  We already do something similar for the
'X' packet (see remote_target::check_binary_download in remote.c).  By
sending a zero length read, a remote target that supports 'x' packets,
and implements GDB's approach, will reply with a single 'b'
character.  If we get back anything else then GDB can just disable 'x'
packet support and fall back to the slower 'm' packet.

In the future remote targets can, if they want, check to see if the
client is GDB or LLDB and then modify their handling of 'x' packets in
order to support both debuggers.

I have not modified gdbserver at all.  I don't know if it is possible
to connect LLDB to gdbserver (I'm guessing not), but if a user tried
such a thing, and LLDB sent an 'x' packet, then gdbserver will send
back a reply with a 'b' prefix, which is going to confuse LLDB.  Until
a user complains about this then I'm going to assume that LLDB and
gdbserver is not a supported combination.

I have added a paragraph to the docs to explain that GDB probes using
a zero length read.  I feel this is important so that targets don't
assume zero length accesses are an error.  Plus the text makes it
clear how GDB expects the access to be handled.

For testing I have tried connecting to gdbserver and checked the
packet log.  I can see the zero length probe, then GDB continues using
'x' packets as normal.

I then modified gdbserver to stop it sending back the 'b' prefix.  Now
I see the zero length probe, and after that GDB switches to using the
'm' packet instead and 'show remote binary-upload-packet' shows the
packet as disabled.

I also built the latest version of `rr` and tested using current HEAD
of master, where I see problems like this:

  (rr) x/10i main
     0x401106 <main>:	Cannot access memory at address 0x401106

Then tested using this patched version of GDB, and now I see:

  (rr) x/10i main
     0x401106 <main>:	push   %rbp
     0x401107 <main+1>:	mov    %rsp,%rbp
     0x40110a <main+4>:	mov    0x2f17(%rip),%rax        # 0x404028 <global_ptr>
     ... etc ...

and looking in the remote log I now see GDB probe with a zero length
access and then fall-back to using 'm' packets.
---
 gdb/doc/gdb.texinfo |   4 ++
 gdb/remote.c        | 106 ++++++++++++++++++++++++++++++--------------
 2 files changed, 77 insertions(+), 33 deletions(-)


base-commit: 95db53e5a03dacef594430f28281748f3111c1f6
  

Comments

Eli Zaretskii Jan. 23, 2025, 8:22 p.m. UTC | #1
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: tankut.baris.aktemur@intel.com, Robert O'Callahan <robert@ocallahan.org>,
>  Andrew Burgess <aburgess@redhat.com>
> Date: Thu, 23 Jan 2025 19:36:45 +0000
> 
>  gdb/doc/gdb.texinfo |   4 ++
>  gdb/remote.c        | 106 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 77 insertions(+), 33 deletions(-)

OK for the documentation part, thanks.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  
Joel Brobecker Jan. 24, 2025, 3:12 a.m. UTC | #2
Hi Andrew,

Quick request, since this work is intended to also be backported to
the gdb-16-branch: Could we track this work with a PR in Bugzilla,
with the milestone set to GDB 16.2?

This is something we said we would do for all corrective releases
(track all changes with a bugzilla PR). That way, all corrections
are guaranteed to be tracked. On my end, I use this to make sure
I don't miss a pending fix, for instance, and also to generate
the list of fixes for the corrective release.

Thank you!
  
Aktemur, Tankut Baris Jan. 24, 2025, 8:29 a.m. UTC | #3
On Thursday, January 23, 2025 8:37 PM, Andrew Burgess wrote:
> This mailing list discussion:
> 
>   https://inbox.sourceware.org/gdb-patches/CAOp6jLYD0g-GUsx7jhO3g8H_4pHkB6dkh51cbyDT-
> 5yMfQwu+A@mail.gmail.com
> 
> highlighted the following issue with GDB's 'x' packet implementation.
> 
> Unfortunately, LLDB also has an 'x' packet, but their implementation
> is different to GDB's and so targets that have implemented LLDB's 'x'
> packet are incompatible with GDB.
> 
> The above thread is specifically about the 'rr' tool, but there could
> be other remote targets out there that have this problem.
> 
> The difference between LLDB and GDB is that GDB expects a 'b' prefix
> on the reply data, while LLDB does not.  The 'b' is important as it
> allows GDB to distinguish between an empty reply (which will be a 'b'
> prefix with no trailing data) and an unsupported packet (which will be
> a completely empty packet).  It is not clear to me how LLDB
> distinguishes these two cases.
> 
> See for discussion of the 'x' packet:
> 
>   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 propose that we start probing for 'x' packet support by sending a
> zero length 'x' packet read.  We already do something similar for the
> 'X' packet (see remote_target::check_binary_download in remote.c).  By
> sending a zero length read, a remote target that supports 'x' packets,
> and implements GDB's approach, will reply with a single 'b'
> character.  If we get back anything else then GDB can just disable 'x'
> packet support and fall back to the slower 'm' packet.
> 
> In the future remote targets can, if they want, check to see if the
> client is GDB or LLDB and then modify their handling of 'x' packets in
> order to support both debuggers.
> 
> I have not modified gdbserver at all.  I don't know if it is possible
> to connect LLDB to gdbserver (I'm guessing not), but if a user tried
> such a thing, and LLDB sent an 'x' packet, then gdbserver will send
> back a reply with a 'b' prefix, which is going to confuse LLDB.  Until
> a user complains about this then I'm going to assume that LLDB and
> gdbserver is not a supported combination.
> 
> I have added a paragraph to the docs to explain that GDB probes using
> a zero length read.  I feel this is important so that targets don't
> assume zero length accesses are an error.  Plus the text makes it
> clear how GDB expects the access to be handled.
> 
> For testing I have tried connecting to gdbserver and checked the
> packet log.  I can see the zero length probe, then GDB continues using
> 'x' packets as normal.
> 
> I then modified gdbserver to stop it sending back the 'b' prefix.  Now
> I see the zero length probe, and after that GDB switches to using the
> 'm' packet instead and 'show remote binary-upload-packet' shows the
> packet as disabled.
> 
> I also built the latest version of `rr` and tested using current HEAD
> of master, where I see problems like this:
> 
>   (rr) x/10i main
>      0x401106 <main>:	Cannot access memory at address 0x401106
> 
> Then tested using this patched version of GDB, and now I see:
> 
>   (rr) x/10i main
>      0x401106 <main>:	push   %rbp
>      0x401107 <main+1>:	mov    %rsp,%rbp
>      0x40110a <main+4>:	mov    0x2f17(%rip),%rax        # 0x404028 <global_ptr>
>      ... etc ...
> 
> and looking in the remote log I now see GDB probe with a zero length
> access and then fall-back to using 'm' packets.
> ---
>  gdb/doc/gdb.texinfo |   4 ++
>  gdb/remote.c        | 106 ++++++++++++++++++++++++++++++--------------
>  2 files changed, 77 insertions(+), 33 deletions(-)
> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index b65124d807f..79403a85db1 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 no data.  @value{GDBN} sends a zero length
> +@samp{x} packet to probe for support of this packet type.
> +

While we are at it, can we please clarify and document the zero-length
write behavior, too?  I had previously sent a patch for that but it did
not reach a conclusion:

  https://inbox.sourceware.org/gdb-patches/20240321133018.602537-1-tankut.baris.aktemur@intel.com/

>  Reply:
>  @table @samp
>  @item b @var{XX@dots{}}
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 64622dbfcdf..bd849c0cbbd 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -1329,6 +1329,7 @@ class remote_target : public process_stratum_target
>    void set_remote_traceframe ();
> 
>    void check_binary_download (CORE_ADDR addr);
> +  void check_binary_upload (CORE_ADDR addr);
> 
>    target_xfer_status remote_write_bytes_aux (const char *header,
>  					     CORE_ADDR memaddr,
> @@ -9411,6 +9412,65 @@ remote_target::check_binary_download (CORE_ADDR addr)
>      }
>  }
> 
> +/* Similar to check_binary_download, determine whether the remote target
> +   supports binary uploading ('x' packet).  This is accomplished by sending
> +   a no-op memory read of zero length to the target at the specified
> +   address ADDR.
> +
> +   Unfortunately we cannot just send the complete packet (with the actual
> +   length) due to divergence between GDB and LLDB.  Both debuggers support
> +   the 'x' packet, but disagree about the expected reply format.  GDB
> +   requires a 'b' prefix on the returned data in order to distinguish
> +   between an unsupported packet (empty), and a failed, or zero length read
> +   ('b' followed by no data).  It's not clear how LLDB distinguishes these
> +   two cases.  However, there are remote targets in the wild that use
> +   LLDB's reply format, which is not compatible with GDB.  For those
> +   targets we will fall back to the slower 'm' packet.
> +
> +   Send the zero length read request.  If we don't get back a packet
> +   containing just 'b', then we assume that 'x' packets are not
> +   supported.  */
> +
> +void
> +remote_target::check_binary_upload (CORE_ADDR addr)
> +{
> +  struct remote_state *rs = get_remote_state ();
> +
> +  switch (m_features.packet_support (PACKET_x))
> +    {
> +    case PACKET_DISABLE:
> +      break;
> +    case PACKET_ENABLE:
> +      break;
> +    case PACKET_SUPPORT_UNKNOWN:
> +      {
> +	char *p = rs->buf.data ();
> +	*p++ = 'x';
> +	p += hexnumstr (p, (ULONGEST) addr);

I feel uneasy about using the passed address here, which is an arbitrary
value.  LLDB docs special-cased x0,0 and not just the zero-length case
with an arbitrary address.  How about then using 0 as the address?

I also think that we should change gdbserver, too, to check in the handling
of the 'x' packet if the length is 0 and return a 'b' with empty data immediately.
The server currently attempts to read the memory first and may return an error
if it fails.  This would conflict the documentation.

IMHO, the zero-length read/write cases are not so clear and would bring questions
about whether it should be an empty success or an error, and what it means to access
a particular (in)valid address with zero-length.  I think it is fair to say that
if we did not have the rr breakage and had to choose between the current GDB
definition of the 'x' packet and LLDB's definition, we'd choose GDB's because 
it is better defined for the error and non-support cases.

For these reasons, I tend to think that in fact your proposed approach #1
(with a "gdb-x+" support announcement) was a better way to fix the breakage.

Thank you.
-Baris


> +	*p++ = ',';
> +	p += hexnumstr (p, (ULONGEST) 0);
> +	*p = '\0';
> +
> +	putpkt (rs->buf);
> +	getpkt (&rs->buf);
> +
> +	/* 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 uploading is supported by target");
> +	    m_features.m_protocol_packets[PACKET_x].support = PACKET_ENABLE;
> +	  }
> +	else
> +	  {
> +	    remote_debug_printf ("binary uploading NOT supported by target");
> +	    m_features.m_protocol_packets[PACKET_x].support = PACKET_DISABLE;
> +	  }
> +	break;
> +      }
> +    }
> +}
> +
>  /* Helper function to resize the payload in order to try to get a good
>     alignment.  We try to write an amount of data such that the next write will
>     start on an address aligned on REMOTE_ALIGN_WRITES.  */
> @@ -9682,17 +9742,9 @@ 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
> -    {
> -      char *buffer = rs->buf.data ();
> -      *buffer++ = format;
> -      buffer += hexnumstr (buffer, (ULONGEST) memaddr);
> -      *buffer++ = ',';
> -      buffer += hexnumstr (buffer, (ULONGEST) todo_units);
> -      *buffer = '\0';
> -      putpkt (rs->buf);
> -    };
> +  /* Check whether the target supports binary uploading.  */
> +  check_binary_upload (memaddr);
> +  gdb_assert (m_features.packet_support (PACKET_x) != PACKET_SUPPORT_UNKNOWN);
> 
>    /* 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
> @@ -9703,32 +9755,20 @@ remote_target::remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte
> *myaddr,
>    else
>      packet_format = 'x';
> 
> -  send_request (packet_format);
> +  /* Construct "m/x"<memaddr>","<len>".  */
> +  char *buffer = rs->buf.data ();
> +  *buffer++ = packet_format;
> +  buffer += hexnumstr (buffer, (ULONGEST) memaddr);
> +  *buffer++ = ',';
> +  buffer += hexnumstr (buffer, (ULONGEST) todo_units);
> +  *buffer = '\0';
> +  putpkt (rs->buf);
> +
> +  /* Fetch and process the reply.  */
>    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;
> 
> base-commit: 95db53e5a03dacef594430f28281748f3111c1f6
> --
> 2.47.1

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
  
Andrew Burgess Jan. 24, 2025, 3:23 p.m. UTC | #4
"Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com> writes:

> On Thursday, January 23, 2025 8:37 PM, Andrew Burgess wrote:
>> This mailing list discussion:
>> 
>>   https://inbox.sourceware.org/gdb-patches/CAOp6jLYD0g-GUsx7jhO3g8H_4pHkB6dkh51cbyDT-
>> 5yMfQwu+A@mail.gmail.com
>> 
>> highlighted the following issue with GDB's 'x' packet implementation.
>> 
>> Unfortunately, LLDB also has an 'x' packet, but their implementation
>> is different to GDB's and so targets that have implemented LLDB's 'x'
>> packet are incompatible with GDB.
>> 
>> The above thread is specifically about the 'rr' tool, but there could
>> be other remote targets out there that have this problem.
>> 
>> The difference between LLDB and GDB is that GDB expects a 'b' prefix
>> on the reply data, while LLDB does not.  The 'b' is important as it
>> allows GDB to distinguish between an empty reply (which will be a 'b'
>> prefix with no trailing data) and an unsupported packet (which will be
>> a completely empty packet).  It is not clear to me how LLDB
>> distinguishes these two cases.
>> 
>> See for discussion of the 'x' packet:
>> 
>>   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 propose that we start probing for 'x' packet support by sending a
>> zero length 'x' packet read.  We already do something similar for the
>> 'X' packet (see remote_target::check_binary_download in remote.c).  By
>> sending a zero length read, a remote target that supports 'x' packets,
>> and implements GDB's approach, will reply with a single 'b'
>> character.  If we get back anything else then GDB can just disable 'x'
>> packet support and fall back to the slower 'm' packet.
>> 
>> In the future remote targets can, if they want, check to see if the
>> client is GDB or LLDB and then modify their handling of 'x' packets in
>> order to support both debuggers.
>> 
>> I have not modified gdbserver at all.  I don't know if it is possible
>> to connect LLDB to gdbserver (I'm guessing not), but if a user tried
>> such a thing, and LLDB sent an 'x' packet, then gdbserver will send
>> back a reply with a 'b' prefix, which is going to confuse LLDB.  Until
>> a user complains about this then I'm going to assume that LLDB and
>> gdbserver is not a supported combination.
>> 
>> I have added a paragraph to the docs to explain that GDB probes using
>> a zero length read.  I feel this is important so that targets don't
>> assume zero length accesses are an error.  Plus the text makes it
>> clear how GDB expects the access to be handled.
>> 
>> For testing I have tried connecting to gdbserver and checked the
>> packet log.  I can see the zero length probe, then GDB continues using
>> 'x' packets as normal.
>> 
>> I then modified gdbserver to stop it sending back the 'b' prefix.  Now
>> I see the zero length probe, and after that GDB switches to using the
>> 'm' packet instead and 'show remote binary-upload-packet' shows the
>> packet as disabled.
>> 
>> I also built the latest version of `rr` and tested using current HEAD
>> of master, where I see problems like this:
>> 
>>   (rr) x/10i main
>>      0x401106 <main>:	Cannot access memory at address 0x401106
>> 
>> Then tested using this patched version of GDB, and now I see:
>> 
>>   (rr) x/10i main
>>      0x401106 <main>:	push   %rbp
>>      0x401107 <main+1>:	mov    %rsp,%rbp
>>      0x40110a <main+4>:	mov    0x2f17(%rip),%rax        # 0x404028 <global_ptr>
>>      ... etc ...
>> 
>> and looking in the remote log I now see GDB probe with a zero length
>> access and then fall-back to using 'm' packets.
>> ---
>>  gdb/doc/gdb.texinfo |   4 ++
>>  gdb/remote.c        | 106 ++++++++++++++++++++++++++++++--------------
>>  2 files changed, 77 insertions(+), 33 deletions(-)
>> 
>> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
>> index b65124d807f..79403a85db1 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 no data.  @value{GDBN} sends a zero length
>> +@samp{x} packet to probe for support of this packet type.
>> +
>
> While we are at it, can we please clarify and document the zero-length
> write behavior, too?  I had previously sent a patch for that but it did
> not reach a conclusion:
>
>   https://inbox.sourceware.org/gdb-patches/20240321133018.602537-1-tankut.baris.aktemur@intel.com/

Yeah, that's unfortunate.  I think my position is still the same, that
we shouldn't be change something that used to succeed into something
that gives an error.

>
>>  Reply:
>>  @table @samp
>>  @item b @var{XX@dots{}}
>> diff --git a/gdb/remote.c b/gdb/remote.c
>> index 64622dbfcdf..bd849c0cbbd 100644
>> --- a/gdb/remote.c
>> +++ b/gdb/remote.c
>> @@ -1329,6 +1329,7 @@ class remote_target : public process_stratum_target
>>    void set_remote_traceframe ();
>> 
>>    void check_binary_download (CORE_ADDR addr);
>> +  void check_binary_upload (CORE_ADDR addr);
>> 
>>    target_xfer_status remote_write_bytes_aux (const char *header,
>>  					     CORE_ADDR memaddr,
>> @@ -9411,6 +9412,65 @@ remote_target::check_binary_download (CORE_ADDR addr)
>>      }
>>  }
>> 
>> +/* Similar to check_binary_download, determine whether the remote target
>> +   supports binary uploading ('x' packet).  This is accomplished by sending
>> +   a no-op memory read of zero length to the target at the specified
>> +   address ADDR.
>> +
>> +   Unfortunately we cannot just send the complete packet (with the actual
>> +   length) due to divergence between GDB and LLDB.  Both debuggers support
>> +   the 'x' packet, but disagree about the expected reply format.  GDB
>> +   requires a 'b' prefix on the returned data in order to distinguish
>> +   between an unsupported packet (empty), and a failed, or zero length read
>> +   ('b' followed by no data).  It's not clear how LLDB distinguishes these
>> +   two cases.  However, there are remote targets in the wild that use
>> +   LLDB's reply format, which is not compatible with GDB.  For those
>> +   targets we will fall back to the slower 'm' packet.
>> +
>> +   Send the zero length read request.  If we don't get back a packet
>> +   containing just 'b', then we assume that 'x' packets are not
>> +   supported.  */
>> +
>> +void
>> +remote_target::check_binary_upload (CORE_ADDR addr)
>> +{
>> +  struct remote_state *rs = get_remote_state ();
>> +
>> +  switch (m_features.packet_support (PACKET_x))
>> +    {
>> +    case PACKET_DISABLE:
>> +      break;
>> +    case PACKET_ENABLE:
>> +      break;
>> +    case PACKET_SUPPORT_UNKNOWN:
>> +      {
>> +	char *p = rs->buf.data ();
>> +	*p++ = 'x';
>> +	p += hexnumstr (p, (ULONGEST) addr);
>
> I feel uneasy about using the passed address here, which is an arbitrary
> value.  LLDB docs special-cased x0,0 and not just the zero-length case
> with an arbitrary address.  How about then using 0 as the address?

The 'X' probe already uses the address we're about to access.  I don't
think we should be handling address 0 differently from any other
address, there are targets where accesses to 0 are valid, so if we start
to special case that address we just introduce weird behaviours.

Better (IMHO) to specify that the length should be checked before the
address, and that zero length accesses always succeed.

> I also think that we should change gdbserver, too, to check in the handling
> of the 'x' packet if the length is 0 and return a 'b' with empty data immediately.
> The server currently attempts to read the memory first and may return an error
> if it fails.  This would conflict the documentation.

It looks like the traceframe support does indeed not check for zero
length accesses.  And there is the case where we have no process.  In
that case even a zero length access will fail.

... but if we make is to read_inferior_memory then we do special case
zero length reads.

>
> IMHO, the zero-length read/write cases are not so clear and would bring questions
> about whether it should be an empty success or an error, and what it means to access
> a particular (in)valid address with zero-length.

I agree that this needs to be clearly documented and it's not "obvious"
what the right behaviour is.  It isn't great that the zero length write
for 'X' probing isn't documented.

>                                                    I think it is fair to say that
> if we did not have the rr breakage and had to choose between the current GDB
> definition of the 'x' packet and LLDB's definition, we'd choose GDB's because 
> it is better defined for the error and non-support cases.

I agree.  I don't understand how LLDB's 'x' packet can work in all cases.

> For these reasons, I tend to think that in fact your proposed approach #1
> (with a "gdb-x+" support announcement) was a better way to fix the
> breakage.

I'll roll a patch which tries this approach and we can see which we
prefer, though I don't think I share your concerns about the probing
approach.

Thanks,
Andrew

>
> Thank you.
> -Baris
>
>
>> +	*p++ = ',';
>> +	p += hexnumstr (p, (ULONGEST) 0);
>> +	*p = '\0';
>> +
>> +	putpkt (rs->buf);
>> +	getpkt (&rs->buf);
>> +
>> +	/* 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 uploading is supported by target");
>> +	    m_features.m_protocol_packets[PACKET_x].support = PACKET_ENABLE;
>> +	  }
>> +	else
>> +	  {
>> +	    remote_debug_printf ("binary uploading NOT supported by target");
>> +	    m_features.m_protocol_packets[PACKET_x].support = PACKET_DISABLE;
>> +	  }
>> +	break;
>> +      }
>> +    }
>> +}
>> +
>>  /* Helper function to resize the payload in order to try to get a good
>>     alignment.  We try to write an amount of data such that the next write will
>>     start on an address aligned on REMOTE_ALIGN_WRITES.  */
>> @@ -9682,17 +9742,9 @@ 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
>> -    {
>> -      char *buffer = rs->buf.data ();
>> -      *buffer++ = format;
>> -      buffer += hexnumstr (buffer, (ULONGEST) memaddr);
>> -      *buffer++ = ',';
>> -      buffer += hexnumstr (buffer, (ULONGEST) todo_units);
>> -      *buffer = '\0';
>> -      putpkt (rs->buf);
>> -    };
>> +  /* Check whether the target supports binary uploading.  */
>> +  check_binary_upload (memaddr);
>> +  gdb_assert (m_features.packet_support (PACKET_x) != PACKET_SUPPORT_UNKNOWN);
>> 
>>    /* 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
>> @@ -9703,32 +9755,20 @@ remote_target::remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte
>> *myaddr,
>>    else
>>      packet_format = 'x';
>> 
>> -  send_request (packet_format);
>> +  /* Construct "m/x"<memaddr>","<len>".  */
>> +  char *buffer = rs->buf.data ();
>> +  *buffer++ = packet_format;
>> +  buffer += hexnumstr (buffer, (ULONGEST) memaddr);
>> +  *buffer++ = ',';
>> +  buffer += hexnumstr (buffer, (ULONGEST) todo_units);
>> +  *buffer = '\0';
>> +  putpkt (rs->buf);
>> +
>> +  /* Fetch and process the reply.  */
>>    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;
>> 
>> base-commit: 95db53e5a03dacef594430f28281748f3111c1f6
>> --
>> 2.47.1
>
> 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
  
Andrew Burgess Jan. 24, 2025, 3:35 p.m. UTC | #5
Joel Brobecker <brobecker@adacore.com> writes:

> Hi Andrew,
>
> Quick request, since this work is intended to also be backported to
> the gdb-16-branch: Could we track this work with a PR in Bugzilla,
> with the milestone set to GDB 16.2?
>
> This is something we said we would do for all corrective releases
> (track all changes with a bugzilla PR). That way, all corrections
> are guaranteed to be tracked. On my end, I use this to make sure
> I don't miss a pending fix, for instance, and also to generate
> the list of fixes for the corrective release.

I created bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32593

but 16.2 isn't available as a milestone, so that bit I cannot do.

Thanks,
Andrew


>
> Thank you!
> -- 
> Joel
>
> PS: A big thank you for the great reactivity!
  
Aktemur, Tankut Baris Jan. 24, 2025, 3:36 p.m. UTC | #6
On Friday, January 24, 2025 4:24 PM, Andrew Burgess wrote:
> >> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> >> index b65124d807f..79403a85db1 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 no data.  @value{GDBN} sends a zero length
> >> +@samp{x} packet to probe for support of this packet type.
> >> +
> >
> > While we are at it, can we please clarify and document the zero-length
> > write behavior, too?  I had previously sent a patch for that but it did
> > not reach a conclusion:
> >
> >   https://inbox.sourceware.org/gdb-patches/20240321133018.602537-1-
> tankut.baris.aktemur@intel.com/
> 
> Yeah, that's unfortunate.  I think my position is still the same, that
> we shouldn't be change something that used to succeed into something
> that gives an error.

That's certainly a reasonable argument.  Frankly, I'm equally fine with defining
it to succeed or fail, as long as the definition is clear, and the doc and the
implementation are in sync.  The reason I had sent a patch that was defining it
to give an error was because Tom had stated that erroring made more sense to him.
I think this is another evidence that zero-length accesses are not obvious to
define.

Regards
-Baris


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
  
Joel Brobecker Jan. 27, 2025, 3:26 a.m. UTC | #7
> I created bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32593
> 
> but 16.2 isn't available as a milestone, so that bit I cannot do.

We had to contact overseers for that, but now that it's been added,
I took care of setting 16.2 as the target milestone for the issue.
  
Eli Zaretskii Jan. 27, 2025, 12:33 p.m. UTC | #8
> Date: Mon, 27 Jan 2025 07:26:03 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: gdb-patches@sourceware.org
> 
> > I created bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32593
> > 
> > but 16.2 isn't available as a milestone, so that bit I cannot do.
> 
> We had to contact overseers for that, but now that it's been added,
> I took care of setting 16.2 as the target milestone for the issue.

Is there some kind of ETA for GDB 16.2?  I understand that its release
schedule should be "soon", but do you have any estimation as to when?

Thanks.
  
Joel Brobecker Jan. 27, 2025, 1:36 p.m. UTC | #9
> > > I created bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32593
> > > 
> > > but 16.2 isn't available as a milestone, so that bit I cannot do.
> > 
> > We had to contact overseers for that, but now that it's been added,
> > I took care of setting 16.2 as the target milestone for the issue.
> 
> Is there some kind of ETA for GDB 16.2?  I understand that its release
> schedule should be "soon", but do you have any estimation as to when?

If the issue with the 'x' packet is resolved this week, I would be able
to produce the release this weekend. Otherwise, the weekend immediately
following the resolution of this issue.

If there are other important issues to fix, we'll have to decide on
a case-by-case basis whether it's worth delaying the 16.2 release,
where we know certain categories of users are very likely to hit
an issue that's difficult for them to diagnose and work around.

This .2 release will be an extra release, in the sense that we will
still want to do the corrective release we typically do about 3 months
after the .1. This time around, it'll be a .3.
  
Eli Zaretskii Jan. 27, 2025, 2:17 p.m. UTC | #10
> Date: Mon, 27 Jan 2025 17:36:20 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> Cc: Joel Brobecker <brobecker@adacore.com>, aburgess@redhat.com,
> 	gdb-patches@sourceware.org
> 
> > Is there some kind of ETA for GDB 16.2?  I understand that its release
> > schedule should be "soon", but do you have any estimation as to when?
> 
> If the issue with the 'x' packet is resolved this week, I would be able
> to produce the release this weekend. Otherwise, the weekend immediately
> following the resolution of this issue.
> 
> If there are other important issues to fix, we'll have to decide on
> a case-by-case basis whether it's worth delaying the 16.2 release,
> where we know certain categories of users are very likely to hit
> an issue that's difficult for them to diagnose and work around.
> 
> This .2 release will be an extra release, in the sense that we will
> still want to do the corrective release we typically do about 3 months
> after the .1. This time around, it'll be a .3.

Thanks, this is good news.
  

Patch

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b65124d807f..79403a85db1 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 no data.  @value{GDBN} sends a zero length
+@samp{x} packet to probe for support of this packet type.
+
 Reply:
 @table @samp
 @item b @var{XX@dots{}}
diff --git a/gdb/remote.c b/gdb/remote.c
index 64622dbfcdf..bd849c0cbbd 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -1329,6 +1329,7 @@  class remote_target : public process_stratum_target
   void set_remote_traceframe ();
 
   void check_binary_download (CORE_ADDR addr);
+  void check_binary_upload (CORE_ADDR addr);
 
   target_xfer_status remote_write_bytes_aux (const char *header,
 					     CORE_ADDR memaddr,
@@ -9411,6 +9412,65 @@  remote_target::check_binary_download (CORE_ADDR addr)
     }
 }
 
+/* Similar to check_binary_download, determine whether the remote target
+   supports binary uploading ('x' packet).  This is accomplished by sending
+   a no-op memory read of zero length to the target at the specified
+   address ADDR.
+
+   Unfortunately we cannot just send the complete packet (with the actual
+   length) due to divergence between GDB and LLDB.  Both debuggers support
+   the 'x' packet, but disagree about the expected reply format.  GDB
+   requires a 'b' prefix on the returned data in order to distinguish
+   between an unsupported packet (empty), and a failed, or zero length read
+   ('b' followed by no data).  It's not clear how LLDB distinguishes these
+   two cases.  However, there are remote targets in the wild that use
+   LLDB's reply format, which is not compatible with GDB.  For those
+   targets we will fall back to the slower 'm' packet.
+
+   Send the zero length read request.  If we don't get back a packet
+   containing just 'b', then we assume that 'x' packets are not
+   supported.  */
+
+void
+remote_target::check_binary_upload (CORE_ADDR addr)
+{
+  struct remote_state *rs = get_remote_state ();
+
+  switch (m_features.packet_support (PACKET_x))
+    {
+    case PACKET_DISABLE:
+      break;
+    case PACKET_ENABLE:
+      break;
+    case PACKET_SUPPORT_UNKNOWN:
+      {
+	char *p = rs->buf.data ();
+	*p++ = 'x';
+	p += hexnumstr (p, (ULONGEST) addr);
+	*p++ = ',';
+	p += hexnumstr (p, (ULONGEST) 0);
+	*p = '\0';
+
+	putpkt (rs->buf);
+	getpkt (&rs->buf);
+
+	/* 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 uploading is supported by target");
+	    m_features.m_protocol_packets[PACKET_x].support = PACKET_ENABLE;
+	  }
+	else
+	  {
+	    remote_debug_printf ("binary uploading NOT supported by target");
+	    m_features.m_protocol_packets[PACKET_x].support = PACKET_DISABLE;
+	  }
+	break;
+      }
+    }
+}
+
 /* Helper function to resize the payload in order to try to get a good
    alignment.  We try to write an amount of data such that the next write will
    start on an address aligned on REMOTE_ALIGN_WRITES.  */
@@ -9682,17 +9742,9 @@  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
-    {
-      char *buffer = rs->buf.data ();
-      *buffer++ = format;
-      buffer += hexnumstr (buffer, (ULONGEST) memaddr);
-      *buffer++ = ',';
-      buffer += hexnumstr (buffer, (ULONGEST) todo_units);
-      *buffer = '\0';
-      putpkt (rs->buf);
-    };
+  /* Check whether the target supports binary uploading.  */
+  check_binary_upload (memaddr);
+  gdb_assert (m_features.packet_support (PACKET_x) != PACKET_SUPPORT_UNKNOWN);
 
   /* 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
@@ -9703,32 +9755,20 @@  remote_target::remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr,
   else
     packet_format = 'x';
 
-  send_request (packet_format);
+  /* Construct "m/x"<memaddr>","<len>".  */
+  char *buffer = rs->buf.data ();
+  *buffer++ = packet_format;
+  buffer += hexnumstr (buffer, (ULONGEST) memaddr);
+  *buffer++ = ',';
+  buffer += hexnumstr (buffer, (ULONGEST) todo_units);
+  *buffer = '\0';
+  putpkt (rs->buf);
+
+  /* Fetch and process the reply.  */
   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;