[2/3] Improve vRun error reporting

  After the previous commit, if starting the inferior process with "run"
(vRun packet) fails, GDBserver reports an error using the "E." verbose
error packet.  On the GDB side, however, GDB doesn't yet do anything
with verbose error strings when handling vRun errors.  This commit
fixes that.

This makes remote debugging output the same as native output, when
possible, another small step in the "local/remote parity" project.

E.g., before, against GNU/Linux GDBserver:

  (gdb) run
  Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
  Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed

After, against GNU/Linux GDBserver (same as native):

  (gdb) run
  Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
  During startup program exited with code 126.

Change-Id: Ib386f267522603f554b52a885b15229c9639e870
---
 gdb/remote.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
  

Hi!

This conflicts with Alexandra's just-merged packet_check_result changes.  I see she has some follow up
patches that would help me here.  I will take a look at those.  For the meantime, disregard this patch.

Pedro Alves

On 2024-02-12 20:01, Pedro Alves wrote:
> After the previous commit, if starting the inferior process with "run"
> (vRun packet) fails, GDBserver reports an error using the "E." verbose
> error packet.  On the GDB side, however, GDB doesn't yet do anything
> with verbose error strings when handling vRun errors.  This commit
> fixes that.
> 
> This makes remote debugging output the same as native output, when
> possible, another small step in the "local/remote parity" project.
> 
> E.g., before, against GNU/Linux GDBserver:
> 
>   (gdb) run
>   Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
>   Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed
> 
> After, against GNU/Linux GDBserver (same as native):
> 
>   (gdb) run
>   Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
>   During startup program exited with code 126.
> 
> Change-Id: Ib386f267522603f554b52a885b15229c9639e870
> ---
>  gdb/remote.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index b58dbd4cb66..dca5add260a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2400,6 +2400,19 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
>      }
>  }
>  
> +/* Strings that starts with "E.", are verbose error messages, like
> +   "E.ERROR_MESSAGE".  If BUF is such an error message, return a
> +   pointer to message after the period.  Otherwise return NULL.  */
> +
> +static const char *
> +verbose_error_message (const char *buf)
> +{
> +  if (buf[0] == 'E' && buf[1] == '.')
> +    return buf + 2;
> +  else
> +    return nullptr;
> +}
> +
>  static enum packet_result
>  packet_check_result (const char *buf)
>  {
> @@ -2415,7 +2428,7 @@ packet_check_result (const char *buf)
>  
>        /* 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 (verbose_error_message (buf) != nullptr)
>  	return PACKET_ERROR;
>  
>        /* The packet may or may not be OK.  Just assume it is.  */
> @@ -10502,7 +10515,13 @@ remote_target::extended_remote_run (const std::string &args)
>      case PACKET_UNKNOWN:
>        return -1;
>      case PACKET_ERROR:
> -      if (remote_exec_file[0] == '\0')
> +      /* If we have a verbose error message, print just that.  This
> +	 makes remote debugging output the same as native output, when
> +	 possible.  */
> +      if (const char *msg = verbose_error_message (rs->buf.data ());
> +	  msg != nullptr)
> +	error (("%s"), msg);
> +      else if (remote_exec_file[0] == '\0')
>  	error (_("Running the default executable on the remote target failed; "
>  		 "try \"set remote exec-file\"?"));
>        else
  

Hi,

I only glanced at this patch, but it seems to me that is would
conflict with this patch: 

    commit 7e9d8a3627c8a80b76c250b6881b7eb6fc2f4443
    Date:   Fri Nov 24 14:33:42 2023 +0100
    
        remote.c: Make packet_check_result return a structure

https://sourceware.org/pipermail/gdb-patches/2024-February/206483.html

I expect that this patch would remove the need for
verbose_error_message, you just need to have some changes in
remote_target::extended_remote_run.

Best,
Lancelot.

On Mon, Feb 12, 2024 at 08:01:52PM +0000, Pedro Alves wrote:
> After the previous commit, if starting the inferior process with "run"
> (vRun packet) fails, GDBserver reports an error using the "E." verbose
> error packet.  On the GDB side, however, GDB doesn't yet do anything
> with verbose error strings when handling vRun errors.  This commit
> fixes that.
> 
> This makes remote debugging output the same as native output, when
> possible, another small step in the "local/remote parity" project.
> 
> E.g., before, against GNU/Linux GDBserver:
> 
>   (gdb) run
>   Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
>   Running ".../gdb.base/run-fail-twice/run-fail-twice.nox" on the remote target failed
> 
> After, against GNU/Linux GDBserver (same as native):
> 
>   (gdb) run
>   Starting program: .../gdb.base/run-fail-twice/run-fail-twice.nox
>   During startup program exited with code 126.
> 
> Change-Id: Ib386f267522603f554b52a885b15229c9639e870
> ---
>  gdb/remote.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index b58dbd4cb66..dca5add260a 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2400,6 +2400,19 @@ add_packet_config_cmd (const unsigned int which_packet, const char *name,
>      }
>  }
>  
> +/* Strings that starts with "E.", are verbose error messages, like
> +   "E.ERROR_MESSAGE".  If BUF is such an error message, return a
> +   pointer to message after the period.  Otherwise return NULL.  */
> +
> +static const char *
> +verbose_error_message (const char *buf)
> +{
> +  if (buf[0] == 'E' && buf[1] == '.')
> +    return buf + 2;
> +  else
> +    return nullptr;
> +}
> +
>  static enum packet_result
>  packet_check_result (const char *buf)
>  {
> @@ -2415,7 +2428,7 @@ packet_check_result (const char *buf)
ss>  
>        /* 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 (verbose_error_message (buf) != nullptr)
>  	return PACKET_ERROR;
>  
>        /* The packet may or may not be OK.  Just assume it is.  */
> @@ -10502,7 +10515,13 @@ remote_target::extended_remote_run (const std::string &args)
>      case PACKET_UNKNOWN:
>        return -1;
>      case PACKET_ERROR:
> -      if (remote_exec_file[0] == '\0')
> +      /* If we have a verbose error message, print just that.  This
> +	 makes remote debugging output the same as native output, when
> +	 possible.  */
> +      if (const char *msg = verbose_error_message (rs->buf.data ());
> +	  msg != nullptr)
> +	error (("%s"), msg);
> +      else if (remote_exec_file[0] == '\0')
>  	error (_("Running the default executable on the remote target failed; "
>  		 "try \"set remote exec-file\"?"));
>        else
> 
> -- 
> 2.43.0
>