* [PATCH 1/2] remote.c: Use packet_check_result
@ 2024-02-12 14:40 Alexandra Hájková
2024-02-12 14:40 ` [PATCH 2/2] remote.c: Make packet_ok return struct packet_result Alexandra Hájková
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Alexandra Hájková @ 2024-02-12 14:40 UTC (permalink / raw)
To: gdb-patches
when processing the GDBserver reply to qRcmd packet.
Print error message or the error code.
Currently, when qRcmd request returns an error,
GDB just prints:
Protocol error with Rcmd
After this change, GDB will also print the error code:
Protocol error with Rcmd: 01.
Add an accept_msg argument to packet_check result. qRcmd
request (such as many other packets) does not recognise
"E.msg" form as an error right now. We want to recognise
"E.msg" as an error response only for the packets where
it's documented.
---
gdb/remote.c | 65 ++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 30 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 14c8b020b1e..8caee0dcff9 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2455,7 +2455,7 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
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)
+packet_check_result (const char *buf, bool accept_msg)
{
if (buf[0] != '\0')
{
@@ -2467,14 +2467,20 @@ packet_check_result (const char *buf)
/* "Enn" - definitely an 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] == '.')
+ /* 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)
{
- if (buf[2] != '\0')
- return { PACKET_ERROR, buf + 2 };
- else
- return { PACKET_ERROR, "no error provided" };
+ /* 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_ERROR, buf + 2 };
+ else
+ return { PACKET_ERROR, "no error provided" };
+ }
}
/* The packet may or may not be OK. Just assume it is. */
@@ -2488,9 +2494,9 @@ packet_check_result (const char *buf)
}
static packet_result
-packet_check_result (const gdb::char_vector &buf)
+packet_check_result (const gdb::char_vector &buf, bool accept_msg)
{
- return packet_check_result (buf.data ());
+ return packet_check_result (buf.data (), accept_msg);
}
packet_status
@@ -2503,7 +2509,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);
+ packet_result result = packet_check_result (buf, true);
switch (result.status ())
{
case PACKET_OK:
@@ -8831,7 +8837,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);
+ packet_result result = packet_check_result (rs->buf, true);
if (result.status () == PACKET_ERROR)
error (_("Could not read registers; remote failure reply '%s'"),
result.err_msg ());
@@ -9140,7 +9146,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);
+ packet_result pkt_status = packet_check_result (rs->buf, true);
if (pkt_status.status () == PACKET_ERROR)
error (_("Could not write registers; remote failure reply '%s'"),
pkt_status.err_msg ());
@@ -9748,7 +9754,7 @@ remote_target::remote_send_printf (const char *format, ...)
rs->buf[0] = '\0';
getpkt (&rs->buf);
- return packet_check_result (rs->buf).status ();
+ return packet_check_result (rs->buf, true).status ();
}
/* Flash writing can take quite some time. We'll set
@@ -11931,20 +11937,19 @@ remote_target::rcmd (const char *command, struct ui_file *outbuf)
continue;
}
buf = rs->buf.data ();
- if (buf[0] == '\0')
- error (_("Target does not support this command."));
if (buf[0] == 'O' && buf[1] != 'K')
{
remote_console_output (buf + 1); /* 'O' message from stub. */
continue;
}
- if (strcmp (buf, "OK") == 0)
+ packet_result result = packet_check_result (buf, true);
+ if (result.status () == PACKET_OK)
break;
- if (strlen (buf) == 3 && buf[0] == 'E'
- && isxdigit (buf[1]) && isxdigit (buf[2]))
- {
- error (_("Protocol error with Rcmd"));
- }
+ else if (result.status () == PACKET_UNKNOWN)
+ error (_("Target does not support this command."));
+ else
+ error (_("Protocol error with Rcmd: %s."), result.err_msg ());
+
for (p = buf; p[0] != '\0' && p[1] != '\0'; p += 2)
{
char c = (fromhex (p[0]) << 4) + fromhex (p[1]);
@@ -15571,7 +15576,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).status () == PACKET_OK;
+ return (packet_check_result (rs->buf, true).status () == PACKET_OK);
}
/* Return true if remote target T is non-stop. */
@@ -15672,26 +15677,26 @@ static void
test_packet_check_result ()
{
std::string buf = "E.msg";
- packet_result result = packet_check_result (buf.data ());
+ packet_result result = packet_check_result (buf.data (), true);
SELF_CHECK (result.status () == PACKET_ERROR);
SELF_CHECK (strcmp(result.err_msg (), "msg") == 0);
- result = packet_check_result ("E01");
+ result = packet_check_result ("E01", true);
SELF_CHECK (result.status () == PACKET_ERROR);
SELF_CHECK (strcmp(result.err_msg (), "01") == 0);
- SELF_CHECK (packet_check_result ("E1").status () == PACKET_OK);
+ SELF_CHECK (packet_check_result ("E1", true).status () == PACKET_OK);
- SELF_CHECK (packet_check_result ("E000").status () == PACKET_OK);
+ SELF_CHECK (packet_check_result ("E000", true).status () == PACKET_OK);
- result = packet_check_result ("E.");
+ result = packet_check_result ("E.", true);
SELF_CHECK (result.status () == PACKET_ERROR);
SELF_CHECK (strcmp(result.err_msg (), "no error provided") == 0);
- SELF_CHECK (packet_check_result ("some response").status () == PACKET_OK);
+ SELF_CHECK (packet_check_result ("some response", true).status () == PACKET_OK);
- SELF_CHECK (packet_check_result ("").status () == PACKET_UNKNOWN);
+ SELF_CHECK (packet_check_result ("", true).status () == PACKET_UNKNOWN);
}
} // namespace selftests
#endif /* GDB_SELF_TEST */
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] remote.c: Make packet_ok return struct packet_result
2024-02-12 14:40 [PATCH 1/2] remote.c: Use packet_check_result Alexandra Hájková
@ 2024-02-12 14:40 ` Alexandra Hájková
2024-02-26 13:16 ` Alexandra Petlanova Hajkova
2024-03-06 16:13 ` Andrew Burgess
2024-02-26 13:15 ` [PATCH 1/2] remote.c: Use packet_check_result Alexandra Petlanova Hajkova
2024-03-06 15:04 ` Andrew Burgess
2 siblings, 2 replies; 6+ messages in thread
From: Alexandra Hájková @ 2024-02-12 14:40 UTC (permalink / raw)
To: gdb-patches
This allows to print the error message stored in a packet_result
to be easily used in the calling function.
---
gdb/remote.c | 166 +++++++++++++++++++++++----------------------------
1 file changed, 76 insertions(+), 90 deletions(-)
diff --git a/gdb/remote.c b/gdb/remote.c
index 8caee0dcff9..85f5624f2b6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -765,8 +765,8 @@ struct remote_features
/* Check result value in BUF for packet WHICH_PACKET and update the packet's
support configuration accordingly. */
- packet_status packet_ok (const char *buf, const int which_packet);
- packet_status packet_ok (const gdb::char_vector &buf, const int which_packet);
+ packet_result packet_ok (const char *buf, const int which_packet);
+ packet_result 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;
@@ -2499,7 +2499,7 @@ packet_check_result (const gdb::char_vector &buf, bool accept_msg)
return packet_check_result (buf.data (), accept_msg);
}
-packet_status
+packet_result
remote_features::packet_ok (const char *buf, const int which_packet)
{
packet_config *config = &m_protocol_packets[which_packet];
@@ -2545,10 +2545,10 @@ remote_features::packet_ok (const char *buf, const int which_packet)
break;
}
- return result.status ();
+ return result;
}
-packet_status
+packet_result
remote_features::packet_ok (const gdb::char_vector &buf, const int which_packet)
{
return packet_ok (buf.data (), which_packet);
@@ -2735,14 +2735,15 @@ remote_target::remote_query_attached (int pid)
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_qAttached))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_qAttached);
+ switch (result.status())
{
case PACKET_OK:
if (strcmp (rs->buf.data (), "1") == 0)
return 1;
break;
case PACKET_ERROR:
- warning (_("Remote failure reply: %s"), rs->buf.data ());
+ warning (_("Remote failure reply: %s"), result.err_msg());
break;
case PACKET_UNKNOWN:
break;
@@ -3047,7 +3048,6 @@ 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_status result;
int n_sysno = 0;
if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
@@ -3103,8 +3103,8 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
putpkt (catch_packet);
getpkt (&rs->buf);
- result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
- if (result == PACKET_OK)
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
+ if (result.status() == PACKET_OK)
return 0;
else
return -1;
@@ -5109,7 +5109,8 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
{
putpkt ("QStartNoAckMode");
getpkt (&rs->buf);
- if (m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode) == PACKET_OK)
+ if ((m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode)).status ()
+ == PACKET_OK)
rs->noack_mode = 1;
}
@@ -5894,9 +5895,10 @@ remote_target::remote_query_supported ()
/* If an error occurred, warn, but do not return - just reset the
buffer to empty and go on to disable features. */
- if (m_features.packet_ok (rs->buf, PACKET_qSupported) == PACKET_ERROR)
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_qSupported);
+ if (result.status () == PACKET_ERROR)
{
- warning (_("Remote failure reply: %s"), rs->buf.data ());
+ warning (_("Remote failure reply: %s"), result.err_msg ());
rs->buf[0] = 0;
}
}
@@ -6548,7 +6550,8 @@ extended_remote_target::attach (const char *args, int from_tty)
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_vAttach))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_vAttach);
+ switch (result.status ())
{
case PACKET_OK:
if (!target_is_non_stop_p ())
@@ -6560,7 +6563,7 @@ extended_remote_target::attach (const char *args, int from_tty)
else if (strcmp (rs->buf.data (), "OK") != 0)
error (_("Attaching to %s failed with: %s"),
target_pid_to_str (ptid_t (pid)).c_str (),
- rs->buf.data ());
+ result.err_msg ());
break;
case PACKET_UNKNOWN:
error (_("This target does not support attaching to a process"));
@@ -7489,14 +7492,15 @@ remote_target::remote_interrupt_ns ()
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_vCtrlC))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_vCtrlC);
+ switch (result.status ())
{
case PACKET_OK:
break;
case PACKET_UNKNOWN:
error (_("No support for interrupting the remote target."));
case PACKET_ERROR:
- error (_("Interrupting target failed: %s"), rs->buf.data ());
+ error (_("Interrupting target failed: %s"), result.err_msg ());
}
}
@@ -8792,7 +8796,8 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
buf = rs->buf.data ();
- switch (m_features.packet_ok (rs->buf, PACKET_p))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_p);
+ switch (result.status ())
{
case PACKET_OK:
break;
@@ -8801,7 +8806,7 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
case PACKET_ERROR:
error (_("Could not fetch register \"%s\"; remote failure reply '%s'"),
gdbarch_register_name (regcache->arch (), reg->regnum),
- buf);
+ result.err_msg ());
}
/* If this register is unfetchable, tell the regcache. */
@@ -9098,13 +9103,14 @@ remote_target::store_register_using_P (const struct regcache *regcache,
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_P))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_P);
+ switch (result.status ())
{
case PACKET_OK:
return 1;
case PACKET_ERROR:
error (_("Could not write register \"%s\"; remote failure reply '%s'"),
- gdbarch_register_name (gdbarch, reg->regnum), rs->buf.data ());
+ gdbarch_register_name (gdbarch, reg->regnum), result.err_msg ());
case PACKET_UNKNOWN:
return 0;
default:
@@ -10535,7 +10541,7 @@ remote_target::remote_vkill (int pid)
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_vKill))
+ switch ((m_features.packet_ok (rs->buf, PACKET_vKill)).status ())
{
case PACKET_OK:
return 0;
@@ -10691,7 +10697,7 @@ remote_target::extended_remote_run (const std::string &args)
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_vRun))
+ switch ((m_features.packet_ok (rs->buf, PACKET_vRun)).status ())
{
case PACKET_OK:
/* We have a wait response. All is well. */
@@ -10798,11 +10804,12 @@ remote_target::extended_remote_set_inferior_cwd ()
putpkt (rs->buf);
getpkt (&rs->buf);
- if (m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir) != PACKET_OK)
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir);
+ if (result.status () != PACKET_OK)
error (_("\
Remote replied unexpectedly while setting the inferior's working\n\
directory: %s"),
- rs->buf.data ());
+ result.err_msg ());
}
}
@@ -10971,7 +10978,7 @@ remote_target::insert_breakpoint (struct gdbarch *gdbarch,
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_Z0))
+ switch ((m_features.packet_ok (rs->buf, PACKET_Z0)).status ())
{
case PACKET_ERROR:
return -1;
@@ -11072,8 +11079,8 @@ remote_target::insert_watchpoint (CORE_ADDR addr, int len,
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
- + to_underlying (packet))))
+ switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
+ + to_underlying (packet)))).status ())
{
case PACKET_ERROR:
return -1;
@@ -11121,8 +11128,8 @@ remote_target::remove_watchpoint (CORE_ADDR addr, int len,
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
- + to_underlying (packet))))
+ switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
+ + to_underlying (packet)))).status ())
{
case PACKET_ERROR:
case PACKET_UNKNOWN:
@@ -11284,7 +11291,7 @@ remote_target::insert_hw_breakpoint (struct gdbarch *gdbarch,
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_Z1))
+ switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
{
case PACKET_ERROR:
if (rs->buf[1] == '.')
@@ -11331,7 +11338,7 @@ remote_target::remove_hw_breakpoint (struct gdbarch *gdbarch,
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_Z1))
+ switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
{
case PACKET_ERROR:
case PACKET_UNKNOWN:
@@ -11372,7 +11379,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
getpkt (&rs->buf);
- status = m_features.packet_ok (rs->buf, PACKET_qCRC);
+ status = (m_features.packet_ok (rs->buf, PACKET_qCRC)).status ();
if (status == PACKET_ERROR)
return -1;
else if (status == PACKET_OK)
@@ -11494,7 +11501,7 @@ remote_target::remote_write_qxfer (const char *object_name,
if (putpkt_binary (rs->buf.data (), i + buf_len) < 0
|| getpkt (&rs->buf) < 0
- || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
+ || (m_features.packet_ok (rs->buf, which_packet)).status () != PACKET_OK)
return TARGET_XFER_E_IO;
unpack_varlen_hex (rs->buf.data (), &n);
@@ -11559,7 +11566,7 @@ remote_target::remote_read_qxfer (const char *object_name,
rs->buf[0] = '\0';
packet_len = getpkt (&rs->buf);
if (packet_len < 0
- || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
+ || (m_features.packet_ok (rs->buf, which_packet)).status () != PACKET_OK)
return TARGET_XFER_E_IO;
if (rs->buf[0] != 'l' && rs->buf[0] != 'm')
@@ -11864,7 +11871,8 @@ remote_target::search_memory (CORE_ADDR start_addr, ULONGEST search_space_len,
if (putpkt_binary (rs->buf.data (), i + escaped_pattern_len) < 0
|| getpkt (&rs->buf) < 0
- || m_features.packet_ok (rs->buf, PACKET_qSearch_memory) != PACKET_OK)
+ || (m_features.packet_ok (rs->buf, PACKET_qSearch_memory)).status ()
+ != PACKET_OK)
{
/* The request may not have worked because the command is not
supported. If so, fall back to the simple way. */
@@ -12257,7 +12265,6 @@ 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_status result;
strcpy (p, "qGetTLSAddr:");
p += strlen (p);
@@ -12270,15 +12277,15 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
putpkt (rs->buf);
getpkt (&rs->buf);
- result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
- if (result == PACKET_OK)
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
+ if (result.status () == PACKET_OK)
{
ULONGEST addr;
unpack_varlen_hex (rs->buf.data (), &addr);
return addr;
}
- else if (result == PACKET_UNKNOWN)
+ else if (result.status () == PACKET_UNKNOWN)
throw_error (TLS_GENERIC_ERROR,
_("Remote target doesn't support qGetTLSAddr packet"));
else
@@ -12303,7 +12310,6 @@ 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_status result;
strcpy (p, "qGetTIBAddr:");
p += strlen (p);
@@ -12312,8 +12318,8 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
putpkt (rs->buf);
getpkt (&rs->buf);
- result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
- if (result == PACKET_OK)
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
+ if (result.status () == PACKET_OK)
{
ULONGEST val;
unpack_varlen_hex (rs->buf.data (), &val);
@@ -12321,7 +12327,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
*addr = (CORE_ADDR) val;
return true;
}
- else if (result == PACKET_UNKNOWN)
+ else if (result.status () == PACKET_UNKNOWN)
error (_("Remote target doesn't support qGetTIBAddr packet"));
else
error (_("Remote target failed to process qGetTIBAddr request"));
@@ -12580,7 +12586,7 @@ remote_target::remote_hostio_send_command (int command_bytes, int which_packet,
return -1;
}
- switch (m_features.packet_ok (rs->buf, which_packet))
+ switch ((m_features.packet_ok (rs->buf, which_packet)).status ())
{
case PACKET_ERROR:
*remote_errno = FILEIO_EINVAL;
@@ -13868,7 +13874,6 @@ remote_target::get_trace_status (struct trace_status *ts)
{
/* Initialize it just to avoid a GCC false warning. */
char *p = NULL;
- enum packet_status result;
struct remote_state *rs = get_remote_state ();
if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
@@ -13894,17 +13899,17 @@ remote_target::get_trace_status (struct trace_status *ts)
throw;
}
- result = m_features.packet_ok (p, PACKET_qTStatus);
+ packet_result result = m_features.packet_ok (p, PACKET_qTStatus);
/* If the remote target doesn't do tracing, flag it. */
- if (result == PACKET_UNKNOWN)
+ if (result.status () == PACKET_UNKNOWN)
return -1;
/* We're working with a live target. */
ts->filename = NULL;
if (*p++ != 'T')
- error (_("Bogus trace status reply from target: %s"), rs->buf.data ());
+ error (_("Bogus trace status reply from target: %s"), result.err_msg ());
/* Function 'parse_trace_status' sets default value of each field of
'ts' at first, so we don't have to do it here. */
@@ -14248,7 +14253,6 @@ 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_status result;
gdb_assert (val >= 0 || val == -1);
buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
@@ -14263,10 +14267,10 @@ remote_target::set_trace_buffer_size (LONGEST val)
putpkt (rs->buf);
remote_get_noisy_reply ();
- result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
- if (result != PACKET_OK)
- warning (_("Bogus reply from target: %s"), rs->buf.data ());
+ if (result.status () != PACKET_OK)
+ warning (_("Bogus reply from target: %s"), result.err_msg ());
}
}
@@ -14692,14 +14696,9 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
putpkt (buf);
getpkt (&rs->buf);
- if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_bts_size)
- == PACKET_ERROR)
- {
- if (buf[0] == 'E' && buf[1] == '.')
- error (_("Failed to configure the BTS buffer size: %s"), buf + 2);
- else
- error (_("Failed to configure the BTS buffer size."));
- }
+ packet_result result = m_features.packet_ok (buf, PACKET_Qbtrace_conf_bts_size);
+ if (result.status () == PACKET_ERROR)
+ error (_("Failed to configure the BTS buffer size: %s"), result.err_msg ());
rs->btrace_config.bts.size = conf->bts.size;
}
@@ -14715,14 +14714,9 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
putpkt (buf);
getpkt (&rs->buf);
- if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_pt_size)
- == PACKET_ERROR)
- {
- if (buf[0] == 'E' && buf[1] == '.')
- error (_("Failed to configure the trace buffer size: %s"), buf + 2);
- else
- error (_("Failed to configure the trace buffer size."));
- }
+ packet_result result = m_features.packet_ok (buf, PACKET_Qbtrace_conf_pt_size);
+ if (result.status () == PACKET_ERROR)
+ error (_("Failed to configure the trace buffer size: %s"), result.err_msg ());
rs->btrace_config.pt.size = conf->pt.size;
}
@@ -14837,15 +14831,10 @@ remote_target::enable_btrace (thread_info *tp,
putpkt (rs->buf);
getpkt (&rs->buf);
- if (m_features.packet_ok (rs->buf, which_packet) == PACKET_ERROR)
- {
- if (rs->buf[0] == 'E' && rs->buf[1] == '.')
- error (_("Could not enable branch tracing for %s: %s"),
- target_pid_to_str (ptid).c_str (), &rs->buf[2]);
- else
- error (_("Could not enable branch tracing for %s."),
- target_pid_to_str (ptid).c_str ());
- }
+ packet_result result = m_features.packet_ok (rs->buf, which_packet);
+ if (result.status () == PACKET_ERROR)
+ error (_("Could not enable branch tracing for %s: %s"),
+ target_pid_to_str (ptid).c_str (), result.err_msg ());
btrace_target_info *tinfo = new btrace_target_info { ptid };
@@ -14883,15 +14872,10 @@ remote_target::disable_btrace (struct btrace_target_info *tinfo)
putpkt (rs->buf);
getpkt (&rs->buf);
- if (m_features.packet_ok (rs->buf, PACKET_Qbtrace_off) == PACKET_ERROR)
- {
- if (rs->buf[0] == 'E' && rs->buf[1] == '.')
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_Qbtrace_off);
+ if (result.status () == PACKET_ERROR)
error (_("Could not disable branch tracing for %s: %s"),
- target_pid_to_str (tinfo->ptid).c_str (), &rs->buf[2]);
- else
- error (_("Could not disable branch tracing for %s."),
- target_pid_to_str (tinfo->ptid).c_str ());
- }
+ target_pid_to_str (tinfo->ptid).c_str (), result.err_msg ());
delete tinfo;
}
@@ -15156,7 +15140,8 @@ remote_target::thread_events (int enable)
putpkt (rs->buf);
getpkt (&rs->buf);
- switch (m_features.packet_ok (rs->buf, PACKET_QThreadEvents))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_QThreadEvents);
+ switch (result.status ())
{
case PACKET_OK:
if (strcmp (rs->buf.data (), "OK") != 0)
@@ -15164,7 +15149,7 @@ remote_target::thread_events (int enable)
rs->last_thread_events = enable;
break;
case PACKET_ERROR:
- warning (_("Remote failure reply: %s"), rs->buf.data ());
+ warning (_("Remote failure reply: %s"), result.err_msg ());
break;
case PACKET_UNKNOWN:
break;
@@ -15211,14 +15196,15 @@ remote_target::commit_requested_thread_options ()
putpkt (rs->buf);
getpkt (&rs->buf, 0);
- switch (m_features.packet_ok (rs->buf, PACKET_QThreadOptions))
+ packet_result result = m_features.packet_ok (rs->buf, PACKET_QThreadOptions);
+ switch (result.status ())
{
case PACKET_OK:
if (strcmp (rs->buf.data (), "OK") != 0)
error (_("Remote refused setting thread options: %s"), rs->buf.data ());
break;
case PACKET_ERROR:
- error (_("Remote failure reply: %s"), rs->buf.data ());
+ error (_("Remote failure reply: %s"), result.err_msg ());
case PACKET_UNKNOWN:
gdb_assert_not_reached ("PACKET_UNKNOWN");
break;
--
2.43.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] remote.c: Use packet_check_result
2024-02-12 14:40 [PATCH 1/2] remote.c: Use packet_check_result Alexandra Hájková
2024-02-12 14:40 ` [PATCH 2/2] remote.c: Make packet_ok return struct packet_result Alexandra Hájková
@ 2024-02-26 13:15 ` Alexandra Petlanova Hajkova
2024-03-06 15:04 ` Andrew Burgess
2 siblings, 0 replies; 6+ messages in thread
From: Alexandra Petlanova Hajkova @ 2024-02-26 13:15 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 7565 bytes --]
ping
On Mon, Feb 12, 2024 at 3:40 PM Alexandra Hájková <ahajkova@redhat.com>
wrote:
> when processing the GDBserver reply to qRcmd packet.
> Print error message or the error code.
> Currently, when qRcmd request returns an error,
> GDB just prints:
>
> Protocol error with Rcmd
>
> After this change, GDB will also print the error code:
>
> Protocol error with Rcmd: 01.
>
> Add an accept_msg argument to packet_check result. qRcmd
> request (such as many other packets) does not recognise
> "E.msg" form as an error right now. We want to recognise
> "E.msg" as an error response only for the packets where
> it's documented.
> ---
> gdb/remote.c | 65 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 14c8b020b1e..8caee0dcff9 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2455,7 +2455,7 @@ add_packet_config_cmd (const unsigned int
> which_packet, const char *name,
> 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)
> +packet_check_result (const char *buf, bool accept_msg)
> {
> if (buf[0] != '\0')
> {
> @@ -2467,14 +2467,20 @@ packet_check_result (const char *buf)
> /* "Enn" - definitely an 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] == '.')
> + /* 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)
> {
> - if (buf[2] != '\0')
> - return { PACKET_ERROR, buf + 2 };
> - else
> - return { PACKET_ERROR, "no error provided" };
> + /* 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_ERROR, buf + 2 };
> + else
> + return { PACKET_ERROR, "no error provided" };
> + }
> }
>
> /* The packet may or may not be OK. Just assume it is. */
> @@ -2488,9 +2494,9 @@ packet_check_result (const char *buf)
> }
>
> static packet_result
> -packet_check_result (const gdb::char_vector &buf)
> +packet_check_result (const gdb::char_vector &buf, bool accept_msg)
> {
> - return packet_check_result (buf.data ());
> + return packet_check_result (buf.data (), accept_msg);
> }
>
> packet_status
> @@ -2503,7 +2509,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);
> + packet_result result = packet_check_result (buf, true);
> switch (result.status ())
> {
> case PACKET_OK:
> @@ -8831,7 +8837,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);
> + packet_result result = packet_check_result (rs->buf, true);
> if (result.status () == PACKET_ERROR)
> error (_("Could not read registers; remote failure reply '%s'"),
> result.err_msg ());
> @@ -9140,7 +9146,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);
> + packet_result pkt_status = packet_check_result (rs->buf, true);
> if (pkt_status.status () == PACKET_ERROR)
> error (_("Could not write registers; remote failure reply '%s'"),
> pkt_status.err_msg ());
> @@ -9748,7 +9754,7 @@ remote_target::remote_send_printf (const char
> *format, ...)
> rs->buf[0] = '\0';
> getpkt (&rs->buf);
>
> - return packet_check_result (rs->buf).status ();
> + return packet_check_result (rs->buf, true).status ();
> }
>
> /* Flash writing can take quite some time. We'll set
> @@ -11931,20 +11937,19 @@ remote_target::rcmd (const char *command, struct
> ui_file *outbuf)
> continue;
> }
> buf = rs->buf.data ();
> - if (buf[0] == '\0')
> - error (_("Target does not support this command."));
> if (buf[0] == 'O' && buf[1] != 'K')
> {
> remote_console_output (buf + 1); /* 'O' message from stub. */
> continue;
> }
> - if (strcmp (buf, "OK") == 0)
> + packet_result result = packet_check_result (buf, true);
> + if (result.status () == PACKET_OK)
> break;
> - if (strlen (buf) == 3 && buf[0] == 'E'
> - && isxdigit (buf[1]) && isxdigit (buf[2]))
> - {
> - error (_("Protocol error with Rcmd"));
> - }
> + else if (result.status () == PACKET_UNKNOWN)
> + error (_("Target does not support this command."));
> + else
> + error (_("Protocol error with Rcmd: %s."), result.err_msg ());
> +
> for (p = buf; p[0] != '\0' && p[1] != '\0'; p += 2)
> {
> char c = (fromhex (p[0]) << 4) + fromhex (p[1]);
> @@ -15571,7 +15576,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).status () == PACKET_OK;
> + return (packet_check_result (rs->buf, true).status () == PACKET_OK);
> }
>
> /* Return true if remote target T is non-stop. */
> @@ -15672,26 +15677,26 @@ static void
> test_packet_check_result ()
> {
> std::string buf = "E.msg";
> - packet_result result = packet_check_result (buf.data ());
> + packet_result result = packet_check_result (buf.data (), true);
>
> SELF_CHECK (result.status () == PACKET_ERROR);
> SELF_CHECK (strcmp(result.err_msg (), "msg") == 0);
>
> - result = packet_check_result ("E01");
> + result = packet_check_result ("E01", true);
> SELF_CHECK (result.status () == PACKET_ERROR);
> SELF_CHECK (strcmp(result.err_msg (), "01") == 0);
>
> - SELF_CHECK (packet_check_result ("E1").status () == PACKET_OK);
> + SELF_CHECK (packet_check_result ("E1", true).status () == PACKET_OK);
>
> - SELF_CHECK (packet_check_result ("E000").status () == PACKET_OK);
> + SELF_CHECK (packet_check_result ("E000", true).status () == PACKET_OK);
>
> - result = packet_check_result ("E.");
> + result = packet_check_result ("E.", true);
> SELF_CHECK (result.status () == PACKET_ERROR);
> SELF_CHECK (strcmp(result.err_msg (), "no error provided") == 0);
>
> - SELF_CHECK (packet_check_result ("some response").status () ==
> PACKET_OK);
> + SELF_CHECK (packet_check_result ("some response", true).status () ==
> PACKET_OK);
>
> - SELF_CHECK (packet_check_result ("").status () == PACKET_UNKNOWN);
> + SELF_CHECK (packet_check_result ("", true).status () == PACKET_UNKNOWN);
> }
> } // namespace selftests
> #endif /* GDB_SELF_TEST */
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] remote.c: Make packet_ok return struct packet_result
2024-02-12 14:40 ` [PATCH 2/2] remote.c: Make packet_ok return struct packet_result Alexandra Hájková
@ 2024-02-26 13:16 ` Alexandra Petlanova Hajkova
2024-03-06 16:13 ` Andrew Burgess
1 sibling, 0 replies; 6+ messages in thread
From: Alexandra Petlanova Hajkova @ 2024-02-26 13:16 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 22081 bytes --]
ping
On Mon, Feb 12, 2024 at 3:40 PM Alexandra Hájková <ahajkova@redhat.com>
wrote:
> This allows to print the error message stored in a packet_result
> to be easily used in the calling function.
> ---
> gdb/remote.c | 166 +++++++++++++++++++++++----------------------------
> 1 file changed, 76 insertions(+), 90 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8caee0dcff9..85f5624f2b6 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -765,8 +765,8 @@ struct remote_features
>
> /* Check result value in BUF for packet WHICH_PACKET and update the
> packet's
> support configuration accordingly. */
> - packet_status packet_ok (const char *buf, const int which_packet);
> - packet_status packet_ok (const gdb::char_vector &buf, const int
> which_packet);
> + packet_result packet_ok (const char *buf, const int which_packet);
> + packet_result 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;
> @@ -2499,7 +2499,7 @@ packet_check_result (const gdb::char_vector &buf,
> bool accept_msg)
> return packet_check_result (buf.data (), accept_msg);
> }
>
> -packet_status
> +packet_result
> remote_features::packet_ok (const char *buf, const int which_packet)
> {
> packet_config *config = &m_protocol_packets[which_packet];
> @@ -2545,10 +2545,10 @@ remote_features::packet_ok (const char *buf, const
> int which_packet)
> break;
> }
>
> - return result.status ();
> + return result;
> }
>
> -packet_status
> +packet_result
> remote_features::packet_ok (const gdb::char_vector &buf, const int
> which_packet)
> {
> return packet_ok (buf.data (), which_packet);
> @@ -2735,14 +2735,15 @@ remote_target::remote_query_attached (int pid)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_qAttached))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_qAttached);
> + switch (result.status())
> {
> case PACKET_OK:
> if (strcmp (rs->buf.data (), "1") == 0)
> return 1;
> break;
> case PACKET_ERROR:
> - warning (_("Remote failure reply: %s"), rs->buf.data ());
> + warning (_("Remote failure reply: %s"), result.err_msg());
> break;
> case PACKET_UNKNOWN:
> break;
> @@ -3047,7 +3048,6 @@ 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_status result;
> int n_sysno = 0;
>
> if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
> @@ -3103,8 +3103,8 @@ remote_target::set_syscall_catchpoint (int pid, bool
> needed, int any_count,
>
> putpkt (catch_packet);
> getpkt (&rs->buf);
> - result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
> - if (result == PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QCatchSyscalls);
> + if (result.status() == PACKET_OK)
> return 0;
> else
> return -1;
> @@ -5109,7 +5109,8 @@ remote_target::start_remote_1 (int from_tty, int
> extended_p)
> {
> putpkt ("QStartNoAckMode");
> getpkt (&rs->buf);
> - if (m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode) ==
> PACKET_OK)
> + if ((m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode)).status
> ()
> + == PACKET_OK)
> rs->noack_mode = 1;
> }
>
> @@ -5894,9 +5895,10 @@ remote_target::remote_query_supported ()
>
> /* If an error occurred, warn, but do not return - just reset the
> buffer to empty and go on to disable features. */
> - if (m_features.packet_ok (rs->buf, PACKET_qSupported) ==
> PACKET_ERROR)
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_qSupported);
> + if (result.status () == PACKET_ERROR)
> {
> - warning (_("Remote failure reply: %s"), rs->buf.data ());
> + warning (_("Remote failure reply: %s"), result.err_msg ());
> rs->buf[0] = 0;
> }
> }
> @@ -6548,7 +6550,8 @@ extended_remote_target::attach (const char *args,
> int from_tty)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vAttach))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_vAttach);
> + switch (result.status ())
> {
> case PACKET_OK:
> if (!target_is_non_stop_p ())
> @@ -6560,7 +6563,7 @@ extended_remote_target::attach (const char *args,
> int from_tty)
> else if (strcmp (rs->buf.data (), "OK") != 0)
> error (_("Attaching to %s failed with: %s"),
> target_pid_to_str (ptid_t (pid)).c_str (),
> - rs->buf.data ());
> + result.err_msg ());
> break;
> case PACKET_UNKNOWN:
> error (_("This target does not support attaching to a process"));
> @@ -7489,14 +7492,15 @@ remote_target::remote_interrupt_ns ()
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vCtrlC))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_vCtrlC);
> + switch (result.status ())
> {
> case PACKET_OK:
> break;
> case PACKET_UNKNOWN:
> error (_("No support for interrupting the remote target."));
> case PACKET_ERROR:
> - error (_("Interrupting target failed: %s"), rs->buf.data ());
> + error (_("Interrupting target failed: %s"), result.err_msg ());
> }
> }
>
> @@ -8792,7 +8796,8 @@ remote_target::fetch_register_using_p (struct
> regcache *regcache,
>
> buf = rs->buf.data ();
>
> - switch (m_features.packet_ok (rs->buf, PACKET_p))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_p);
> + switch (result.status ())
> {
> case PACKET_OK:
> break;
> @@ -8801,7 +8806,7 @@ remote_target::fetch_register_using_p (struct
> regcache *regcache,
> case PACKET_ERROR:
> error (_("Could not fetch register \"%s\"; remote failure reply
> '%s'"),
> gdbarch_register_name (regcache->arch (), reg->regnum),
> - buf);
> + result.err_msg ());
> }
>
> /* If this register is unfetchable, tell the regcache. */
> @@ -9098,13 +9103,14 @@ remote_target::store_register_using_P (const
> struct regcache *regcache,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_P))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_P);
> + switch (result.status ())
> {
> case PACKET_OK:
> return 1;
> case PACKET_ERROR:
> error (_("Could not write register \"%s\"; remote failure reply
> '%s'"),
> - gdbarch_register_name (gdbarch, reg->regnum), rs->buf.data
> ());
> + gdbarch_register_name (gdbarch, reg->regnum), result.err_msg
> ());
> case PACKET_UNKNOWN:
> return 0;
> default:
> @@ -10535,7 +10541,7 @@ remote_target::remote_vkill (int pid)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vKill))
> + switch ((m_features.packet_ok (rs->buf, PACKET_vKill)).status ())
> {
> case PACKET_OK:
> return 0;
> @@ -10691,7 +10697,7 @@ remote_target::extended_remote_run (const
> std::string &args)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vRun))
> + switch ((m_features.packet_ok (rs->buf, PACKET_vRun)).status ())
> {
> case PACKET_OK:
> /* We have a wait response. All is well. */
> @@ -10798,11 +10804,12 @@ remote_target::extended_remote_set_inferior_cwd
> ()
>
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - if (m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir) !=
> PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QSetWorkingDir);
> + if (result.status () != PACKET_OK)
> error (_("\
> Remote replied unexpectedly while setting the inferior's working\n\
> directory: %s"),
> - rs->buf.data ());
> + result.err_msg ());
>
> }
> }
> @@ -10971,7 +10978,7 @@ remote_target::insert_breakpoint (struct gdbarch
> *gdbarch,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_Z0))
> + switch ((m_features.packet_ok (rs->buf, PACKET_Z0)).status ())
> {
> case PACKET_ERROR:
> return -1;
> @@ -11072,8 +11079,8 @@ remote_target::insert_watchpoint (CORE_ADDR addr,
> int len,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> - + to_underlying (packet))))
> + switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> + + to_underlying
> (packet)))).status ())
> {
> case PACKET_ERROR:
> return -1;
> @@ -11121,8 +11128,8 @@ remote_target::remove_watchpoint (CORE_ADDR addr,
> int len,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> - + to_underlying (packet))))
> + switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> + + to_underlying
> (packet)))).status ())
> {
> case PACKET_ERROR:
> case PACKET_UNKNOWN:
> @@ -11284,7 +11291,7 @@ remote_target::insert_hw_breakpoint (struct
> gdbarch *gdbarch,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> + switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
> {
> case PACKET_ERROR:
> if (rs->buf[1] == '.')
> @@ -11331,7 +11338,7 @@ remote_target::remove_hw_breakpoint (struct
> gdbarch *gdbarch,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> + switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
> {
> case PACKET_ERROR:
> case PACKET_UNKNOWN:
> @@ -11372,7 +11379,7 @@ remote_target::verify_memory (const gdb_byte
> *data, CORE_ADDR lma, ULONGEST size
>
> getpkt (&rs->buf);
>
> - status = m_features.packet_ok (rs->buf, PACKET_qCRC);
> + status = (m_features.packet_ok (rs->buf, PACKET_qCRC)).status ();
> if (status == PACKET_ERROR)
> return -1;
> else if (status == PACKET_OK)
> @@ -11494,7 +11501,7 @@ remote_target::remote_write_qxfer (const char
> *object_name,
>
> if (putpkt_binary (rs->buf.data (), i + buf_len) < 0
> || getpkt (&rs->buf) < 0
> - || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> + || (m_features.packet_ok (rs->buf, which_packet)).status () !=
> PACKET_OK)
> return TARGET_XFER_E_IO;
>
> unpack_varlen_hex (rs->buf.data (), &n);
> @@ -11559,7 +11566,7 @@ remote_target::remote_read_qxfer (const char
> *object_name,
> rs->buf[0] = '\0';
> packet_len = getpkt (&rs->buf);
> if (packet_len < 0
> - || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> + || (m_features.packet_ok (rs->buf, which_packet)).status () !=
> PACKET_OK)
> return TARGET_XFER_E_IO;
>
> if (rs->buf[0] != 'l' && rs->buf[0] != 'm')
> @@ -11864,7 +11871,8 @@ remote_target::search_memory (CORE_ADDR
> start_addr, ULONGEST search_space_len,
>
> if (putpkt_binary (rs->buf.data (), i + escaped_pattern_len) < 0
> || getpkt (&rs->buf) < 0
> - || m_features.packet_ok (rs->buf, PACKET_qSearch_memory) !=
> PACKET_OK)
> + || (m_features.packet_ok (rs->buf, PACKET_qSearch_memory)).status ()
> + != PACKET_OK)
> {
> /* The request may not have worked because the command is not
> supported. If so, fall back to the simple way. */
> @@ -12257,7 +12265,6 @@ 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_status result;
>
> strcpy (p, "qGetTLSAddr:");
> p += strlen (p);
> @@ -12270,15 +12277,15 @@ remote_target::get_thread_local_address (ptid_t
> ptid, CORE_ADDR lm,
>
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
> - if (result == PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_qGetTLSAddr);
> + if (result.status () == PACKET_OK)
> {
> ULONGEST addr;
>
> unpack_varlen_hex (rs->buf.data (), &addr);
> return addr;
> }
> - else if (result == PACKET_UNKNOWN)
> + else if (result.status () == PACKET_UNKNOWN)
> throw_error (TLS_GENERIC_ERROR,
> _("Remote target doesn't support qGetTLSAddr
> packet"));
> else
> @@ -12303,7 +12310,6 @@ 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_status result;
>
> strcpy (p, "qGetTIBAddr:");
> p += strlen (p);
> @@ -12312,8 +12318,8 @@ remote_target::get_tib_address (ptid_t ptid,
> CORE_ADDR *addr)
>
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
> - if (result == PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_qGetTIBAddr);
> + if (result.status () == PACKET_OK)
> {
> ULONGEST val;
> unpack_varlen_hex (rs->buf.data (), &val);
> @@ -12321,7 +12327,7 @@ remote_target::get_tib_address (ptid_t ptid,
> CORE_ADDR *addr)
> *addr = (CORE_ADDR) val;
> return true;
> }
> - else if (result == PACKET_UNKNOWN)
> + else if (result.status () == PACKET_UNKNOWN)
> error (_("Remote target doesn't support qGetTIBAddr packet"));
> else
> error (_("Remote target failed to process qGetTIBAddr request"));
> @@ -12580,7 +12586,7 @@ remote_target::remote_hostio_send_command (int
> command_bytes, int which_packet,
> return -1;
> }
>
> - switch (m_features.packet_ok (rs->buf, which_packet))
> + switch ((m_features.packet_ok (rs->buf, which_packet)).status ())
> {
> case PACKET_ERROR:
> *remote_errno = FILEIO_EINVAL;
> @@ -13868,7 +13874,6 @@ remote_target::get_trace_status (struct
> trace_status *ts)
> {
> /* Initialize it just to avoid a GCC false warning. */
> char *p = NULL;
> - enum packet_status result;
> struct remote_state *rs = get_remote_state ();
>
> if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
> @@ -13894,17 +13899,17 @@ remote_target::get_trace_status (struct
> trace_status *ts)
> throw;
> }
>
> - result = m_features.packet_ok (p, PACKET_qTStatus);
> + packet_result result = m_features.packet_ok (p, PACKET_qTStatus);
>
> /* If the remote target doesn't do tracing, flag it. */
> - if (result == PACKET_UNKNOWN)
> + if (result.status () == PACKET_UNKNOWN)
> return -1;
>
> /* We're working with a live target. */
> ts->filename = NULL;
>
> if (*p++ != 'T')
> - error (_("Bogus trace status reply from target: %s"), rs->buf.data
> ());
> + error (_("Bogus trace status reply from target: %s"), result.err_msg
> ());
>
> /* Function 'parse_trace_status' sets default value of each field of
> 'ts' at first, so we don't have to do it here. */
> @@ -14248,7 +14253,6 @@ 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_status result;
>
> gdb_assert (val >= 0 || val == -1);
> buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> @@ -14263,10 +14267,10 @@ remote_target::set_trace_buffer_size (LONGEST
> val)
>
> putpkt (rs->buf);
> remote_get_noisy_reply ();
> - result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QTBuffer_size);
>
> - if (result != PACKET_OK)
> - warning (_("Bogus reply from target: %s"), rs->buf.data ());
> + if (result.status () != PACKET_OK)
> + warning (_("Bogus reply from target: %s"), result.err_msg ());
> }
> }
>
> @@ -14692,14 +14696,9 @@ remote_target::btrace_sync_conf (const
> btrace_config *conf)
> putpkt (buf);
> getpkt (&rs->buf);
>
> - if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_bts_size)
> - == PACKET_ERROR)
> - {
> - if (buf[0] == 'E' && buf[1] == '.')
> - error (_("Failed to configure the BTS buffer size: %s"), buf +
> 2);
> - else
> - error (_("Failed to configure the BTS buffer size."));
> - }
> + packet_result result = m_features.packet_ok (buf,
> PACKET_Qbtrace_conf_bts_size);
> + if (result.status () == PACKET_ERROR)
> + error (_("Failed to configure the BTS buffer size: %s"),
> result.err_msg ());
>
> rs->btrace_config.bts.size = conf->bts.size;
> }
> @@ -14715,14 +14714,9 @@ remote_target::btrace_sync_conf (const
> btrace_config *conf)
> putpkt (buf);
> getpkt (&rs->buf);
>
> - if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_pt_size)
> - == PACKET_ERROR)
> - {
> - if (buf[0] == 'E' && buf[1] == '.')
> - error (_("Failed to configure the trace buffer size: %s"), buf
> + 2);
> - else
> - error (_("Failed to configure the trace buffer size."));
> - }
> + packet_result result = m_features.packet_ok (buf,
> PACKET_Qbtrace_conf_pt_size);
> + if (result.status () == PACKET_ERROR)
> + error (_("Failed to configure the trace buffer size: %s"),
> result.err_msg ());
>
> rs->btrace_config.pt.size = conf->pt.size;
> }
> @@ -14837,15 +14831,10 @@ remote_target::enable_btrace (thread_info *tp,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - if (m_features.packet_ok (rs->buf, which_packet) == PACKET_ERROR)
> - {
> - if (rs->buf[0] == 'E' && rs->buf[1] == '.')
> - error (_("Could not enable branch tracing for %s: %s"),
> - target_pid_to_str (ptid).c_str (), &rs->buf[2]);
> - else
> - error (_("Could not enable branch tracing for %s."),
> - target_pid_to_str (ptid).c_str ());
> - }
> + packet_result result = m_features.packet_ok (rs->buf, which_packet);
> + if (result.status () == PACKET_ERROR)
> + error (_("Could not enable branch tracing for %s: %s"),
> + target_pid_to_str (ptid).c_str (), result.err_msg ());
>
> btrace_target_info *tinfo = new btrace_target_info { ptid };
>
> @@ -14883,15 +14872,10 @@ remote_target::disable_btrace (struct
> btrace_target_info *tinfo)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - if (m_features.packet_ok (rs->buf, PACKET_Qbtrace_off) == PACKET_ERROR)
> - {
> - if (rs->buf[0] == 'E' && rs->buf[1] == '.')
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_Qbtrace_off);
> + if (result.status () == PACKET_ERROR)
> error (_("Could not disable branch tracing for %s: %s"),
> - target_pid_to_str (tinfo->ptid).c_str (), &rs->buf[2]);
> - else
> - error (_("Could not disable branch tracing for %s."),
> - target_pid_to_str (tinfo->ptid).c_str ());
> - }
> + target_pid_to_str (tinfo->ptid).c_str (), result.err_msg
> ());
>
> delete tinfo;
> }
> @@ -15156,7 +15140,8 @@ remote_target::thread_events (int enable)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_QThreadEvents))
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QThreadEvents);
> + switch (result.status ())
> {
> case PACKET_OK:
> if (strcmp (rs->buf.data (), "OK") != 0)
> @@ -15164,7 +15149,7 @@ remote_target::thread_events (int enable)
> rs->last_thread_events = enable;
> break;
> case PACKET_ERROR:
> - warning (_("Remote failure reply: %s"), rs->buf.data ());
> + warning (_("Remote failure reply: %s"), result.err_msg ());
> break;
> case PACKET_UNKNOWN:
> break;
> @@ -15211,14 +15196,15 @@ remote_target::commit_requested_thread_options ()
> putpkt (rs->buf);
> getpkt (&rs->buf, 0);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_QThreadOptions))
> + packet_result result = m_features.packet_ok (rs->buf,
> PACKET_QThreadOptions);
> + switch (result.status ())
> {
> case PACKET_OK:
> if (strcmp (rs->buf.data (), "OK") != 0)
> error (_("Remote refused setting thread options: %s"),
> rs->buf.data ());
> break;
> case PACKET_ERROR:
> - error (_("Remote failure reply: %s"), rs->buf.data ());
> + error (_("Remote failure reply: %s"), result.err_msg ());
> case PACKET_UNKNOWN:
> gdb_assert_not_reached ("PACKET_UNKNOWN");
> break;
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] remote.c: Use packet_check_result
2024-02-12 14:40 [PATCH 1/2] remote.c: Use packet_check_result Alexandra Hájková
2024-02-12 14:40 ` [PATCH 2/2] remote.c: Make packet_ok return struct packet_result Alexandra Hájková
2024-02-26 13:15 ` [PATCH 1/2] remote.c: Use packet_check_result Alexandra Petlanova Hajkova
@ 2024-03-06 15:04 ` Andrew Burgess
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Burgess @ 2024-03-06 15:04 UTC (permalink / raw)
To: Alexandra Hájková, gdb-patches
Alexandra Hájková <ahajkova@redhat.com> writes:
> when processing the GDBserver reply to qRcmd packet.
> Print error message or the error code.
> Currently, when qRcmd request returns an error,
> GDB just prints:
>
> Protocol error with Rcmd
>
> After this change, GDB will also print the error code:
>
> Protocol error with Rcmd: 01.
>
> Add an accept_msg argument to packet_check result. qRcmd
> request (such as many other packets) does not recognise
> "E.msg" form as an error right now. We want to recognise
> "E.msg" as an error response only for the packets where
> it's documented.
> ---
> gdb/remote.c | 65 ++++++++++++++++++++++++++++------------------------
> 1 file changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 14c8b020b1e..8caee0dcff9 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2455,7 +2455,7 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
> 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)
> +packet_check_result (const char *buf, bool accept_msg)
I think the comment before this function should be extended to mention
the new ACCEPT_MSG argument, and should describe how to use it.
> {
> if (buf[0] != '\0')
> {
> @@ -2467,14 +2467,20 @@ packet_check_result (const char *buf)
> /* "Enn" - definitely an 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] == '.')
> + /* 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)
> {
> - if (buf[2] != '\0')
> - return { PACKET_ERROR, buf + 2 };
> - else
> - return { PACKET_ERROR, "no error provided" };
> + /* 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_ERROR, buf + 2 };
> + else
> + return { PACKET_ERROR, "no error provided" };
> + }
> }
>
> /* The packet may or may not be OK. Just assume it is. */
<snip>
> @@ -11931,20 +11937,19 @@ remote_target::rcmd (const char *command, struct ui_file *outbuf)
> continue;
> }
> buf = rs->buf.data ();
> - if (buf[0] == '\0')
> - error (_("Target does not support this command."));
> if (buf[0] == 'O' && buf[1] != 'K')
> {
> remote_console_output (buf + 1); /* 'O' message from stub. */
> continue;
> }
> - if (strcmp (buf, "OK") == 0)
> + packet_result result = packet_check_result (buf, true);
I was expecting the last parameter to be 'false' here as the commit
message says that Rcmd doesn't accept E.msg style errors.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] remote.c: Make packet_ok return struct packet_result
2024-02-12 14:40 ` [PATCH 2/2] remote.c: Make packet_ok return struct packet_result Alexandra Hájková
2024-02-26 13:16 ` Alexandra Petlanova Hajkova
@ 2024-03-06 16:13 ` Andrew Burgess
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Burgess @ 2024-03-06 16:13 UTC (permalink / raw)
To: Alexandra Hájková, gdb-patches
Alexandra Hájková <ahajkova@redhat.com> writes:
> This allows to print the error message stored in a packet_result
> to be easily used in the calling function.
> ---
> gdb/remote.c | 166 +++++++++++++++++++++++----------------------------
> 1 file changed, 76 insertions(+), 90 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 8caee0dcff9..85f5624f2b6 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -765,8 +765,8 @@ struct remote_features
>
> /* Check result value in BUF for packet WHICH_PACKET and update the packet's
> support configuration accordingly. */
> - packet_status packet_ok (const char *buf, const int which_packet);
> - packet_status packet_ok (const gdb::char_vector &buf, const int which_packet);
> + packet_result packet_ok (const char *buf, const int which_packet);
> + packet_result 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;
> @@ -2499,7 +2499,7 @@ packet_check_result (const gdb::char_vector &buf, bool accept_msg)
> return packet_check_result (buf.data (), accept_msg);
> }
>
> -packet_status
> +packet_result
> remote_features::packet_ok (const char *buf, const int which_packet)
> {
> packet_config *config = &m_protocol_packets[which_packet];
> @@ -2545,10 +2545,10 @@ remote_features::packet_ok (const char *buf, const int which_packet)
> break;
> }
>
> - return result.status ();
> + return result;
> }
>
> -packet_status
> +packet_result
> remote_features::packet_ok (const gdb::char_vector &buf, const int which_packet)
> {
> return packet_ok (buf.data (), which_packet);
> @@ -2735,14 +2735,15 @@ remote_target::remote_query_attached (int pid)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_qAttached))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_qAttached);
> + switch (result.status())
> {
> case PACKET_OK:
> if (strcmp (rs->buf.data (), "1") == 0)
> return 1;
> break;
> case PACKET_ERROR:
> - warning (_("Remote failure reply: %s"), rs->buf.data ());
> + warning (_("Remote failure reply: %s"), result.err_msg());
> break;
> case PACKET_UNKNOWN:
> break;
> @@ -3047,7 +3048,6 @@ 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_status result;
> int n_sysno = 0;
>
> if (m_features.packet_support (PACKET_QCatchSyscalls) == PACKET_DISABLE)
> @@ -3103,8 +3103,8 @@ remote_target::set_syscall_catchpoint (int pid, bool needed, int any_count,
>
> putpkt (catch_packet);
> getpkt (&rs->buf);
> - result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
> - if (result == PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_QCatchSyscalls);
> + if (result.status() == PACKET_OK)
> return 0;
> else
> return -1;
> @@ -5109,7 +5109,8 @@ remote_target::start_remote_1 (int from_tty, int extended_p)
> {
> putpkt ("QStartNoAckMode");
> getpkt (&rs->buf);
> - if (m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode) == PACKET_OK)
> + if ((m_features.packet_ok (rs->buf, PACKET_QStartNoAckMode)).status ()
> + == PACKET_OK)
> rs->noack_mode = 1;
> }
>
> @@ -5894,9 +5895,10 @@ remote_target::remote_query_supported ()
>
> /* If an error occurred, warn, but do not return - just reset the
> buffer to empty and go on to disable features. */
> - if (m_features.packet_ok (rs->buf, PACKET_qSupported) == PACKET_ERROR)
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_qSupported);
> + if (result.status () == PACKET_ERROR)
> {
> - warning (_("Remote failure reply: %s"), rs->buf.data ());
> + warning (_("Remote failure reply: %s"), result.err_msg ());
> rs->buf[0] = 0;
> }
> }
> @@ -6548,7 +6550,8 @@ extended_remote_target::attach (const char *args, int from_tty)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vAttach))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_vAttach);
> + switch (result.status ())
> {
> case PACKET_OK:
> if (!target_is_non_stop_p ())
> @@ -6560,7 +6563,7 @@ extended_remote_target::attach (const char *args, int from_tty)
> else if (strcmp (rs->buf.data (), "OK") != 0)
> error (_("Attaching to %s failed with: %s"),
> target_pid_to_str (ptid_t (pid)).c_str (),
> - rs->buf.data ());
> + result.err_msg ());
I don't think this change is correct. Notice that we're inside a
PACKET_OK case at this point. This is slightly confusing. When a
packet has packet_status PACKET_OK, it means the contents are not one of
the error forms, but otherwise the content could be anything. 'FOO'
would be PACKET_OK, as would the string 'OK'.
I wonder if this error() call should have a slightly different message,
maybe:
error (_("Attaching to %s returned unexpected packet: %s"),
target_pid_to_str (ptid_t (pid)).c_str (),
rs->buf.data ());
But the important thing is that we need to print the packet contents,
not 'result.err_msg ()' as that will not have been set.
Meanwhile, within this same switch statement, the 'default' case is
where PACKET_ERROR is handled, in this case it might be nice to add
printing of 'result.err_msg ()' as currently we don't do that. Maybe
the 'default:' case label should be changed to 'case PACKET_ERROR:',
relying on 'default:' here just seems sloppy.
> break;
> case PACKET_UNKNOWN:
> error (_("This target does not support attaching to a process"));
> @@ -7489,14 +7492,15 @@ remote_target::remote_interrupt_ns ()
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vCtrlC))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_vCtrlC);
> + switch (result.status ())
> {
> case PACKET_OK:
> break;
> case PACKET_UNKNOWN:
> error (_("No support for interrupting the remote target."));
> case PACKET_ERROR:
> - error (_("Interrupting target failed: %s"), rs->buf.data ());
> + error (_("Interrupting target failed: %s"), result.err_msg ());
> }
> }
>
> @@ -8792,7 +8796,8 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
>
> buf = rs->buf.data ();
>
> - switch (m_features.packet_ok (rs->buf, PACKET_p))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_p);
> + switch (result.status ())
> {
> case PACKET_OK:
> break;
> @@ -8801,7 +8806,7 @@ remote_target::fetch_register_using_p (struct regcache *regcache,
> case PACKET_ERROR:
> error (_("Could not fetch register \"%s\"; remote failure reply '%s'"),
> gdbarch_register_name (regcache->arch (), reg->regnum),
> - buf);
> + result.err_msg ());
> }
>
> /* If this register is unfetchable, tell the regcache. */
> @@ -9098,13 +9103,14 @@ remote_target::store_register_using_P (const struct regcache *regcache,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_P))
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_P);
> + switch (result.status ())
> {
> case PACKET_OK:
> return 1;
> case PACKET_ERROR:
> error (_("Could not write register \"%s\"; remote failure reply '%s'"),
> - gdbarch_register_name (gdbarch, reg->regnum), rs->buf.data ());
> + gdbarch_register_name (gdbarch, reg->regnum), result.err_msg ());
> case PACKET_UNKNOWN:
> return 0;
> default:
> @@ -10535,7 +10541,7 @@ remote_target::remote_vkill (int pid)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vKill))
> + switch ((m_features.packet_ok (rs->buf, PACKET_vKill)).status ())
> {
> case PACKET_OK:
> return 0;
> @@ -10691,7 +10697,7 @@ remote_target::extended_remote_run (const std::string &args)
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_vRun))
> + switch ((m_features.packet_ok (rs->buf, PACKET_vRun)).status ())
> {
> case PACKET_OK:
> /* We have a wait response. All is well. */
> @@ -10798,11 +10804,12 @@ remote_target::extended_remote_set_inferior_cwd ()
>
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - if (m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir) != PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_QSetWorkingDir);
> + if (result.status () != PACKET_OK)
> error (_("\
> Remote replied unexpectedly while setting the inferior's working\n\
> directory: %s"),
> - rs->buf.data ());
> + result.err_msg ());
We need to be careful here too. There's a risk that we might get
PACKET_UNKNOWN in which case calling 'result.err_msg ()' will be
invalid. I think you might need to change this into a switch statement
and handle each case differently:
PACKET_OK - nothing to do,
PACKET_ERROR - print a message including the error text,
PACKET_UNKNOWN - tell the user that the remote doesn't support this.
>
> }
> }
> @@ -10971,7 +10978,7 @@ remote_target::insert_breakpoint (struct gdbarch *gdbarch,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_Z0))
> + switch ((m_features.packet_ok (rs->buf, PACKET_Z0)).status ())
> {
> case PACKET_ERROR:
> return -1;
> @@ -11072,8 +11079,8 @@ remote_target::insert_watchpoint (CORE_ADDR addr, int len,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> - + to_underlying (packet))))
> + switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> + + to_underlying (packet)))).status ())
> {
> case PACKET_ERROR:
> return -1;
> @@ -11121,8 +11128,8 @@ remote_target::remove_watchpoint (CORE_ADDR addr, int len,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> - + to_underlying (packet))))
> + switch ((m_features.packet_ok (rs->buf, (to_underlying (PACKET_Z0)
> + + to_underlying (packet)))).status ())
> {
> case PACKET_ERROR:
> case PACKET_UNKNOWN:
> @@ -11284,7 +11291,7 @@ remote_target::insert_hw_breakpoint (struct gdbarch *gdbarch,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> + switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
> {
> case PACKET_ERROR:
> if (rs->buf[1] == '.')
So the code here (the existing code, not what you've written) is a real
mess. In the PACKET_ERROR case we attempt to "find" an error message in
the case that the packet is an 'E.msg' style packet.
However, if we do then we call 'error()', meanwhile, if the packet isn't
an 'E.msg' style packet, we return -1, which is pretty different than
throwing an exception.
This caught my eye because when I say the 'E.msg' check I thought, Oh,
we can just capture the packet_result object and call ::err_msg(),
however, this leads to an obvious question: should we throw an
exception, or return -1?
The comment in target.h for target_insert_hw_breakpoint suggests that
the function can _either_ return non-zero OR throw an exception on
error, so I'd be tempted to move to an always throw an error solution
for the PACKET_ERROR case.
> @@ -11331,7 +11338,7 @@ remote_target::remove_hw_breakpoint (struct gdbarch *gdbarch,
> putpkt (rs->buf);
> getpkt (&rs->buf);
>
> - switch (m_features.packet_ok (rs->buf, PACKET_Z1))
> + switch ((m_features.packet_ok (rs->buf, PACKET_Z1)).status ())
> {
> case PACKET_ERROR:
> case PACKET_UNKNOWN:
> @@ -11372,7 +11379,7 @@ remote_target::verify_memory (const gdb_byte *data, CORE_ADDR lma, ULONGEST size
>
> getpkt (&rs->buf);
>
> - status = m_features.packet_ok (rs->buf, PACKET_qCRC);
> + status = (m_features.packet_ok (rs->buf, PACKET_qCRC)).status ();
> if (status == PACKET_ERROR)
> return -1;
> else if (status == PACKET_OK)
> @@ -11494,7 +11501,7 @@ remote_target::remote_write_qxfer (const char *object_name,
>
> if (putpkt_binary (rs->buf.data (), i + buf_len) < 0
> || getpkt (&rs->buf) < 0
> - || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> + || (m_features.packet_ok (rs->buf, which_packet)).status () != PACKET_OK)
> return TARGET_XFER_E_IO;
>
> unpack_varlen_hex (rs->buf.data (), &n);
> @@ -11559,7 +11566,7 @@ remote_target::remote_read_qxfer (const char *object_name,
> rs->buf[0] = '\0';
> packet_len = getpkt (&rs->buf);
> if (packet_len < 0
> - || m_features.packet_ok (rs->buf, which_packet) != PACKET_OK)
> + || (m_features.packet_ok (rs->buf, which_packet)).status () != PACKET_OK)
> return TARGET_XFER_E_IO;
>
> if (rs->buf[0] != 'l' && rs->buf[0] != 'm')
> @@ -11864,7 +11871,8 @@ remote_target::search_memory (CORE_ADDR start_addr, ULONGEST search_space_len,
>
> if (putpkt_binary (rs->buf.data (), i + escaped_pattern_len) < 0
> || getpkt (&rs->buf) < 0
> - || m_features.packet_ok (rs->buf, PACKET_qSearch_memory) != PACKET_OK)
> + || (m_features.packet_ok (rs->buf, PACKET_qSearch_memory)).status ()
> + != PACKET_OK)
> {
> /* The request may not have worked because the command is not
> supported. If so, fall back to the simple way. */
> @@ -12257,7 +12265,6 @@ 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_status result;
>
> strcpy (p, "qGetTLSAddr:");
> p += strlen (p);
> @@ -12270,15 +12277,15 @@ remote_target::get_thread_local_address (ptid_t ptid, CORE_ADDR lm,
>
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
> - if (result == PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_qGetTLSAddr);
> + if (result.status () == PACKET_OK)
> {
> ULONGEST addr;
>
> unpack_varlen_hex (rs->buf.data (), &addr);
> return addr;
> }
> - else if (result == PACKET_UNKNOWN)
> + else if (result.status () == PACKET_UNKNOWN)
> throw_error (TLS_GENERIC_ERROR,
> _("Remote target doesn't support qGetTLSAddr packet"));
> else
> @@ -12303,7 +12310,6 @@ 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_status result;
>
> strcpy (p, "qGetTIBAddr:");
> p += strlen (p);
> @@ -12312,8 +12318,8 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
>
> putpkt (rs->buf);
> getpkt (&rs->buf);
> - result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
> - if (result == PACKET_OK)
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_qGetTIBAddr);
> + if (result.status () == PACKET_OK)
> {
> ULONGEST val;
> unpack_varlen_hex (rs->buf.data (), &val);
> @@ -12321,7 +12327,7 @@ remote_target::get_tib_address (ptid_t ptid, CORE_ADDR *addr)
> *addr = (CORE_ADDR) val;
> return true;
> }
> - else if (result == PACKET_UNKNOWN)
> + else if (result.status () == PACKET_UNKNOWN)
> error (_("Remote target doesn't support qGetTIBAddr packet"));
> else
> error (_("Remote target failed to process qGetTIBAddr request"));
This seems like somewhere that we could make use of result.err_msg().
Not a hard requirement, but given the nature of this series I think this
would be appropriate.
> @@ -12580,7 +12586,7 @@ remote_target::remote_hostio_send_command (int command_bytes, int which_packet,
> return -1;
> }
>
> - switch (m_features.packet_ok (rs->buf, which_packet))
> + switch ((m_features.packet_ok (rs->buf, which_packet)).status ())
> {
> case PACKET_ERROR:
> *remote_errno = FILEIO_EINVAL;
> @@ -13868,7 +13874,6 @@ remote_target::get_trace_status (struct trace_status *ts)
> {
> /* Initialize it just to avoid a GCC false warning. */
> char *p = NULL;
> - enum packet_status result;
> struct remote_state *rs = get_remote_state ();
>
> if (m_features.packet_support (PACKET_qTStatus) == PACKET_DISABLE)
> @@ -13894,17 +13899,17 @@ remote_target::get_trace_status (struct trace_status *ts)
> throw;
> }
>
> - result = m_features.packet_ok (p, PACKET_qTStatus);
> + packet_result result = m_features.packet_ok (p, PACKET_qTStatus);
>
> /* If the remote target doesn't do tracing, flag it. */
> - if (result == PACKET_UNKNOWN)
> + if (result.status () == PACKET_UNKNOWN)
> return -1;
>
> /* We're working with a live target. */
> ts->filename = NULL;
>
> if (*p++ != 'T')
> - error (_("Bogus trace status reply from target: %s"), rs->buf.data ());
> + error (_("Bogus trace status reply from target: %s"), result.err_msg ());
This is another place where we've not confirmed that we have a
PACKET_ERROR, it's just that the packet contents don't match the
"expected" format.
Maybe the original PACKET_UNKNOWN check should be changed into a switch
that can also check for PACKET_ERROR and error() with an appropriate
message?
But right here, when we find invalid packet contents, we have to print
'rs->buf.data ()'.
>
> /* Function 'parse_trace_status' sets default value of each field of
> 'ts' at first, so we don't have to do it here. */
> @@ -14248,7 +14253,6 @@ 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_status result;
>
> gdb_assert (val >= 0 || val == -1);
> buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> @@ -14263,10 +14267,10 @@ remote_target::set_trace_buffer_size (LONGEST val)
>
> putpkt (rs->buf);
> remote_get_noisy_reply ();
> - result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
> + packet_result result = m_features.packet_ok (rs->buf, PACKET_QTBuffer_size);
>
> - if (result != PACKET_OK)
> - warning (_("Bogus reply from target: %s"), rs->buf.data ());
> + if (result.status () != PACKET_OK)
> + warning (_("Bogus reply from target: %s"), result.err_msg ());
Again, we don't know this is a PACKET_ERROR so ::err_msg() might not be
valid.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-06 16:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-12 14:40 [PATCH 1/2] remote.c: Use packet_check_result Alexandra Hájková
2024-02-12 14:40 ` [PATCH 2/2] remote.c: Make packet_ok return struct packet_result Alexandra Hájková
2024-02-26 13:16 ` Alexandra Petlanova Hajkova
2024-03-06 16:13 ` Andrew Burgess
2024-02-26 13:15 ` [PATCH 1/2] remote.c: Use packet_check_result Alexandra Petlanova Hajkova
2024-03-06 15:04 ` 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).