[PATCHv2] gdb/remote: add 'binary-upload' feature to guard 'x' packet use

Message ID a54e8855fec52917b68ba940231d0bc1e964de77.1737735670.git.aburgess@redhat.com
State New
Headers
Series [PATCHv2] gdb/remote: add 'binary-upload' feature to guard 'x' packet use |

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. 24, 2025, 4:21 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 add a new feature 'binary-upload' which can be
reported by a stub in its qSupported reply.  By default this feature
is "off", meaning GDB will not use the 'x' packet unless a stub
advertises this feature.

I have updated gdbserver to send 'binary-upload+', and when I examine
the gdbserver log I can see this feature being sent back, and then GDB
will use the 'x' packet.

When connecting to an older gdbserver, the feature is not sent, and
GDB does not try to use the 'x' packet at all.

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 see GDB is now using the 'm' packet
instead of the 'x' packet.
---
 gdb/NEWS            |  8 ++++++++
 gdb/doc/gdb.texinfo | 12 ++++++++++++
 gdb/remote.c        |  1 +
 gdbserver/server.cc |  2 +-
 4 files changed, 22 insertions(+), 1 deletion(-)


base-commit: 95db53e5a03dacef594430f28281748f3111c1f6
  

Comments

Eli Zaretskii Jan. 24, 2025, 4:46 p.m. UTC | #1
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: Andrew Burgess <aburgess@redhat.com>, tankut.baris.aktemur@intel.com,
>  Robert O'Callahan <robert@ocallahan.org>
> Date: Fri, 24 Jan 2025 16:21:55 +0000
> 
>  gdb/NEWS            |  8 ++++++++
>  gdb/doc/gdb.texinfo | 12 ++++++++++++
>  gdb/remote.c        |  1 +
>  gdbserver/server.cc |  2 +-
>  4 files changed, 22 insertions(+), 1 deletion(-)

OK for the documentation parts.

Reviewed-By: Eli Zaretskii <eliz@gnu.org>
  
Aktemur, Tankut Baris Jan. 28, 2025, 11:34 a.m. UTC | #2
On Friday, January 24, 2025 5:22 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 add a new feature 'binary-upload' which can be
> reported by a stub in its qSupported reply.  By default this feature
> is "off", meaning GDB will not use the 'x' packet unless a stub
> advertises this feature.
> 
> I have updated gdbserver to send 'binary-upload+', and when I examine
> the gdbserver log I can see this feature being sent back, and then GDB
> will use the 'x' packet.
> 
> When connecting to an older gdbserver, the feature is not sent, and
> GDB does not try to use the 'x' packet at all.
> 
> 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 see GDB is now using the 'm' packet
> instead of the 'x' packet.
> ---
>  gdb/NEWS            |  8 ++++++++
>  gdb/doc/gdb.texinfo | 12 ++++++++++++
>  gdb/remote.c        |  1 +
>  gdbserver/server.cc |  2 +-
>  4 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index eec7cf4695d..e20d9e9d307 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -46,6 +46,14 @@ maintenance check symtabs
>    ** New constant PARAM_COLOR represents color type of a value
>       of a <gdb:parameter> object.  Parameter's value is <gdb::color> instance.
> 
> +* New remote packets
> +
> +binary-upload in qSupported reply
> +  If the stub sends back 'binary-upload+' in it's qSupported reply,
> +  then GDB will, where possible, make use of the 'x' packet.  If the
> +  stub doesn't report this feature supported, then GDB will not use
> +  the 'x' packet.
> +
>  *** Changes in GDB 16
> 
>  * Support for Nios II targets has been removed as this architecture
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index b65124d807f..201ce02e934 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.
> 
> +@value{GDBN} will only use this packet if the stub reports the
> +@samp{binary-upload} feature is supported in its @samp{qSupported}
> +reply (@pxref{qSupported}).
> +
>  Reply:
>  @table @samp
>  @item b @var{XX@dots{}}
> @@ -45178,6 +45182,11 @@ General Query Packets
>  @tab @samp{+}
>  @tab No
> 
> +@item @samp{binary-upload}
> +@tab No
> +@tab @samp{-}
> +@tab No
> +
>  @end multitable
> 
>  These are the currently defined stub features, in more detail:
> @@ -45424,6 +45433,9 @@ General Query Packets
>  send this feature back to @value{GDBN} in the @samp{qSupported} reply,
>  @value{GDBN} will always support @samp{E.@var{errtext}} format replies
>  if it sent the @samp{error-message} feature.
> +
> +@item binary-upload
> +The remote stub supports the @samp{x} packet (@pxref{x packet}).
>  @end table
> 
>  @item qSymbol::
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 64622dbfcdf..f3687fbaa70 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -5847,6 +5847,7 @@ static const struct protocol_feature remote_protocol_features[] = {
>      PACKET_memory_tagging_feature },
>    { "error-message", PACKET_ENABLE, remote_supported_packet,
>      PACKET_accept_error_message },
> +  { "binary-upload", PACKET_DISABLE, remote_supported_packet, PACKET_x },
>  };
> 
>  static char *remote_support_xml;
> diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> index c1b18cc947e..0ad27d5e830 100644
> --- a/gdbserver/server.cc
> +++ b/gdbserver/server.cc
> @@ -2757,7 +2757,7 @@ handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
>  	       "PacketSize=%x;QPassSignals+;QProgramSignals+;"
>  	       "QStartupWithShell+;QEnvironmentHexEncoded+;"
>  	       "QEnvironmentReset+;QEnvironmentUnset+;"
> -	       "QSetWorkingDir+",
> +	       "QSetWorkingDir+;binary-upload+",
>  	       PBUFSIZ - 1);
> 
>        if (target_supports_catch_syscall ())
> 
> base-commit: 95db53e5a03dacef594430f28281748f3111c1f6
> --
> 2.47.1

This patch looks good to me.  Thank you.

Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

GDB still has the code that attempts to send the 'x' packet if the packet
state is UNKNOWN, to probe for support.  With this patch applied, in practice,
we'd expect the binary-upload feature to be either reported as supported by
the server or left unspecified, in which case it means the feature is unsupported.
So, in practice, we'd expect the packet to be either in the DISABLE state
(default) or ENABLE, but not UNKNOWN.  Hence, it may appear like the existing
probing behavior can be removed for simplification purposes, but from a pedantic
side, a server can send "binary-upload?" in the qSupported reply, and turn the 
packet state to UNKNOWN.  Therefore, I think it makes sense to keep the existing
implementation of the 'x' packet in ` remote_read_bytes_1` for completeness.

Just noting this as a comment.

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
  

Patch

diff --git a/gdb/NEWS b/gdb/NEWS
index eec7cf4695d..e20d9e9d307 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -46,6 +46,14 @@  maintenance check symtabs
   ** New constant PARAM_COLOR represents color type of a value
      of a <gdb:parameter> object.  Parameter's value is <gdb::color> instance.
 
+* New remote packets
+
+binary-upload in qSupported reply
+  If the stub sends back 'binary-upload+' in it's qSupported reply,
+  then GDB will, where possible, make use of the 'x' packet.  If the
+  stub doesn't report this feature supported, then GDB will not use
+  the 'x' packet.
+
 *** Changes in GDB 16
 
 * Support for Nios II targets has been removed as this architecture
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b65124d807f..201ce02e934 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.
 
+@value{GDBN} will only use this packet if the stub reports the
+@samp{binary-upload} feature is supported in its @samp{qSupported}
+reply (@pxref{qSupported}).
+
 Reply:
 @table @samp
 @item b @var{XX@dots{}}
@@ -45178,6 +45182,11 @@  General Query Packets
 @tab @samp{+}
 @tab No
 
+@item @samp{binary-upload}
+@tab No
+@tab @samp{-}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -45424,6 +45433,9 @@  General Query Packets
 send this feature back to @value{GDBN} in the @samp{qSupported} reply,
 @value{GDBN} will always support @samp{E.@var{errtext}} format replies
 if it sent the @samp{error-message} feature.
+
+@item binary-upload
+The remote stub supports the @samp{x} packet (@pxref{x packet}).
 @end table
 
 @item qSymbol::
diff --git a/gdb/remote.c b/gdb/remote.c
index 64622dbfcdf..f3687fbaa70 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -5847,6 +5847,7 @@  static const struct protocol_feature remote_protocol_features[] = {
     PACKET_memory_tagging_feature },
   { "error-message", PACKET_ENABLE, remote_supported_packet,
     PACKET_accept_error_message },
+  { "binary-upload", PACKET_DISABLE, remote_supported_packet, PACKET_x },
 };
 
 static char *remote_support_xml;
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index c1b18cc947e..0ad27d5e830 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2757,7 +2757,7 @@  handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 	       "PacketSize=%x;QPassSignals+;QProgramSignals+;"
 	       "QStartupWithShell+;QEnvironmentHexEncoded+;"
 	       "QEnvironmentReset+;QEnvironmentUnset+;"
-	       "QSetWorkingDir+",
+	       "QSetWorkingDir+;binary-upload+",
 	       PBUFSIZ - 1);
 
       if (target_supports_catch_syscall ())