Add "error_message+" feature to qSupported

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

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

Approved-By: Tom Tromey <tom@tromey.com>
Approved-By: Andrew Burgess <aburgess@redhat.com>

4 files changed, 83 insertions, 34 deletions

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 750f368..86cd420 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42970,7 +42970,9 @@ server was able to read only part of the region of memory.
 
 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
@@ -44480,7 +44482,9 @@ A command response with the hex encoded output string @var{OUTPUT}.
 
 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}).
 
 (Note that the @code{qRcmd} packet's name is separated from the
 command by a @samp{,}, not a @samp{:}, contrary to the naming
@@ -44627,6 +44631,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
+in @samp{E.@var{errtext}} format (@xref{textual error reply}) from the
+@samp{qRcmd} and @samp{m} packets.  These packets, historically,
+didn't support @samp{E.@var{errtext}}, and older versions of
+@value{GDBN} didn't expect to see a reply in this format.
+
+New packets should be written to support @samp{E.@var{errtext}}
+regardless of this feature being true or not.
 @end table
 
 Stubs should ignore any unknown values for
@@ -44910,6 +44925,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab No
 
+@item @samp{error-message}
+@tab No
+@tab @samp{+}
+@tab No
+
 @end multitable
 
 These are the currently defined stub features, in more detail:
@@ -45143,6 +45163,13 @@ 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
+@samp{E.@var{errtext}} (@xref{textual error reply}) format from the
+@samp{m} and @samp{qRcmd} packets.  It is not usually necessary to
+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.
 @end table
 
 @item qSymbol::
diff --git a/gdb/remote.c b/gdb/remote.c
index d1d11fb..90a4bd5 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -379,6 +379,18 @@ enum {
   /* Support for the qIsAddressTagged packet.  */
   PACKET_qIsAddressTagged,
 
+  /* Support for accepting error message in a E.errtext format.
+     This allows every remote packet to return E.errtext.
+
+     This feature only exists to fix a backwards compatibility issue
+     with the qRcmd and m packets.  Historically, these two packets didn't
+     support E.errtext style errors, but when this feature is on
+     these two packets can receive E.errtext style errors.
+
+     All new packets should be written to always accept E.errtext style
+     errors, and so they should not need to check for this feature.  */
+  PACKET_accept_error_message,
+
   PACKET_MAX
 };
 
@@ -2502,10 +2514,9 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
    PACKET_ERROR case.
 
    An error packet can always take the form Exx (where xx is a hex
-   code).  When ACCEPT_MSG is true error messages can also take the
-   form E.msg (where msg is any arbitrary string).  */
+   code).  */
 static packet_result
-packet_check_result (const char *buf, bool accept_msg)
+packet_check_result (const char *buf)
 {
   if (buf[0] != '\0')
     {
@@ -2520,17 +2531,14 @@ packet_check_result (const char *buf, bool accept_msg)
       /* Not every request accepts an error in a E.msg form.
 	 Some packets accepts only Enn, in this case E. is not
 	 defined and E. is treated as PACKET_OK.  */
-      if (accept_msg)
+      /* Always treat "E." as an error.  This will be used for
+	 more verbose error messages, such as E.memtypes.  */
+      if (buf[0] == 'E' && buf[1] == '.')
 	{
-	  /* Always treat "E." as an error.  This will be used for
-	     more verbose error messages, such as E.memtypes.  */
-	  if (buf[0] == 'E' && buf[1] == '.')
-	    {
-	      if (buf[2] != '\0')
-		return packet_result::make_textual_error (buf + 2);
-	      else
-		return packet_result::make_textual_error ("no error provided");
-	    }
+	  if (buf[2] != '\0')
+	    return packet_result::make_textual_error (buf + 2);
+	  else
+	    return packet_result::make_textual_error ("no error provided");
 	}
 
       /* The packet may or may not be OK.  Just assume it is.  */
@@ -2544,9 +2552,9 @@ packet_check_result (const char *buf, bool accept_msg)
 }
 
 static packet_result
-packet_check_result (const gdb::char_vector &buf, bool accept_msg)
+packet_check_result (const gdb::char_vector &buf)
 {
-  return packet_check_result (buf.data (), accept_msg);
+  return packet_check_result (buf.data ());
 }
 
 packet_result
@@ -2559,7 +2567,7 @@ remote_features::packet_ok (const char *buf, const int which_packet)
       && config->support == PACKET_DISABLE)
     internal_error (_("packet_ok: attempt to use a disabled packet"));
 
-  packet_result result = packet_check_result (buf, true);
+  packet_result result = packet_check_result (buf);
   switch (result.status ())
     {
     case PACKET_OK:
@@ -5820,6 +5828,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_ENABLE, remote_supported_packet,
+    PACKET_accept_error_message },
 };
 
 static char *remote_support_xml;
@@ -5938,6 +5948,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 ());
 
@@ -8892,7 +8906,7 @@ remote_target::send_g_packet ()
   xsnprintf (rs->buf.data (), get_remote_packet_size (), "g");
   putpkt (rs->buf);
   getpkt (&rs->buf);
-  packet_result result = packet_check_result (rs->buf, true);
+  packet_result result = packet_check_result (rs->buf);
   if (result.status () == PACKET_ERROR)
     error (_("Could not read registers; remote failure reply '%s'"),
 	   result.err_msg ());
@@ -9202,7 +9216,7 @@ remote_target::store_registers_using_G (const struct regcache *regcache)
   bin2hex (regs, p, rsa->sizeof_g_packet);
   putpkt (rs->buf);
   getpkt (&rs->buf);
-  packet_result pkt_status = packet_check_result (rs->buf, true);
+  packet_result pkt_status = packet_check_result (rs->buf);
   if (pkt_status.status () == PACKET_ERROR)
     error (_("Could not write registers; remote failure reply '%s'"),
 	   pkt_status.err_msg ());
@@ -9654,7 +9668,7 @@ 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);
   if (result.status () == PACKET_ERROR)
     return TARGET_XFER_E_IO;
   /* Reply describes memory byte by byte, each byte encoded as two hex
@@ -9809,7 +9823,7 @@ remote_target::remote_send_printf (const char *format, ...)
   rs->buf[0] = '\0';
   getpkt (&rs->buf);
 
-  return packet_check_result (rs->buf, true).status ();
+  return packet_check_result (rs->buf).status ();
 }
 
 /* Flash writing can take quite some time.  We'll set
@@ -12002,7 +12016,7 @@ remote_target::rcmd (const char *command, struct ui_file *outbuf)
 	  remote_console_output (buf + 1, outbuf);
 	  continue;
 	}
-      packet_result result = packet_check_result (buf, false);
+      packet_result result = packet_check_result (buf);
       switch (result.status ())
 	{
 	case PACKET_UNKNOWN:
@@ -15682,7 +15696,7 @@ remote_target::store_memtags (CORE_ADDR address, size_t len,
   getpkt (&rs->buf);
 
   /* Verify if the request was successful.  */
-  return packet_check_result (rs->buf, true).status () == PACKET_OK;
+  return packet_check_result (rs->buf).status () == PACKET_OK;
 }
 
 /* Implement the "is_address_tagged" target_ops method.  */
@@ -15883,29 +15897,26 @@ static void
 test_packet_check_result ()
 {
   std::string buf = "E.msg";
-  packet_result result = packet_check_result (buf.data (), true);
+  packet_result result = packet_check_result (buf.data ());
 
   SELF_CHECK (result.status () == PACKET_ERROR);
   SELF_CHECK (strcmp(result.err_msg (), "msg") == 0);
 
-  result = packet_check_result ("E01", true);
+  result = packet_check_result ("E01");
   SELF_CHECK (result.status () == PACKET_ERROR);
   SELF_CHECK (strcmp(result.err_msg (), "01") == 0);
 
-  SELF_CHECK (packet_check_result ("E1", true).status () == PACKET_OK);
+  SELF_CHECK (packet_check_result ("E1").status () == PACKET_OK);
 
-  SELF_CHECK (packet_check_result ("E000", true).status () == PACKET_OK);
+  SELF_CHECK (packet_check_result ("E000").status () == PACKET_OK);
 
-  result = packet_check_result ("E.", true);
+  result = packet_check_result ("E.");
   SELF_CHECK (result.status () == PACKET_ERROR);
   SELF_CHECK (strcmp(result.err_msg (), "no error provided") == 0);
 
-  SELF_CHECK (packet_check_result ("some response", true).status () == PACKET_OK);
+  SELF_CHECK (packet_check_result ("some response").status () == PACKET_OK);
 
-  SELF_CHECK (packet_check_result ("", true).status () == PACKET_UNKNOWN);
-
-  result = packet_check_result ("E.msg", false);
-  SELF_CHECK (result.status () == PACKET_OK);
+  SELF_CHECK (packet_check_result ("").status () == PACKET_UNKNOWN);
 }
 } // namespace selftests
 #endif /* GDB_SELF_TEST */
@@ -16274,6 +16285,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 6f9d2a8..ab9a33f 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -2699,6 +2699,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.  */
@@ -4364,6 +4366,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 0074818..eabedb9 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.errtext style errors are supported everywhere,
+     including for the qRcmd and m packet.  When false E.errtext 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 ();