Alexandra Hájková <ahajkova@khirnov.net> writes:
> From: Alexandra Hájková <ahajkova@redhat.com>
>
> Check if the gdbserver GDB is communicating with supports
> responding with the error message in an E.errtext format to GDB's
> packets.
>
> 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>
> ---
> v3:
> - Improved documentation.
> - Simplified the handling of the feature, GDB send the feature request to the gdbserver, so gdbserver know
> it can always reply with 'E.errtext', but I dropped sending a response if gdbserver is going to be sending errors in
> such a way. GDB can handle both versions of the errors now and do not need to know if gdbserver supports E.errtext
> response.
>
> gdb/doc/gdb.texinfo | 31 +++++++++++++++++--
> gdb/remote.c | 75 +++++++++++++++++++++++++++------------------
> gdbserver/server.cc | 3 ++
> gdbserver/server.h | 5 +++
> 4 files changed, 82 insertions(+), 32 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 42b446c7e27..d4ddd3b2998 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.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
> };
>
> @@ -2503,7 +2515,7 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
> code). When ACCEPT_MSG is true error messages can also take the
> form E.msg (where msg is any arbitrary string). */
> static packet_result
> -packet_check_result (const char *buf, bool accept_msg)
> +packet_check_result (const char *buf)
Please update the function comment to remove reference to ACCEPT_MSG.
> 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;
We seem to have standardised on 'E.errtext' as the description of this
packet type. Please change 'E.message' to 'E.errtext' in this commit.
I think with these two fixes, this can be merged.
Approved-By: Andrew Burgess <aburgess@redhat.com>
Thanks,
Andrew
@@ -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::
@@ -377,6 +377,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
};
@@ -2503,7 +2515,7 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
code). When ACCEPT_MSG is true error messages can also take the
form E.msg (where msg is any arbitrary string). */
static packet_result
-packet_check_result (const char *buf, bool accept_msg)
+packet_check_result (const char *buf)
{
if (buf[0] != '\0')
{
@@ -2518,17 +2530,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. */
@@ -2542,9 +2551,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
@@ -2557,7 +2566,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:
@@ -5818,6 +5827,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;
@@ -5936,6 +5947,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 ());
@@ -8890,7 +8905,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 ());
@@ -9200,7 +9215,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 ());
@@ -9652,7 +9667,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
@@ -9807,7 +9822,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
@@ -12000,7 +12015,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:
@@ -15672,7 +15687,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. */
@@ -15873,29 +15888,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 */
@@ -16264,6 +16276,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. */
{
@@ -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. */
@@ -4375,6 +4377,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);
@@ -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 ();