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

  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(-)
  

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