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

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.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32593
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Reviewed-By: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>

diff --git a/gdb/NEWS b/gdb/NEWS
index bd408f1bb64424b88c06cd69eaa4cea4e04c772f..d69b0dd993de99f40e288d85e890c89e6d864bd0 100644 (file)
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -204,6 +204,12 @@ x addr,length
   equivalent to 'm', except that the data in the response are in
   binary format.
 
+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 15
 
 * The MPX commands "show/set mpx bound" have been deprecated, as Intel

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index b985399cf34039dbb416fd862d19198bf3954123..7b10d3ddf0900f2be4cad054ca8b988c3a8a6fd9 100644 (file)
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -43467,6 +43467,10 @@ and @var{length} is a multiple of the word size, the stub is free to
 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{}}
@@ -45114,6 +45118,11 @@ These are the currently defined stub features and their properties:
 @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:
@@ -45360,6 +45369,9 @@ The remote stub supports replying with an error in a
 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 d9ad6974366a514bfa46349a3b0eaf9e1f1f63b1..e00ddc047929cf42089ada337fa86a8782abc8fa 100644 (file)
--- 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 55898f59556b2d16bebe319e48321cdfb0cc9f89..0ad19e621de382571381777edb73489bcf062257 100644 (file)
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2777,7 +2777,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 ())