From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 116648 invoked by alias); 23 Nov 2016 16:23:10 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 116628 invoked by uid 89); 23 Nov 2016 16:23:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 23 Nov 2016 16:22:59 +0000 Received: from svr-orw-mbx-03.mgc.mentorg.com ([147.34.90.203]) by relay1.mentorg.com with esmtp id 1c9aK0-0003oj-C1 from Luis_Gustavo@mentor.com ; Wed, 23 Nov 2016 08:22:56 -0800 Received: from [172.30.6.103] (147.34.91.1) by svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Wed, 23 Nov 2016 08:22:53 -0800 Subject: Re: [PATCH] gdbserver: Introduce write_error_msg References: <1479915352-23983-1-git-send-email-palves@redhat.com> To: Pedro Alves , Reply-To: Luis Machado From: Luis Machado Message-ID: <3247c080-4536-4cb9-755b-a740827f3214@codesourcery.com> Date: Wed, 23 Nov 2016 16:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <1479915352-23983-1-git-send-email-palves@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-02.mgc.mentorg.com (147.34.90.202) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00677.txt.bz2 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 > > * 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.