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

[Andrew Burgess <aburgess@redhat.com>]

Alexandra Hájková <ahajkova@khirnov.net> writes:

> From: Alexandra Hájková <ahajkova@redhat.com>
>
> 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 | 75 +++++++++++++++++++++++++++++-----------------------
>  1 file changed, 42 insertions(+), 33 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 14c8b020b1e..8462b7e4e60 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2451,11 +2451,15 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
>      }
>  }
>  
> -/* Check GDBserver's reply packet.  Return packet_result
> -   structure which contains the packet_status enum
> -   and an error message for the PACKET_ERROR case.  */
> +/* Check GDBserver's reply packet.  Return packet_result structure
> +   which contains the packet_status enum and an error message for the
> +   PACKET_ERROR case.
> +
> +   An error packet can always take the form Exx (where xx is a hex
> +   code).  When ACCEPT_MSG is true error messages can also take the
> +   form E.msg (where msg is any arbitrary string).  */
>  static packet_result
> -packet_check_result (const char *buf)
> +packet_check_result (const char *buf, bool accept_msg)
>  {
>    if (buf[0] != '\0')
>      {
> @@ -2467,14 +2471,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 +2498,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 +2513,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 +8841,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 +9150,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 +9758,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 +11941,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, false);
> +      if (result.status () == PACKET_OK)
>  	break;

Unfortunately this is a change in behaviour.  PACKET_OK status does not
mean that the packet contains the text "OK", it just means that the
packet was not an error, and was not the empty (PACKET_UNKNOWN) packet.

If the packet contained the text "ABCD" then previously we would end up
in the loop below which converts the value from a hex-encoded string,
but now I believe we'll end up breaking out of the while loop.

> -      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 +15580,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 +15681,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);

I wonder if we should add a unit test here where we pass an 'E.msg' and
pass the second argument to packet_check_result as false and ensure we
get back the results we expect (which I think is PACKET_OK).

Thanks,
Andrew


>  }
>  } // namespace selftests
>  #endif /* GDB_SELF_TEST */
> -- 
> 2.44.0