[v2] Add "error_message+" feature to qSupported

Message ID 20240509121126.78317-1-ahajkova@khirnov.net
State New
Headers
Series [v2] Add "error_message+" feature to qSupported |

Checks

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

Commit Message

Alexandra Hájková May 9, 2024, 12:11 p.m. UTC
  From: Alexandra Hájková <ahajkova@redhat.com>

Check if the gdbserver GDB is communicating with supports
responding with the error message in an E.message format to GDB's
packets.

Add a new 'error_message' feature to the qSupported packet. When GDB
and gdbserver support this feature then gdbserver is able to send
errors in the E.message format for the qRcmd and m packets.

Update qRcmd packet and m packets documentation as qRcmd newly
accepts errors in a E.message format.
Previously these two packets didn't support E.message style errors.
---

v2:
- Add two spaces between sentences in documentation when missing.
- Use @code{accept-error-message} instead of the quotes.
- Remove the ambiguous part about errno from the m packet documentation.
- Replace error_message+ with error-message+.
- Replace accept-error-message with just error-message.

 gdb/doc/gdb.texinfo | 29 +++++++++++++++++++++----
 gdb/remote.c        | 53 ++++++++++++++++++++++++++++++++++++++++++++-
 gdbserver/server.cc |  6 +++++
 gdbserver/server.h  |  5 +++++
 4 files changed, 88 insertions(+), 5 deletions(-)
  

Comments

Tom Tromey May 10, 2024, 6:01 p.m. UTC | #1
>>>>> "Alexandra" == Alexandra Hájková <ahajkova@khirnov.net> writes:

Thank you for the patch.

Alexandra> +@item E.errtext
Alexandra> +An error can be sent as a string.  This reply is only valid
Alexandra> +if the @code{error-message feature} (@pxref{error-message}) is enabled.

Should this instead read: "if the @code{error-message} feature"?
That is, I think @code should probably only apply to the feature name.

Other than this nit, this looks good to me.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
  
Pedro Alves May 10, 2024, 6:46 p.m. UTC | #2
On 2024-05-09 13:11, Alexandra Hájková wrote:

> 
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 61f91ef4ad6..4cf0140e588 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -42966,12 +42966,11 @@ Reply:
>  Memory contents; each byte is transmitted as a two-digit hexadecimal number.
>  The reply may contain fewer addressable memory units than requested if the
>  server was able to read only part of the region of memory.
> +@item E.errtext
> +An error can be sent as a string.  This reply is only valid
> +if the @code{error-message feature} (@pxref{error-message}) is enabled.
>  @end table
>  
> -Unlike most packets, this packet does not support
> -@samp{E.@var{errtext}}-style textual error replies (@pxref{textual
> -error reply}).
> -

I think it would make more sense to keep this in a separate paragraph.  If you put E.errtext
in the list of possible replies, when it's possible for a reader to confused wondering whether
ENN is meant to be supported.

I.e., adjust the paragraph like so:

 Unlike most packets, this packet does not support
 @samp{E.@var{errtext}}-style textual error replies (@pxref{textual
-error reply}).
+error reply}) by default.  Stubs should be careful to only send such a
+reply if @value{GDBN} reported support for it with the
+@code{error-message feature} (@pxref{error-message}).

>  @item M @var{addr},@var{length}:@var{XX@dots{}}
>  @cindex @samp{M} packet
>  Write @var{length} addressable memory units starting at address @var{addr}
> @@ -44476,6 +44475,13 @@ Reply:
>  A command response with no output.
>  @item @var{OUTPUT}
>  A command response with the hex encoded output string @var{OUTPUT}.
> +@item E @var{NN}
> +Indicate a badly formed request.  The error number @var{NN} is given as
> +hex digits.
> +@item E.errtext
> +Alternatively, an error can be sent as a string.  This reply is only valid
> +if the @code{error-message} feature (@pxref{error-message}) is enabled.
> +@item @w{}
>  @end table

Well, here you added ENN to the list, but ...

>  
>  Unlike most packets, this packet does not support

... kept the paragraph, which leaves behind a contradiction.  I think you shouldn't add
the items to the list, and instead should update the paragraph, like I described for
the m packet above.

> @@ -44627,6 +44633,17 @@ including @samp{exec-events+} in its @samp{qSupported} reply.
>  @item vContSupported
>  This feature indicates whether @value{GDBN} wants to know the
>  supported actions in the reply to @samp{vCont?} packet.
> +
> +@anchor{error-message}
> +@item error-message
> +This feature indicates whether @value{GDBN} supports accepting
> +a reply to a request in an @code{E.message} format including

"E.message" is actually documented as "E.errtext", like so:

 @anchor{textual error reply}
 @item @samp{E.@var{errtext}}
 An error has occurred; @var{errtext} is the textual error message,
 encoded in @sc{ascii}.

We should be consistent throughout.  Also, note there's an anchor here you can xref.

> +@samp{error-message+} in its @samp{qSupported} reply.  This
> +feature only covers the @code{qRcmd} and @code{m} packets, and exists for
> +backwards compatibility reasons.  Those packets, historically, didn't support
> +@code{E.message}.  New packets should be written to support
> +@code{E.message} regardless of this feature being true or not.
> +

Ditto.  Xref to anchor.

>  @end table
>  
>  Stubs should ignore any unknown values for
> @@ -45143,6 +45160,10 @@ inspected, if @samp{qIsAddressTagged} (@pxref{qIsAddressTagged}) packet
>  is not supported by the stub.  Access to the @file{/proc/@var{pid}/smaps}
>  file is done via @samp{vFile} requests.
>  
> +@item error-message
> +The remote stub supports replying with an error in a
> +@code{E.message} format.
> +

Ditto.  Xref.

>  @end table
>  
>  @item qSymbol::
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 55069559a60..3cd2060ff55 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -377,6 +377,18 @@ enum {
>    /* Support for the qIsAddressTagged packet.  */
>    PACKET_qIsAddressTagged,
>  
> +  /* Support for accepting error message in a E.message format.
> +     This allows every remote packet to return E.message.
> +
> +     This feature only exists to fix a backwards compatibility issue
> +     with the qRcmd and m packets.  Historically, these two packets didn't
> +     support E.message style errors, but when this feature is on
> +     these two packets can receive E.message style errors.
> +
> +     All new packets should be written to always accepts E.message style
> +     errors, and so they should not need to check for this feature.  */
> +  PACKET_accept_error_message,
> +
>    PACKET_MAX
>  };
>  
> @@ -798,6 +810,21 @@ struct remote_features
>    bool remote_memory_tagging_p () const
>    { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
>  
> +  /* Returns true if accepting E.message in the packet response is supported,
> +     false otherwise.
> +
> +     Don't call this function.  This function, and the feature it wraps
> +     around only exists to fix a backwards compatibility issue with the
> +     qRcmd and m packets.  Historically, these two packets didn't
> +     support E.message style errors, but when this feature is on (this
> +     function returns true) these two packets can receive E.message style
> +     errors.
> +
> +     All new packets should be written to always accepts E.message style

"always accepts" -> "always accept"

> +     errors, and so they should not need to check for this feature.  */
> +  bool accept_error_message_p () const
> +  { return packet_support (PACKET_accept_error_message) == PACKET_ENABLE; }
> +


Not really sure what is the point of gdbserver reporting the feature back to GDB.
What does it affect?
  

Patch

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 61f91ef4ad6..4cf0140e588 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42966,12 +42966,11 @@  Reply:
 Memory contents; each byte is transmitted as a two-digit hexadecimal number.
 The reply may contain fewer addressable memory units than requested if the
 server was able to read only part of the region of memory.
+@item E.errtext
+An error can be sent as a string.  This reply is only valid
+if the @code{error-message feature} (@pxref{error-message}) is enabled.
 @end table
 
-Unlike most packets, this packet does not support
-@samp{E.@var{errtext}}-style textual error replies (@pxref{textual
-error reply}).
-
 @item M @var{addr},@var{length}:@var{XX@dots{}}
 @cindex @samp{M} packet
 Write @var{length} addressable memory units starting at address @var{addr}
@@ -44476,6 +44475,13 @@  Reply:
 A command response with no output.
 @item @var{OUTPUT}
 A command response with the hex encoded output string @var{OUTPUT}.
+@item E @var{NN}
+Indicate a badly formed request.  The error number @var{NN} is given as
+hex digits.
+@item E.errtext
+Alternatively, an error can be sent as a string.  This reply is only valid
+if the @code{error-message} feature (@pxref{error-message}) is enabled.
+@item @w{}
 @end table
 
 Unlike most packets, this packet does not support
@@ -44627,6 +44633,17 @@  including @samp{exec-events+} in its @samp{qSupported} reply.
 @item vContSupported
 This feature indicates whether @value{GDBN} wants to know the
 supported actions in the reply to @samp{vCont?} packet.
+
+@anchor{error-message}
+@item error-message
+This feature indicates whether @value{GDBN} supports accepting
+a reply to a request in an @code{E.message} format including
+@samp{error-message+} in its @samp{qSupported} reply.  This
+feature only covers the @code{qRcmd} and @code{m} packets, and exists for
+backwards compatibility reasons.  Those packets, historically, didn't support
+@code{E.message}.  New packets should be written to support
+@code{E.message} regardless of this feature being true or not.
+
 @end table
 
 Stubs should ignore any unknown values for
@@ -45143,6 +45160,10 @@  inspected, if @samp{qIsAddressTagged} (@pxref{qIsAddressTagged}) packet
 is not supported by the stub.  Access to the @file{/proc/@var{pid}/smaps}
 file is done via @samp{vFile} requests.
 
+@item error-message
+The remote stub supports replying with an error in a
+@code{E.message} format.
+
 @end table
 
 @item qSymbol::
diff --git a/gdb/remote.c b/gdb/remote.c
index 55069559a60..3cd2060ff55 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -377,6 +377,18 @@  enum {
   /* Support for the qIsAddressTagged packet.  */
   PACKET_qIsAddressTagged,
 
+  /* Support for accepting error message in a E.message format.
+     This allows every remote packet to return E.message.
+
+     This feature only exists to fix a backwards compatibility issue
+     with the qRcmd and m packets.  Historically, these two packets didn't
+     support E.message style errors, but when this feature is on
+     these two packets can receive E.message style errors.
+
+     All new packets should be written to always accepts E.message style
+     errors, and so they should not need to check for this feature.  */
+  PACKET_accept_error_message,
+
   PACKET_MAX
 };
 
@@ -798,6 +810,21 @@  struct remote_features
   bool remote_memory_tagging_p () const
   { return packet_support (PACKET_memory_tagging_feature) == PACKET_ENABLE; }
 
+  /* Returns true if accepting E.message in the packet response is supported,
+     false otherwise.
+
+     Don't call this function.  This function, and the feature it wraps
+     around only exists to fix a backwards compatibility issue with the
+     qRcmd and m packets.  Historically, these two packets didn't
+     support E.message style errors, but when this feature is on (this
+     function returns true) these two packets can receive E.message style
+     errors.
+
+     All new packets should be written to always accepts E.message style
+     errors, and so they should not need to check for this feature.  */
+  bool accept_error_message_p () const
+  { return packet_support (PACKET_accept_error_message) == PACKET_ENABLE; }
+
   /* Reset all packets back to "unknown support".  Called when opening a
      new connection to a remote target.  */
   void reset_all_packet_configs_support ();
@@ -1389,6 +1416,20 @@  class remote_target : public process_stratum_target
 
   bool start_remote_1 (int from_tty, int extended_p);
 
+  /* Don't call this function.  This function, and the feature it wraps
+     around only exists to fix a backwards compatibility issue with the
+     qRcmd and m packets.  Historically, these two packets didn't
+     support E.message style errors, but when this feature is on (this
+     function returns true) these two packets can receive E.message style
+     errors.
+
+     All new packets should be written to always accepts E.message style
+     errors, and so they should not need to check for this feature.  */
+  bool supports_error_message ()
+  {
+    return m_features.accept_error_message_p ();
+  }
+
   /* The remote state.  Don't reference this directly.  Use the
      get_remote_state method instead.  */
   remote_state m_remote_state;
@@ -5815,6 +5856,8 @@  static const struct protocol_feature remote_protocol_features[] = {
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
   { "memory-tagging", PACKET_DISABLE, remote_supported_packet,
     PACKET_memory_tagging_feature },
+  { "error-message", PACKET_DISABLE, remote_supported_packet,
+    PACKET_accept_error_message },
 };
 
 static char *remote_support_xml;
@@ -5933,6 +5976,10 @@  remote_target::remote_query_supported ()
 	      != PACKET_DISABLE))
 	remote_query_supported_append (&q, remote_support_xml);
 
+      if (m_features.packet_set_cmd_state (PACKET_accept_error_message)
+	  != AUTO_BOOLEAN_FALSE)
+      remote_query_supported_append (&q, "error-message+");
+
       q = "qSupported:" + q;
       putpkt (q.c_str ());
 
@@ -9649,7 +9696,8 @@  remote_target::remote_read_bytes_1 (CORE_ADDR memaddr, gdb_byte *myaddr,
   *p = '\0';
   putpkt (rs->buf);
   getpkt (&rs->buf);
-  packet_result result = packet_check_result (rs->buf, false);
+  packet_result result = packet_check_result (rs->buf,
+					      supports_error_message ());
   if (result.status () == PACKET_ERROR)
     return TARGET_XFER_E_IO;
   /* Reply describes memory byte by byte, each byte encoded as two hex
@@ -16260,6 +16308,9 @@  Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_qIsAddressTagged,
 			 "qIsAddressTagged", "memory-tagging-address-check", 0);
 
+  add_packet_config_cmd (PACKET_accept_error_message,
+			 "error-message", "error-message", 0);
+
   /* Assert that we've registered "set remote foo-packet" commands
      for all packet configs.  */
   {
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 789af36d9a4..8891038976e 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2710,6 +2710,8 @@  handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
 		  if (target_supports_memory_tagging ())
 		    cs.memory_tagging_feature = true;
 		}
+	      else if (feature == "error-message+")
+		cs.error_message_supported = true;
 	      else
 		{
 		  /* Move the unknown features all together.  */
@@ -2839,6 +2841,9 @@  handle_query (char *own_buf, int packet_len, int *new_packet_len_p)
       if (target_supports_memory_tagging ())
 	strcat (own_buf, ";memory-tagging+");
 
+      if (cs.error_message_supported)
+	strcat (own_buf, ";error-message+");
+
       /* Reinitialize components as needed for the new connection.  */
       hostio_handle_new_gdb_connection ();
       target_handle_new_gdb_connection ();
@@ -4375,6 +4380,7 @@  captured_main (int argc, char *argv[])
       cs.hwbreak_feature = 0;
       cs.vCont_supported = 0;
       cs.memory_tagging_feature = false;
+      cs.error_message_supported = false;
 
       remote_open (port);
 
diff --git a/gdbserver/server.h b/gdbserver/server.h
index 0074818c6df..c39241c960d 100644
--- a/gdbserver/server.h
+++ b/gdbserver/server.h
@@ -192,6 +192,11 @@  struct client_state
   /* If true, memory tagging features are supported.  */
   bool memory_tagging_feature = false;
 
+  /* If true then E.message style errors are supported everywhere,
+     including for the qRcmd and m packet.  When false E.message errors
+     are not supported with qRcmd and m packets, but are still supported
+     everywhere else.  This is for backward compatibility reasons.  */
+  bool error_message_supported = false;
 };
 
 client_state &get_client_state ();