* [PATCH v4] remote.c: Make packet_check_result return a structure
@ 2024-01-22 16:39 Alexandra Hájková
2024-01-29 10:01 ` Andrew Burgess
0 siblings, 1 reply; 2+ messages in thread
From: Alexandra Hájková @ 2024-01-22 16:39 UTC (permalink / raw)
To: gdb-patches
packet_check_result currently returns an packet_result enum.
If GDB will recieve an error in a format E.errtext, which
is possible for some q packets, such errtext is lost if
treated by packet_check_result.
Introduce a new structure which bundles enum packet_result
with possible error message or error code returned by
the GDBserver.
I plan to make more GDBserver response checking functions to use
packet_check_result to make remote.c code more consistent.
This will also allow to use E.errtext more in the future.
Beside adding the unit test, I tested this by modifying
store_registers_using_G function to get an error message:
[remote] Sending packet: $GG00000000 ...
gdbserver: Wrong sized register packet (expected 1792 bytes, got 1793)
gdbserver: Invalid hex digit 71
Killing process(es): 1104002
[remote] Packet received: E01
Could not write registers; remote failure reply '01'
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
---
v4: - cosmetics
- simplify the code a bit
- include a test checking packet_status::err_msg
to the unit test
gdb/remote.c | 134 +++++++++++++++++++++++++++++++++++++--------------
1 file changed, 98 insertions(+), 36 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 72f14e28f54..f33d1ba0fa4 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -18,7 +18,6 @@
along with this program. If not, see <http://www.gnu.org/licenses/>. */
/* See the GDB User Guide for details of the GDB remote protocol. */
-
#include "defs.h"
#include <ctype.h>
#include <fcntl.h>
@@ -149,13 +148,46 @@ get_target_type_name (bool target_connected)
/* Analyze a packet's return value and update the packet config
accordingly. */
-enum packet_result
+enum packet_status
{
PACKET_ERROR,
PACKET_OK,
PACKET_UNKNOWN
};
+/* Keeps packet's return value. If packet's return value is PACKET_ERROR,
+ err_msg contains an error message string from E.string or the number
+ stored as a string from E.num. */
+struct packet_result
+{
+ packet_result (enum packet_status status, std::string err_msg)
+ : m_status (status), m_err_msg (std::move (err_msg))
+ {
+ gdb_assert (status == PACKET_ERROR);
+ }
+
+ explicit packet_result (enum packet_status status)
+ : m_status (status)
+ {
+ gdb_assert (status != PACKET_ERROR);
+ }
+
+ enum packet_status status () const
+ {
+ return this->m_status;
+ }
+
+ const char *err_msg () const
+ {
+ gdb_assert (this->m_status == PACKET_ERROR);
+ return this->m_err_msg.c_str ();
+ }
+
+private:
+ enum packet_status m_status;
+ std::string m_err_msg;
+};
+
/* Enumeration of packets for a remote target. */
enum {
@@ -732,8 +764,8 @@ struct remote_features
/* Check result value in BUF for packet WHICH_PACKET and update the packet's
support configuration accordingly. */
- packet_result packet_ok (const char *buf, const int which_packet);
- packet_result packet_ok (const gdb::char_vector &buf, const int which_packet);
+ packet_status packet_ok (const char *buf, const int which_packet);
+ packet_status packet_ok (const gdb::char_vector &buf, const int which_packet);
/* Configuration of a remote target's memory read packet. */
memory_packet_config m_memory_read_packet_config;
@@ -1254,7 +1286,7 @@ class remote_target : public process_stratum_target
int unit_size,
ULONGEST *xfered_len);
- packet_result remote_send_printf (const char *format, ...)
+ packet_status remote_send_printf (const char *format, ...)
ATTRIBUTE_PRINTF (2, 3);
target_xfer_status remote_flash_write (ULONGEST address,
@@ -2418,7 +2450,10 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
}
}
-static enum packet_result
+/* Check GDBserver's reply packet. Return packet_result
+ structure which contains the packet_status enum
+ and an error message for the PACKET_ERROR case. */
+static packet_result
packet_check_result (const char *buf)
{
if (buf[0] != '\0')
@@ -2429,41 +2464,41 @@ packet_check_result (const char *buf)
&& isxdigit (buf[1]) && isxdigit (buf[2])
&& buf[3] == '\0')
/* "Enn" - definitely an error. */
- return PACKET_ERROR;
+ return { PACKET_ERROR, 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] == '.')
- return PACKET_ERROR;
+ return { PACKET_ERROR, buf + 2 };
/* The packet may or may not be OK. Just assume it is. */
- return PACKET_OK;
+ return packet_result ( PACKET_OK );
}
else
+ {
/* The stub does not support the packet. */
- return PACKET_UNKNOWN;
+ return packet_result ( PACKET_UNKNOWN );
+ }
}
-static enum packet_result
+static packet_result
packet_check_result (const gdb::char_vector &buf)
{
return packet_check_result (buf.data ());
}
-packet_result
+packet_status
remote_features::packet_ok (const char *buf, const int which_packet)
{
packet_config *config = &m_protocol_packets[which_packet];
packet_description *descr = &packets_descriptions[which_packet];
- enum packet_result result;
-
if (config->detect != AUTO_BOOLEAN_TRUE
&& config->support == PACKET_DISABLE)
internal_error (_("packet_ok: attempt to use a disabled packet"));
- result = packet_check_result (buf);
- switch (result)
+ packet_result result = packet_check_result (buf);
+ switch (result.status ())
{
case PACKET_OK:
case PACKET_ERROR:
@@ -2498,10 +2533,10 @@ remote_features::packet_ok (const char *buf, const int which_packet)
break;
}
- return result;
+ return result.status ();
}
-packet_result
+packet_status
remote_features::packet_ok (const gdb::char_vector &buf, const int which_packet)
{
return packet_ok (buf.data (), which_packet);
@@ -3000,7 +3035,7 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
gdb::array_view<const int> syscall_counts)
{
const char *catch_packet;
- enum packet_result result;
+ enum packet_status result;
int n_sysno = 0;
if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
@@ -8791,9 +8826,10 @@ remote_target::send_g_packet ()
xsnprintf (rs->buf.data (), get_remote_packet_size (), "g");
putpkt (rs->buf);
getpkt (&rs->buf);
- if (packet_check_result (rs->buf) == PACKET_ERROR)
+ packet_result result = packet_check_result (rs->buf);
+ if (result.status () == PACKET_ERROR)
error (_("Could not read registers; remote failure reply '%s'"),
- rs->buf.data ());
+ result.err_msg ());
/* We can get out of synch in various cases. If the first character
in the buffer is not a hex character, assume that has happened
@@ -9099,9 +9135,10 @@ remote_target::store_registers_using_G (const struct regcache *regcache)
bin2hex (regs, p, rsa->sizeof_g_packet);
putpkt (rs->buf);
getpkt (&rs->buf);
- if (packet_check_result (rs->buf) == PACKET_ERROR)
+ packet_result pkt_status = packet_check_result (rs->buf);
+ if (pkt_status.status () == PACKET_ERROR)
error (_("Could not write registers; remote failure reply '%s'"),
- rs->buf.data ());
+ pkt_status.err_msg ());
}
/* Store register REGNUM, or all registers if REGNUM == -1, from the contents
@@ -9683,7 +9720,7 @@ remote_target::remote_read_bytes (CORE_ADDR memaddr,
FORMAT and the remaining arguments, then gets the reply. Returns
whether the packet was a success, a failure, or unknown. */
-packet_result
+packet_status
remote_target::remote_send_printf (const char *format, ...)
{
struct remote_state *rs = get_remote_state ();
@@ -9706,7 +9743,7 @@ remote_target::remote_send_printf (const char *format, ...)
rs->buf[0] = '\0';
getpkt (&rs->buf);
- return packet_check_result (rs->buf);
+ return packet_check_result (rs->buf).status ();
}
/* Flash writing can take quite some time. We'll set
@@ -9718,7 +9755,7 @@ void
remote_target::flash_erase (ULONGEST address, LONGEST length)
{
int addr_size = gdbarch_addr_bit (current_inferior ()->arch ()) / 8;
- enum packet_result ret;
+ enum packet_status ret;
scoped_restore restore_timeout
= make_scoped_restore (&remote_timeout, remote_flash_timeout);
@@ -11308,7 +11345,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
if (target_has_execution ()
&& m_features.packet_support (PACKET_qCRC) != PACKET_DISABLE)
{
- enum packet_result result;
+ enum packet_status status;
/* Make sure the remote is pointing at the right process. */
set_general_process ();
@@ -11324,10 +11361,10 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
getpkt (&rs->buf);
- result = m_features.packet_ok (rs->buf, PACKET_qCRC);
- if (result == PACKET_ERROR)
+ status = m_features.packet_ok (rs->buf, PACKET_qCRC);
+ if (status == PACKET_ERROR)
return -1;
- else if (result == PACKET_OK)
+ else if (status == PACKET_OK)
{
for (target_crc = 0, tmp = &rs->buf[1]; *tmp; tmp++)
target_crc = target_crc * 16 + fromhex (*tmp);
@@ -12210,7 +12247,7 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
struct remote_state *rs = get_remote_state ();
char *p = rs->buf.data ();
char *endp = p + get_remote_packet_size ();
- enum packet_result result;
+ enum packet_status result;
strcpy (p, "qGetTLSAddr:");
p += strlen (p);
@@ -12256,7 +12293,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
struct remote_state *rs = get_remote_state ();
char *p = rs->buf.data ();
char *endp = p + get_remote_packet_size ();
- enum packet_result result;
+ enum packet_status result;
strcpy (p, "qGetTIBAddr:");
p += strlen (p);
@@ -13821,7 +13858,7 @@ remote_target::get_trace_status (struct trace_status *ts)
{
/* Initialize it just to avoid a GCC false warning. */
char *p = NULL;
- enum packet_result result;
+ enum packet_status result;
struct remote_state *rs = get_remote_state ();
if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
@@ -14201,7 +14238,7 @@ remote_target::set_trace_buffer_size (LONGEST val)
struct remote_state *rs = get_remote_state ();
char *buf = rs->buf.data ();
char *endbuf = buf + get_remote_packet_size ();
- enum packet_result result;
+ enum packet_status result;
gdb_assert (val >= 0 || val == -1);
buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
@@ -15529,7 +15566,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.data ()) == PACKET_OK;
+ return packet_check_result (rs->buf).status () == PACKET_OK;
}
/* Return true if remote target T is non-stop. */
@@ -15626,8 +15663,31 @@ test_memory_tagging_functions ()
expected.length ()) == 0);
}
+static void
+test_packet_check_result ()
+{
+ std::string buf = "E.msg";
+ packet_result result = packet_check_result (buf.data ());
+
+ SELF_CHECK (result.status () == PACKET_ERROR);
+ SELF_CHECK (strcmp(result.err_msg (), "msg") == 0);
+
+ SELF_CHECK (packet_check_result ("E01").status () == PACKET_ERROR);
+
+ SELF_CHECK (packet_check_result ("E1").status () == PACKET_OK);
+
+ SELF_CHECK (packet_check_result ("E000").status () == PACKET_OK);
+
+ SELF_CHECK (packet_check_result ("E.").status () == PACKET_ERROR);
+
+ SELF_CHECK (packet_check_result ("some response").status () == PACKET_OK);
+
+ SELF_CHECK (packet_check_result ("").status () == PACKET_UNKNOWN);
+
+}
+
} // namespace selftests
-#endif /* GDB_SELF_TEST */
+#endif /* GDB_SELTEST */
void _initialize_remote ();
void
@@ -16125,5 +16185,7 @@ from the target."),
#if GDB_SELF_TEST
selftests::register_test ("remote_memory_tagging",
selftests::test_memory_tagging_functions);
+ selftests::register_test ("packet_check_result",
+ selftests::test_packet_check_result);
#endif
}
--
2.43.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v4] remote.c: Make packet_check_result return a structure
2024-01-22 16:39 [PATCH v4] remote.c: Make packet_check_result return a structure Alexandra Hájková
@ 2024-01-29 10:01 ` Andrew Burgess
0 siblings, 0 replies; 2+ messages in thread
From: Andrew Burgess @ 2024-01-29 10:01 UTC (permalink / raw)
To: Alexandra Hájková, gdb-patches
Alexandra Hájková <ahajkova@redhat.com> writes:
> packet_check_result currently returns an packet_result enum.
> If GDB will recieve an error in a format E.errtext, which
> is possible for some q packets, such errtext is lost if
> treated by packet_check_result.
> Introduce a new structure which bundles enum packet_result
> with possible error message or error code returned by
> the GDBserver.
> I plan to make more GDBserver response checking functions to use
> packet_check_result to make remote.c code more consistent.
> This will also allow to use E.errtext more in the future.
>
> Beside adding the unit test, I tested this by modifying
> store_registers_using_G function to get an error message:
>
> [remote] Sending packet: $GG00000000 ...
>
> gdbserver: Wrong sized register packet (expected 1792 bytes, got 1793)
> gdbserver: Invalid hex digit 71
> Killing process(es): 1104002
> [remote] Packet received: E01
> Could not write registers; remote failure reply '01'
>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
> v4: - cosmetics
> - simplify the code a bit
> - include a test checking packet_status::err_msg
> to the unit test
>
> gdb/remote.c | 134 +++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 98 insertions(+), 36 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 72f14e28f54..f33d1ba0fa4 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -18,7 +18,6 @@
> along with this program. If not, see <http://www.gnu.org/licenses/>. */
>
> /* See the GDB User Guide for details of the GDB remote protocol. */
> -
Spurious line deletion here.
> #include "defs.h"
> #include <ctype.h>
> #include <fcntl.h>
> @@ -149,13 +148,46 @@ get_target_type_name (bool target_connected)
> /* Analyze a packet's return value and update the packet config
> accordingly. */
>
> -enum packet_result
> +enum packet_status
> {
> PACKET_ERROR,
> PACKET_OK,
> PACKET_UNKNOWN
> };
>
> +/* Keeps packet's return value. If packet's return value is PACKET_ERROR,
> + err_msg contains an error message string from E.string or the number
> + stored as a string from E.num. */
> +struct packet_result
> +{
> + packet_result (enum packet_status status, std::string err_msg)
> + : m_status (status), m_err_msg (std::move (err_msg))
> + {
> + gdb_assert (status == PACKET_ERROR);
> + }
> +
> + explicit packet_result (enum packet_status status)
> + : m_status (status)
> + {
> + gdb_assert (status != PACKET_ERROR);
> + }
> +
> + enum packet_status status () const
> + {
> + return this->m_status;
> + }
> +
> + const char *err_msg () const
> + {
> + gdb_assert (this->m_status == PACKET_ERROR);
> + return this->m_err_msg.c_str ();
> + }
> +
> +private:
> + enum packet_status m_status;
> + std::string m_err_msg;
> +};
> +
> /* Enumeration of packets for a remote target. */
>
> enum {
> @@ -732,8 +764,8 @@ struct remote_features
>
> /* Check result value in BUF for packet WHICH_PACKET and update the packet's
> support configuration accordingly. */
> - packet_result packet_ok (const char *buf, const int which_packet);
> - packet_result packet_ok (const gdb::char_vector &buf, const int which_packet);
> + packet_status packet_ok (const char *buf, const int which_packet);
> + packet_status packet_ok (const gdb::char_vector &buf, const int which_packet);
>
> /* Configuration of a remote target's memory read packet. */
> memory_packet_config m_memory_read_packet_config;
> @@ -1254,7 +1286,7 @@ class remote_target : public process_stratum_target
> int unit_size,
> ULONGEST *xfered_len);
>
> - packet_result remote_send_printf (const char *format, ...)
> + packet_status remote_send_printf (const char *format, ...)
> ATTRIBUTE_PRINTF (2, 3);
>
> target_xfer_status remote_flash_write (ULONGEST address,
> @@ -2418,7 +2450,10 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
> }
> }
>
> -static enum packet_result
> +/* Check GDBserver's reply packet. Return packet_result
> + structure which contains the packet_status enum
> + and an error message for the PACKET_ERROR case. */
> +static packet_result
> packet_check_result (const char *buf)
> {
> if (buf[0] != '\0')
> @@ -2429,41 +2464,41 @@ packet_check_result (const char *buf)
> && isxdigit (buf[1]) && isxdigit (buf[2])
> && buf[3] == '\0')
> /* "Enn" - definitely an error. */
> - return PACKET_ERROR;
> + return { PACKET_ERROR, 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] == '.')
> - return PACKET_ERROR;
> + return { PACKET_ERROR, buf + 2 };
>
> /* The packet may or may not be OK. Just assume it is. */
> - return PACKET_OK;
> + return packet_result ( PACKET_OK );
Unnecessary whitepsace around PACKET_OK, should be:
return packet_result (PACKET_OK);
> }
> else
> + {
> /* The stub does not support the packet. */
> - return PACKET_UNKNOWN;
> + return packet_result ( PACKET_UNKNOWN );
Same here, but PACKET_UNKNOWN.
> + }
> }
>
> -static enum packet_result
> +static packet_result
> packet_check_result (const gdb::char_vector &buf)
> {
> return packet_check_result (buf.data ());
> }
>
> -packet_result
> +packet_status
> remote_features::packet_ok (const char *buf, const int which_packet)
> {
> packet_config *config = &m_protocol_packets[which_packet];
> packet_description *descr = &packets_descriptions[which_packet];
>
> - enum packet_result result;
> -
> if (config->detect != AUTO_BOOLEAN_TRUE
> && config->support == PACKET_DISABLE)
> internal_error (_("packet_ok: attempt to use a disabled packet"));
>
> - result = packet_check_result (buf);
> - switch (result)
> + packet_result result = packet_check_result (buf);
> + switch (result.status ())
> {
> case PACKET_OK:
> case PACKET_ERROR:
> @@ -2498,10 +2533,10 @@ remote_features::packet_ok (const char *buf, const int which_packet)
> break;
> }
>
> - return result;
> + return result.status ();
> }
>
> -packet_result
> +packet_status
> remote_features::packet_ok (const gdb::char_vector &buf, const int which_packet)
> {
> return packet_ok (buf.data (), which_packet);
> @@ -3000,7 +3035,7 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
> gdb::array_view<const int> syscall_counts)
> {
> const char *catch_packet;
> - enum packet_result result;
> + enum packet_status result;
> int n_sysno = 0;
>
> if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
> @@ -8791,9 +8826,10 @@ remote_target::send_g_packet ()
> xsnprintf (rs->buf.data (), get_remote_packet_size (), "g");
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - if (packet_check_result (rs->buf) == PACKET_ERROR)
> + packet_result result = packet_check_result (rs->buf);
> + if (result.status () == PACKET_ERROR)
> error (_("Could not read registers; remote failure reply '%s'"),
> - rs->buf.data ());
> + result.err_msg ());
>
> /* We can get out of synch in various cases. If the first character
> in the buffer is not a hex character, assume that has happened
> @@ -9099,9 +9135,10 @@ remote_target::store_registers_using_G (const struct regcache *regcache)
> bin2hex (regs, p, rsa->sizeof_g_packet);
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - if (packet_check_result (rs->buf) == PACKET_ERROR)
> + packet_result pkt_status = packet_check_result (rs->buf);
> + if (pkt_status.status () == PACKET_ERROR)
> error (_("Could not write registers; remote failure reply '%s'"),
> - rs->buf.data ());
> + pkt_status.err_msg ());
> }
>
> /* Store register REGNUM, or all registers if REGNUM == -1, from the contents
> @@ -9683,7 +9720,7 @@ remote_target::remote_read_bytes (CORE_ADDR memaddr,
> FORMAT and the remaining arguments, then gets the reply. Returns
> whether the packet was a success, a failure, or unknown. */
>
> -packet_result
> +packet_status
> remote_target::remote_send_printf (const char *format, ...)
> {
> struct remote_state *rs = get_remote_state ();
> @@ -9706,7 +9743,7 @@ remote_target::remote_send_printf (const char *format, ...)
> rs->buf[0] = '\0';
> getpkt (&rs->buf);
>
> - return packet_check_result (rs->buf);
> + return packet_check_result (rs->buf).status ();
> }
>
> /* Flash writing can take quite some time. We'll set
> @@ -9718,7 +9755,7 @@ void
> remote_target::flash_erase (ULONGEST address, LONGEST length)
> {
> int addr_size = gdbarch_addr_bit (current_inferior ()->arch ()) / 8;
> - enum packet_result ret;
> + enum packet_status ret;
> scoped_restore restore_timeout
> = make_scoped_restore (&remote_timeout, remote_flash_timeout);
>
> @@ -11308,7 +11345,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
> if (target_has_execution ()
> && m_features.packet_support (PACKET_qCRC) != PACKET_DISABLE)
> {
> - enum packet_result result;
> + enum packet_status status;
>
> /* Make sure the remote is pointing at the right process. */
> set_general_process ();
> @@ -11324,10 +11361,10 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
>
> getpkt (&rs->buf);
>
> - result = m_features.packet_ok (rs->buf, PACKET_qCRC);
> - if (result == PACKET_ERROR)
> + status = m_features.packet_ok (rs->buf, PACKET_qCRC);
> + if (status == PACKET_ERROR)
> return -1;
> - else if (result == PACKET_OK)
> + else if (status == PACKET_OK)
> {
> for (target_crc = 0, tmp = &rs->buf[1]; *tmp; tmp++)
> target_crc = target_crc * 16 + fromhex (*tmp);
> @@ -12210,7 +12247,7 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
> struct remote_state *rs = get_remote_state ();
> char *p = rs->buf.data ();
> char *endp = p + get_remote_packet_size ();
> - enum packet_result result;
> + enum packet_status result;
>
> strcpy (p, "qGetTLSAddr:");
> p += strlen (p);
> @@ -12256,7 +12293,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
> struct remote_state *rs = get_remote_state ();
> char *p = rs->buf.data ();
> char *endp = p + get_remote_packet_size ();
> - enum packet_result result;
> + enum packet_status result;
>
> strcpy (p, "qGetTIBAddr:");
> p += strlen (p);
> @@ -13821,7 +13858,7 @@ remote_target::get_trace_status (struct trace_status *ts)
> {
> /* Initialize it just to avoid a GCC false warning. */
> char *p = NULL;
> - enum packet_result result;
> + enum packet_status result;
> struct remote_state *rs = get_remote_state ();
>
> if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
> @@ -14201,7 +14238,7 @@ remote_target::set_trace_buffer_size (LONGEST val)
> struct remote_state *rs = get_remote_state ();
> char *buf = rs->buf.data ();
> char *endbuf = buf + get_remote_packet_size ();
> - enum packet_result result;
> + enum packet_status result;
>
> gdb_assert (val >= 0 || val == -1);
> buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> @@ -15529,7 +15566,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.data ()) == PACKET_OK;
> + return packet_check_result (rs->buf).status () == PACKET_OK;
> }
>
> /* Return true if remote target T is non-stop. */
> @@ -15626,8 +15663,31 @@ test_memory_tagging_functions ()
> expected.length ()) == 0);
> }
>
> +static void
> +test_packet_check_result ()
> +{
> + std::string buf = "E.msg";
> + packet_result result = packet_check_result (buf.data ());
> +
> + SELF_CHECK (result.status () == PACKET_ERROR);
> + SELF_CHECK (strcmp(result.err_msg (), "msg") == 0);
> +
> + SELF_CHECK (packet_check_result ("E01").status () == PACKET_ERROR);
I think you should rewrite this check in the same way that you check
'E.msg', this will allow you to check both the status and the err_msg.
> +
> + SELF_CHECK (packet_check_result ("E1").status () == PACKET_OK);
> +
> + SELF_CHECK (packet_check_result ("E000").status () == PACKET_OK);
> +
> + SELF_CHECK (packet_check_result ("E.").status () == PACKET_ERROR);
Again, you should check the err_msg content here, though I think this
will highlight an issue: should we ever get this back from the remote
then the err_msg text will be empty, which isn't going to look good in
the output.
I think the choices here are (a) define that 'E.' is NOT an error packet
(same as E1 or E000 is not an error packet), in which case 'E.' should
return PACKET_OK, or (b) provide some default text, e.g. 'no error
provided'.
Given how we treat E1 and E000, I'd be tempted to go with option (a)
myself.
> +
> + SELF_CHECK (packet_check_result ("some response").status () == PACKET_OK);
> +
> + SELF_CHECK (packet_check_result ("").status () == PACKET_UNKNOWN);
> +
> +}
> +
> } // namespace selftests
> -#endif /* GDB_SELF_TEST */
> +#endif /* GDB_SELTEST */
Spurious change here.
Thanks,
Andrew
>
> void _initialize_remote ();
> void
> @@ -16125,5 +16185,7 @@ from the target."),
> #if GDB_SELF_TEST
> selftests::register_test ("remote_memory_tagging",
> selftests::test_memory_tagging_functions);
> + selftests::register_test ("packet_check_result",
> + selftests::test_packet_check_result);
> #endif
> }
> --
> 2.43.0
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-01-29 10:01 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-22 16:39 [PATCH v4] remote.c: Make packet_check_result return a structure Alexandra Hájková
2024-01-29 10:01 ` Andrew Burgess
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).