[binutils-gdb] Improve vRun error reporting

[binutils-gdb] Improve vRun error reporting

[Pedro Alves]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=33befefc3d6bf643af9b521e85feaeff0b06ee7b

commit 33befefc3d6bf643af9b521e85feaeff0b06ee7b
Author: Pedro Alves <pedro@palves.net>
Date:   Fri Jan 26 18:00:42 2024 +0000

    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." textual
    error packet.  On the GDB side, however, GDB doesn't yet do anything
    with the textual error string.  This commit improves 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.
    
    To know whether we have a textual error message, extend packet_result
    to carry that information.  While at it, convert packet_result to use
    factory methods, and change its std::string parameter to a plain const
    char *, as that it always what we have handy to pass to it.
    
    Change-Id: Ib386f267522603f554b52a885b15229c9639e870
    Approved-By: Tom Tromey <tom@tromey.com>

Diff:
---
 gdb/remote.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 55 insertions(+), 12 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index c1a6c137dd4..d425e558422 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -158,20 +158,27 @@ enum packet_status
 /* Keeps packet's return value. If packet's return value is PACKET_ERROR,
    err_msg contains an error message string from E.string or the number
    stored as a string from E.num.  */
-struct packet_result
+class packet_result
 {
-  packet_result (enum packet_status status, std::string err_msg)
-    : m_status (status), m_err_msg (std::move (err_msg))
-  {
-    gdb_assert (status == PACKET_ERROR);
-  }
+private:
+  /* Private ctors for internal use.  Clients should use the public
+     factory static methods instead.  */
+
+  /* Construct a PACKET_ERROR packet_result.  */
+  packet_result (const char *err_msg, bool textual_err_msg)
+    : m_status (PACKET_ERROR),
+      m_err_msg (err_msg),
+      m_textual_err_msg (textual_err_msg)
+  {}
 
+  /* Construct an PACKET_OK/PACKET_UNKNOWN packet_result.  */
   explicit packet_result (enum packet_status status)
     : m_status (status)
   {
     gdb_assert (status != PACKET_ERROR);
   }
 
+public:
   enum packet_status status () const
   {
     return this->m_status;
@@ -183,9 +190,39 @@ struct packet_result
     return this->m_err_msg.c_str ();
   }
 
+  bool textual_err_msg () const
+  {
+    gdb_assert (this->m_status == PACKET_ERROR);
+    return this->m_textual_err_msg;
+  }
+
+  static packet_result make_numeric_error (const char *err_msg)
+  {
+    return packet_result (err_msg, false);
+  }
+
+  static packet_result make_textual_error (const char *err_msg)
+  {
+    return packet_result (err_msg, true);
+  }
+
+  static packet_result make_ok ()
+  {
+    return packet_result (PACKET_OK);
+  }
+
+  static packet_result make_unknown ()
+  {
+    return packet_result (PACKET_UNKNOWN);
+  }
+
 private:
   enum packet_status m_status;
   std::string m_err_msg;
+
+  /* True if we have a textual error message, from an "E.MESSAGE"
+     response.  */
+  bool m_textual_err_msg;
 };
 
 /* Enumeration of packets for a remote target.  */
@@ -2473,7 +2510,7 @@ packet_check_result (const char *buf, bool accept_msg)
 	  && isxdigit (buf[1]) && isxdigit (buf[2])
 	  && buf[3] == '\0')
 	/* "Enn"  - definitely an error.  */
-	return { PACKET_ERROR, buf + 1 };
+	return packet_result::make_numeric_error (buf + 1);
 
       /* Not every request accepts an error in a E.msg form.
 	 Some packets accepts only Enn, in this case E. is not
@@ -2485,19 +2522,19 @@ packet_check_result (const char *buf, bool accept_msg)
 	  if (buf[0] == 'E' && buf[1] == '.')
 	    {
 	      if (buf[2] != '\0')
-		return { PACKET_ERROR, buf + 2 };
+		return packet_result::make_textual_error (buf + 2);
 	      else
-		return { PACKET_ERROR, "no error provided" };
+		return packet_result::make_textual_error ("no error provided");
 	    }
 	}
 
       /* The packet may or may not be OK.  Just assume it is.  */
-      return packet_result (PACKET_OK);
+      return packet_result::make_ok ();
     }
   else
   {
     /* The stub does not support the packet.  */
-    return packet_result (PACKET_UNKNOWN);
+    return packet_result::make_unknown ();
   }
 }
 
@@ -10704,7 +10741,8 @@ remote_target::extended_remote_run (const std::string &args)
   putpkt (rs->buf);
   getpkt (&rs->buf);
 
-  switch ((m_features.packet_ok (rs->buf, PACKET_vRun)).status ())
+  packet_result result = m_features.packet_ok (rs->buf, PACKET_vRun);
+  switch (result.status ())
     {
     case PACKET_OK:
       /* We have a wait response.  All is well.  */
@@ -10712,6 +10750,11 @@ remote_target::extended_remote_run (const std::string &args)
     case PACKET_UNKNOWN:
       return -1;
     case PACKET_ERROR:
+      /* If we have a textual error message, print just that.  This
+	 makes remote debugging output the same as native output, when
+	 possible.  */
+      if (result.textual_err_msg ())
+	error (("%s"), result.err_msg ());
       if (remote_exec_file[0] == '\0')
 	error (_("Running the default executable on the remote target failed; "
 		 "try \"set remote exec-file\"?"));