This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] gdbserver: Introduce write_error_msg


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]