[PATCH] gdbserver: Introduce write_error_msg

[PATCH] gdbserver: Introduce write_error_msg

[Pedro Alves]

Instead of writing an error message to (gdbserver's) stderr and then
returning back "E01" to GDB, send the error message to GDB directly
using the E.MSG format, so the user can potentially see it.  Several
places in the code already do this manually.  This patch adds a new
function (write_error_msg) that abstracts out the "E." prefix detail.

gdb/gdbserver/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* remote-utils.c (write_error_msg): New function.
	* remote-utils.h (write_error_msg): Declare.
	* server.c (handle_btrace_general_set)
	(handle_btrace_conf_general_set, handle_general_set)
	(handle_qxfer_btrace, handle_qxfer_btrace_conf, resume)
	(handle_v_requests): Use write_error_msg.
---
 gdb/gdbserver/remote-utils.c | 15 +++++++++
 gdb/gdbserver/remote-utils.h |  7 +++++
 gdb/gdbserver/server.c       | 73 +++++++++++++++++++++-----------------------
 3 files changed, 57 insertions(+), 38 deletions(-)

diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 3212ec9..b6c521b 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -1083,6 +1083,21 @@ write_enn (char *buf)
   buf[3] = '\0';
 }
 
+/* See remote-utils.h.  */
+
+void
+write_error_msg (char *buf, const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+
+  buf[0] = 'E';
+  buf[1] = '.';
+  vsprintf (own_buf + 2, fmt, ap);
+  va_end (ap);
+}
+
 #endif
 
 #ifndef IN_PROCESS_AGENT
diff --git a/gdb/gdbserver/remote-utils.h b/gdb/gdbserver/remote-utils.h
index 2ddf590..9f7a587 100644
--- a/gdb/gdbserver/remote-utils.h
+++ b/gdb/gdbserver/remote-utils.h
@@ -40,6 +40,13 @@ void remote_open (char *name);
 void remote_close (void);
 void write_ok (char *buf);
 void write_enn (char *buf);
+
+/* Write an RSP error message in BUF, using the "E.MSG" format.  The
+   error message is constructed using a printf style argument
+   list.  */
+void write_error_msg (char *buf, const char *fmt, ...)
+  ATTRIBUTE_PRINTF (2, 3);
+
 void initialize_async_io (void);
 void enable_async_io (void);
 void disable_async_io (void);
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index ef8dd03..006d768 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -500,14 +500,14 @@ handle_btrace_general_set (char *own_buf)
   if (ptid_equal (general_thread, null_ptid)
       || ptid_equal (general_thread, minus_one_ptid))
     {
-      strcpy (own_buf, "E.Must select a single thread.");
+      write_error_msg (own_buf, "Must select a single thread.");
       return -1;
     }
 
   thread = find_thread_ptid (general_thread);
   if (thread == NULL)
     {
-      strcpy (own_buf, "E.No such thread.");
+      write_error_msg (own_buf, "No such thread.");
       return -1;
     }
 
@@ -546,14 +546,14 @@ handle_btrace_conf_general_set (char *own_buf)
   if (ptid_equal (general_thread, null_ptid)
       || ptid_equal (general_thread, minus_one_ptid))
     {
-      strcpy (own_buf, "E.Must select a single thread.");
+      write_error_msg (own_buf, "Must select a single thread.");
       return -1;
     }
 
   thread = find_thread_ptid (general_thread);
   if (thread == NULL)
     {
-      strcpy (own_buf, "E.No such thread.");
+      write_error_msg (own_buf, "No such thread.");
       return -1;
     }
 
@@ -566,7 +566,7 @@ handle_btrace_conf_general_set (char *own_buf)
       size = strtoul (op + strlen ("bts:size="), &endp, 16);
       if (endp == NULL || *endp != 0 || errno != 0 || size > UINT_MAX)
 	{
-	  strcpy (own_buf, "E.Bad size value.");
+	  write_error_msg (own_buf, "Bad size value.");
 	  return -1;
 	}
 
@@ -581,7 +581,7 @@ handle_btrace_conf_general_set (char *own_buf)
       size = strtoul (op + strlen ("pt:size="), &endp, 16);
       if (endp == NULL || *endp != 0 || errno != 0 || size > UINT_MAX)
 	{
-	  strcpy (own_buf, "E.Bad size value.");
+	  write_error_msg (own_buf, "Bad size value.");
 	  return -1;
 	}
 
@@ -589,7 +589,7 @@ handle_btrace_conf_general_set (char *own_buf)
     }
   else
     {
-      strcpy (own_buf, "E.Bad Qbtrace configuration option.");
+      write_error_msg (own_buf, "Bad Qbtrace configuration option.");
       return -1;
     }
 
@@ -673,9 +673,10 @@ handle_general_set (char *own_buf)
 	enabled = 1;
       else
 	{
-	  fprintf (stderr, "Unknown catch-syscalls mode requested: %s\n",
-		   own_buf);
-	  write_enn (own_buf);
+	  /* Must make a copy of P, since P overlaps with OWN_BUF.  */
+	  write_error_msg (own_buf,
+			   "Unknown catch-syscalls mode requested: %s\n",
+			   std::string (p).c_str ());
 	  return;
 	}
 
@@ -727,19 +728,18 @@ handle_general_set (char *own_buf)
 	req = 1;
       else
 	{
-	  /* We don't know what this mode is, so complain to
-	     GDB.  */
-	  fprintf (stderr, "Unknown non-stop mode requested: %s\n",
-		   own_buf);
-	  write_enn (own_buf);
+	  /* We don't know what this mode is, so complain to GDB.
+	     Must make a copy of MODE, since MODE overlaps with
+	     OWN_BUF.  */
+	  write_error_msg (own_buf, "Unknown non-stop mode requested: %s\n",
+			   std::string (mode).c_str ());
 	  return;
 	}
 
       req_str = req ? "non-stop" : "all-stop";
       if (start_non_stop (req) != 0)
 	{
-	  fprintf (stderr, "Setting %s mode failed\n", req_str);
-	  write_enn (own_buf);
+	  write_error_msg (own_buf, "Setting %s mode failed\n", req_str);
 	  return;
 	}
 
@@ -787,7 +787,7 @@ handle_general_set (char *own_buf)
       else
 	{
 	  /* We don't know what this value is, so complain to GDB.  */
-	  sprintf (own_buf, "E.Unknown QAgent value");
+	  write_error_msg (own_buf, "Unknown QAgent value");
 	  return;
 	}
 
@@ -816,12 +816,12 @@ handle_general_set (char *own_buf)
 	req = TRIBOOL_TRUE;
       else
 	{
-	  char *mode_copy = xstrdup (mode);
-
-	  /* We don't know what this mode is, so complain to GDB.  */
-	  sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
-		   mode_copy);
-	  xfree (mode_copy);
+	  /* We don't know what this mode is, so complain to GDB.
+	     (Must make a copy of MODE, since MODE overlaps with
+	     OWN_BUF.)  */
+	  write_error_msg (own_buf,
+			   "Unknown thread-events mode requested: %s\n",
+			   std::string (mode).c_str ());
 	  return;
 	}
 
@@ -1735,20 +1735,20 @@ handle_qxfer_btrace (const char *annex,
   if (ptid_equal (general_thread, null_ptid)
       || ptid_equal (general_thread, minus_one_ptid))
     {
-      strcpy (own_buf, "E.Must select a single thread.");
+      write_error_msg (own_buf, "Must select a single thread.");
       return -3;
     }
 
   thread = find_thread_ptid (general_thread);
   if (thread == NULL)
     {
-      strcpy (own_buf, "E.No such thread.");
+      write_error_msg (own_buf, "No such thread.");
       return -3;
     }
 
   if (thread->btrace == NULL)
     {
-      strcpy (own_buf, "E.Btrace not enabled.");
+      write_error_msg (own_buf, "Btrace not enabled.");
       return -3;
     }
 
@@ -1760,7 +1760,7 @@ handle_qxfer_btrace (const char *annex,
     type = BTRACE_READ_DELTA;
   else
     {
-      strcpy (own_buf, "E.Bad annex.");
+      write_error_msg (own_buf, "Bad annex.");
       return -3;
     }
 
@@ -1809,20 +1809,20 @@ handle_qxfer_btrace_conf (const char *annex,
   if (ptid_equal (general_thread, null_ptid)
       || ptid_equal (general_thread, minus_one_ptid))
     {
-      strcpy (own_buf, "E.Must select a single thread.");
+      write_error_msg (own_buf, "Must select a single thread.");
       return -3;
     }
 
   thread = find_thread_ptid (general_thread);
   if (thread == NULL)
     {
-      strcpy (own_buf, "E.No such thread.");
+      write_error_msg (own_buf, "No such thread.");
       return -3;
     }
 
   if (thread->btrace == NULL)
     {
-      strcpy (own_buf, "E.Btrace not enabled.");
+      write_error_msg (own_buf, "Btrace not enabled.");
       return -3;
     }
 
@@ -2790,7 +2790,7 @@ resume (struct thread_resume *actions, size_t num_actions)
 	{
 	  /* The client does not support this stop reply.  At least
 	     return error.  */
-	  sprintf (own_buf, "E.No unwaited-for children left.");
+	  write_error_msg (own_buf, "No unwaited-for children left.");
 	  disable_async_io ();
 	  return;
 	}
@@ -3016,8 +3016,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
     {
       if ((!extended_protocol || !multi_process) && target_running ())
 	{
-	  fprintf (stderr, "Already debugging a process\n");
-	  write_enn (own_buf);
+	  write_error_msg (own_buf, "Already debugging a process");
 	  return;
 	}
       handle_v_attach (own_buf);
@@ -3028,8 +3027,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
     {
       if ((!extended_protocol || !multi_process) && target_running ())
 	{
-	  fprintf (stderr, "Already debugging a process\n");
-	  write_enn (own_buf);
+	  write_error_msg (own_buf, "Already debugging a process");
 	  return;
 	}
       handle_v_run (own_buf);
@@ -3040,8 +3038,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
     {
       if (!target_running ())
 	{
-	  fprintf (stderr, "No process to kill\n");
-	  write_enn (own_buf);
+	  write_error_msg (own_buf, "No process to kill");
 	  return;
 	}
       handle_v_kill (own_buf);
-- 
2.5.5

RE: [PATCH] gdbserver: Introduce write_error_msg

[Metzger, Markus T]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Wednesday, November 23, 2016 4:36 PM
> To: gdb-patches@sourceware.org
> Subject: [PATCH] gdbserver: Introduce write_error_msg
> 
> Instead of writing an error message to (gdbserver's) stderr and then
> returning back "E01" to GDB, send the error message to GDB directly
> using the E.MSG format, so the user can potentially see it.  Several
> places in the code already do this manually.  This patch adds a new
> function (write_error_msg) that abstracts out the "E." prefix detail.
> 
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* remote-utils.c (write_error_msg): New function.
> 	* remote-utils.h (write_error_msg): Declare.
> 	* server.c (handle_btrace_general_set)
> 	(handle_btrace_conf_general_set, handle_general_set)
> 	(handle_qxfer_btrace, handle_qxfer_btrace_conf, resume)
> 	(handle_v_requests): Use write_error_msg.

The btrace bits look good to me.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH] gdbserver: Introduce write_error_msg

[Luis Machado]

On 11/23/2016 09:35 AM, Pedro Alves wrote:
> Instead of writing an error message to (gdbserver's) stderr and then
> returning back "E01" to GDB, send the error message to GDB directly
> using the E.MSG format, so the user can potentially see it.  Several
> places in the code already do this manually.  This patch adds a new
> function (write_error_msg) that abstracts out the "E." prefix detail.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
>
> 	* remote-utils.c (write_error_msg): New function.
> 	* remote-utils.h (write_error_msg): Declare.
> 	* server.c (handle_btrace_general_set)
> 	(handle_btrace_conf_general_set, handle_general_set)
> 	(handle_qxfer_btrace, handle_qxfer_btrace_conf, resume)
> 	(handle_v_requests): Use write_error_msg.
> ---
>  gdb/gdbserver/remote-utils.c | 15 +++++++++
>  gdb/gdbserver/remote-utils.h |  7 +++++
>  gdb/gdbserver/server.c       | 73 +++++++++++++++++++++-----------------------
>  3 files changed, 57 insertions(+), 38 deletions(-)
>
> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 3212ec9..b6c521b 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -1083,6 +1083,21 @@ write_enn (char *buf)
>    buf[3] = '\0';
>  }
>
> +/* See remote-utils.h.  */
> +
> +void
> +write_error_msg (char *buf, const char *fmt, ...)
> +{
> +  va_list ap;
> +
> +  va_start (ap, fmt);
> +
> +  buf[0] = 'E';
> +  buf[1] = '.';
> +  vsprintf (own_buf + 2, fmt, ap);
> +  va_end (ap);
> +}
> +
>  #endif
>
>  #ifndef IN_PROCESS_AGENT
> diff --git a/gdb/gdbserver/remote-utils.h b/gdb/gdbserver/remote-utils.h
> index 2ddf590..9f7a587 100644
> --- a/gdb/gdbserver/remote-utils.h
> +++ b/gdb/gdbserver/remote-utils.h
> @@ -40,6 +40,13 @@ void remote_open (char *name);
>  void remote_close (void);
>  void write_ok (char *buf);
>  void write_enn (char *buf);
> +
> +/* Write an RSP error message in BUF, using the "E.MSG" format.  The
> +   error message is constructed using a printf style argument
> +   list.  */
> +void write_error_msg (char *buf, const char *fmt, ...)
> +  ATTRIBUTE_PRINTF (2, 3);
> +
>  void initialize_async_io (void);
>  void enable_async_io (void);
>  void disable_async_io (void);
> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
> index ef8dd03..006d768 100644
> --- a/gdb/gdbserver/server.c
> +++ b/gdb/gdbserver/server.c
> @@ -500,14 +500,14 @@ handle_btrace_general_set (char *own_buf)
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -1;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -1;
>      }
>
> @@ -546,14 +546,14 @@ handle_btrace_conf_general_set (char *own_buf)
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -1;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -1;
>      }
>
> @@ -566,7 +566,7 @@ handle_btrace_conf_general_set (char *own_buf)
>        size = strtoul (op + strlen ("bts:size="), &endp, 16);
>        if (endp == NULL || *endp != 0 || errno != 0 || size > UINT_MAX)
>  	{
> -	  strcpy (own_buf, "E.Bad size value.");
> +	  write_error_msg (own_buf, "Bad size value.");
>  	  return -1;
>  	}
>
> @@ -581,7 +581,7 @@ handle_btrace_conf_general_set (char *own_buf)
>        size = strtoul (op + strlen ("pt:size="), &endp, 16);
>        if (endp == NULL || *endp != 0 || errno != 0 || size > UINT_MAX)
>  	{
> -	  strcpy (own_buf, "E.Bad size value.");
> +	  write_error_msg (own_buf, "Bad size value.");
>  	  return -1;
>  	}
>
> @@ -589,7 +589,7 @@ handle_btrace_conf_general_set (char *own_buf)
>      }
>    else
>      {
> -      strcpy (own_buf, "E.Bad Qbtrace configuration option.");
> +      write_error_msg (own_buf, "Bad Qbtrace configuration option.");
>        return -1;
>      }
>
> @@ -673,9 +673,10 @@ handle_general_set (char *own_buf)
>  	enabled = 1;
>        else
>  	{
> -	  fprintf (stderr, "Unknown catch-syscalls mode requested: %s\n",
> -		   own_buf);
> -	  write_enn (own_buf);
> +	  /* Must make a copy of P, since P overlaps with OWN_BUF.  */
> +	  write_error_msg (own_buf,
> +			   "Unknown catch-syscalls mode requested: %s\n",
> +			   std::string (p).c_str ());
>  	  return;
>  	}
>
> @@ -727,19 +728,18 @@ handle_general_set (char *own_buf)
>  	req = 1;
>        else
>  	{
> -	  /* We don't know what this mode is, so complain to
> -	     GDB.  */
> -	  fprintf (stderr, "Unknown non-stop mode requested: %s\n",
> -		   own_buf);
> -	  write_enn (own_buf);
> +	  /* We don't know what this mode is, so complain to GDB.
> +	     Must make a copy of MODE, since MODE overlaps with
> +	     OWN_BUF.  */
> +	  write_error_msg (own_buf, "Unknown non-stop mode requested: %s\n",
> +			   std::string (mode).c_str ());
>  	  return;
>  	}
>
>        req_str = req ? "non-stop" : "all-stop";
>        if (start_non_stop (req) != 0)
>  	{
> -	  fprintf (stderr, "Setting %s mode failed\n", req_str);
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "Setting %s mode failed\n", req_str);
>  	  return;
>  	}
>
> @@ -787,7 +787,7 @@ handle_general_set (char *own_buf)
>        else
>  	{
>  	  /* We don't know what this value is, so complain to GDB.  */
> -	  sprintf (own_buf, "E.Unknown QAgent value");
> +	  write_error_msg (own_buf, "Unknown QAgent value");
>  	  return;
>  	}
>
> @@ -816,12 +816,12 @@ handle_general_set (char *own_buf)
>  	req = TRIBOOL_TRUE;
>        else
>  	{
> -	  char *mode_copy = xstrdup (mode);
> -
> -	  /* We don't know what this mode is, so complain to GDB.  */
> -	  sprintf (own_buf, "E.Unknown thread-events mode requested: %s\n",
> -		   mode_copy);
> -	  xfree (mode_copy);
> +	  /* We don't know what this mode is, so complain to GDB.
> +	     (Must make a copy of MODE, since MODE overlaps with
> +	     OWN_BUF.)  */
> +	  write_error_msg (own_buf,
> +			   "Unknown thread-events mode requested: %s\n",
> +			   std::string (mode).c_str ());
>  	  return;
>  	}
>
> @@ -1735,20 +1735,20 @@ handle_qxfer_btrace (const char *annex,
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -3;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -3;
>      }
>
>    if (thread->btrace == NULL)
>      {
> -      strcpy (own_buf, "E.Btrace not enabled.");
> +      write_error_msg (own_buf, "Btrace not enabled.");
>        return -3;
>      }
>
> @@ -1760,7 +1760,7 @@ handle_qxfer_btrace (const char *annex,
>      type = BTRACE_READ_DELTA;
>    else
>      {
> -      strcpy (own_buf, "E.Bad annex.");
> +      write_error_msg (own_buf, "Bad annex.");
>        return -3;
>      }
>
> @@ -1809,20 +1809,20 @@ handle_qxfer_btrace_conf (const char *annex,
>    if (ptid_equal (general_thread, null_ptid)
>        || ptid_equal (general_thread, minus_one_ptid))
>      {
> -      strcpy (own_buf, "E.Must select a single thread.");
> +      write_error_msg (own_buf, "Must select a single thread.");
>        return -3;
>      }
>
>    thread = find_thread_ptid (general_thread);
>    if (thread == NULL)
>      {
> -      strcpy (own_buf, "E.No such thread.");
> +      write_error_msg (own_buf, "No such thread.");
>        return -3;
>      }
>
>    if (thread->btrace == NULL)
>      {
> -      strcpy (own_buf, "E.Btrace not enabled.");
> +      write_error_msg (own_buf, "Btrace not enabled.");
>        return -3;
>      }
>
> @@ -2790,7 +2790,7 @@ resume (struct thread_resume *actions, size_t num_actions)
>  	{
>  	  /* The client does not support this stop reply.  At least
>  	     return error.  */
> -	  sprintf (own_buf, "E.No unwaited-for children left.");
> +	  write_error_msg (own_buf, "No unwaited-for children left.");
>  	  disable_async_io ();
>  	  return;
>  	}
> @@ -3016,8 +3016,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
>      {
>        if ((!extended_protocol || !multi_process) && target_running ())
>  	{
> -	  fprintf (stderr, "Already debugging a process\n");
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "Already debugging a process");
>  	  return;
>  	}
>        handle_v_attach (own_buf);
> @@ -3028,8 +3027,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
>      {
>        if ((!extended_protocol || !multi_process) && target_running ())
>  	{
> -	  fprintf (stderr, "Already debugging a process\n");
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "Already debugging a process");
>  	  return;
>  	}
>        handle_v_run (own_buf);
> @@ -3040,8 +3038,7 @@ handle_v_requests (char *own_buf, int packet_len, int *new_packet_len)
>      {
>        if (!target_running ())
>  	{
> -	  fprintf (stderr, "No process to kill\n");
> -	  write_enn (own_buf);
> +	  write_error_msg (own_buf, "No process to kill");
>  	  return;
>  	}
>        handle_v_kill (own_buf);
>

This looks OK to me, though the duplicate messages caught my attention. 
We could do something about them (having a predefined string that gets 
reused).

Not sure if we care about translations of such messages. Anyway, just a 
suggestion.

Re: [PATCH] gdbserver: Introduce write_error_msg

[Philippe Waroquiers]

On Wed, 2016-11-23 at 15:35 +0000, Pedro Alves wrote:
> Instead of writing an error message to (gdbserver's) stderr and then
> returning back "E01" to GDB, send the error message to GDB directly
> using the E.MSG format, so the user can potentially see it.  Several
> places in the code already do this manually.  This patch adds a new
> function (write_error_msg) that abstracts out the "E." prefix detail.
The protocol description/doc does not (very) clearly describe E. prefix.
In the doc, I found the E. is used only at a few places:
  vFlashWrite and a few packets used for branch tracing,
while the gdbserver code uses it at more places.

I think that gdb will accept such an error reply
as a reply to any? many?  packet that can cause an error
(at least I have used E. here and there in Valgrind gdbserver
and it is always accepted by gdb).

The E (error packet) is not formally described.
In the protocol Overview, there is:
"   The error response returned for some packets includes a two
character error number.  That number is not well defined."

But the E packet itself seems not well defined :).

This patch is maybe the occasion to clarify/document
explicitely what gdbserver stubs are allowed to send as an
error response, and then change the doc to just indicate
what packets can send an E error message, rather than
document that 'E NN' is only to be expected for many
packets. 

Thanks

Philippe