Re: [PATCH 1/2] remote.c: Use packet_check_result

[Alexandra Petlanova Hajkova <ahajkova@redhat.com>]

[-- 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
>
>