[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
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
> 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>
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
@@ -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
@@ -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::
@@ -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;
@@ -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 ())