From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89332 invoked by alias); 22 Sep 2015 16:29:40 -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 89317 invoked by uid 89); 22 Sep 2015 16:29:39 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 22 Sep 2015 16:29:37 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 792E24C0B3; Tue, 22 Sep 2015 16:29:36 +0000 (UTC) Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t8MGTYOq013232; Tue, 22 Sep 2015 12:29:35 -0400 Message-ID: <560181EE.7010102@redhat.com> Date: Tue, 22 Sep 2015 16:29:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Simon Marchi , gdb-patches@sourceware.org Subject: Re: [PATCH] [PR breakpoints/18275] Introduce dprintf-flush-function for call-style dprintfs References: <1441827156-8518-1-git-send-email-simon.marchi@ericsson.com> In-Reply-To: <1441827156-8518-1-git-send-email-simon.marchi@ericsson.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-09/txt/msg00530.txt.bz2 On 09/09/2015 08:32 PM, Simon Marchi wrote: > A little bit of context: a colleague of mine likes debugging the JVM and > wants to use dynamic printfs to do so. Unfortunately, openjdk sets the > standard output as fully buffered, as opposed to line buffered by > default. This means that when using simple dprintfs such as: > > (gdb) dprintf file.c:1234, "I'm here\n" > > the output won't show up on the terminal. He can always stop the > program and manually call fflush from gdb. Not very practical for > debugging. > > He could also call setlinebuf(stdout) manually. However, this can't be > done before the inferior is started, and it's possible for the inferior > to change it back. He could also write a wrapper to printf that calls > fflush after printing. In both cases you need some quite good insight > to guess why your dprintfs are not showing up. I'd rather have something > that "just works". > > The parameter dprintf-flush-function is meant to refer to a function > called after the dprintf-function was called, in order to do the > flushing. By default, the function is set to fflush and is passed > stdout, which makes sense since the default value of the dprintf-function > (printf) prints on stdout. If dprintf-channel is set, it passes that > value instead to the function. > > A test is included. It doesn't do much, it just verifies that the > specified flush function indeed gets called. I also updated dprintf.exp > to test the error message when dprintf-function is set to an empty > value. This probably ends up making dprintf-call about 2x slower, as now you we do twice the infcalls. But I can't really argue against making it Just Work OOTB. Just a few comments below. > > gdb/ChangeLog: > > PR breakpoints/18275 > * breakpoint.c (update_dprintf_command_list): Add call to the > flush function to the command list, factor out code. > (_initialize_breakpoint): Register set/show > dprintf-flush-function. > (dprintf_flush_function): New global. > (get_dprintf_call_print_line): New function. > (get_dprintf_call_flush_line): New function. > * NEWS: Mention set/show dprintf-flush-function. > > gdb/testsuite/ChangeLog: > > PR breakpoints/18275 > * gdb.base/dprintf-flush.exp: New file. > * gdb.base/dprintf-flush.c: New file. > * gdb.base/dprintf.exp: Test error message when dprintf-function > is empty. > > gdb/doc/ChangeLog: > > PR breakpoints/18275 > * gdb.texinfo (Dynamic Printf): Document set > dprintf-flush-function. > > Change-Id: Ia47bdd7ba1e684dcbde2eddfb1d05d1a2df03316 > --- > gdb/NEWS | 4 ++ > gdb/breakpoint.c | 113 +++++++++++++++++++++++++++---- > gdb/doc/gdb.texinfo | 11 +++ > gdb/testsuite/gdb.base/dprintf-flush.c | 58 ++++++++++++++++ > gdb/testsuite/gdb.base/dprintf-flush.exp | 56 +++++++++++++++ > gdb/testsuite/gdb.base/dprintf.exp | 7 ++ > 6 files changed, 237 insertions(+), 12 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/dprintf-flush.c > create mode 100644 gdb/testsuite/gdb.base/dprintf-flush.exp > > diff --git a/gdb/NEWS b/gdb/NEWS > index 0cf51e1..b65d669 100644 > --- a/gdb/NEWS > +++ b/gdb/NEWS > @@ -36,6 +36,10 @@ set remote multiprocess-extensions-packet > show remote multiprocess-extensions-packet > Set/show the use of the remote protocol multiprocess extensions. > > +set dprintf-flush-function > +show dprintf-flush-function > + Set/show the function to use for flushing after a "call" dynamic printf. > + > * The "disassemble" command accepts a new modifier: /s. > It prints mixed source+disassembly like /m with two differences: > - disassembled instructions are now printed in program order, and > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 5067222..1dc7dd5 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -345,6 +345,11 @@ static const char *dprintf_style = dprintf_style_gdb; > > static char *dprintf_function = ""; > > +/* The function to use for flushing output after a dynamic printf when > + dprintf-style is "call". It works the same way as dprintf_function. */ > + > +static char *dprintf_flush_function = ""; > + > /* The channel to use for dynamic printf if the preferred style is to > call into the inferior; if a nonempty string, it will be passed to > the call as the first argument, with the format string as the > @@ -9024,6 +9029,76 @@ bp_loc_is_permanent (struct bp_location *loc) > return retval; > } > > +/* Return an allocated string with the command line for printing a > + "dprintf-style call" dprintf. */ > + > +static char * > +get_dprintf_call_print_line (char *dprintf_function, > + char *dprintf_channel, > + char *dprintf_args) > +{ > + char *arg, *printf_line; > + > + gdb_assert (strcmp (dprintf_style, dprintf_style_call) == 0); > + gdb_assert (dprintf_args != NULL); > + > + if (dprintf_function == NULL || strlen (dprintf_function) == 0) Write (*dprintf_function == '\0') for empty string tests. > + { > + error (_("No function supplied for dprintf call")); > + } Unnecessary braces. > + > + if (dprintf_channel != NULL && strlen (dprintf_channel) > 0) As above. > + { > + printf_line = xstrprintf ("call (void) %s (%s,%s)", > + dprintf_function, > + dprintf_channel, > + dprintf_args); > + } > + else > + { > + printf_line = xstrprintf ("call (void) %s (%s)", > + dprintf_function, > + dprintf_args); > + } > + > + return printf_line; > +} > + > +/* Return an allocated string with the command line for flushing the output > + after a "dprintf-style call" dprintf. This function returns NULL if > + dprintf_flush_function is NULL or empty. */ > + > +static char * > +get_dprintf_call_flush_line (char *dprintf_flush_function, > + char *dprintf_channel) > +{ > + char *arg, *flush_line; > + > + gdb_assert (strcmp (dprintf_style, dprintf_style_call) == 0); > + > + if (dprintf_flush_function == NULL || strlen (dprintf_flush_function) == 0) > + { > + return NULL; > + } > + > + if (dprintf_channel != NULL && strlen (dprintf_channel) > 0) > + { > + arg = dprintf_channel; > + } > + else > + { > + /* On a 64-bits platform (e.g. x86-64), if we only have minimal > + symbols for stdout, calling fflush (stdout) won't work. > + stdout will be treated as a 4-bytes integer and only 4 bytes will > + be passed to fflush, when in reality it's an 8-bytes pointer. > + Taking the address, casting to void** and dereferencing allows to > + make sure that we go read and pass the full pointer. */ > + arg = "*((void **) &stdout)"; Urgh. Note this won't work everywhere -- you can't portably take the address of stdout, which according to the C standard is a macro. I believe this won't work on mingw, for example. If we do this, it should probably end up being a gdbarch variable/method. > + } > + > + return xstrprintf ("call (void) %s (%s)", dprintf_flush_function, arg); > +} > + > /* Build a command list for the dprintf corresponding to the current > settings of the dprintf style options. */ > > @@ -9032,6 +9107,7 @@ update_dprintf_command_list (struct breakpoint *b) > { > char *dprintf_args = b->extra_string; > char *printf_line = NULL; > + char *flush_line = NULL; > > if (!dprintf_args) > return; > @@ -9051,18 +9127,10 @@ update_dprintf_command_list (struct breakpoint *b) > printf_line = xstrprintf ("printf %s", dprintf_args); > else if (strcmp (dprintf_style, dprintf_style_call) == 0) > { > - if (!dprintf_function) > - error (_("No function supplied for dprintf call")); > - > - if (dprintf_channel && strlen (dprintf_channel) > 0) > - printf_line = xstrprintf ("call (void) %s (%s,%s)", > - dprintf_function, > - dprintf_channel, > - dprintf_args); > - else > - printf_line = xstrprintf ("call (void) %s (%s)", > - dprintf_function, > - dprintf_args); > + printf_line = get_dprintf_call_print_line (dprintf_function, > + dprintf_channel, dprintf_args); > + flush_line = get_dprintf_call_flush_line (dprintf_flush_function, > + dprintf_channel); > } > else if (strcmp (dprintf_style, dprintf_style_agent) == 0) > { > @@ -9089,6 +9157,19 @@ update_dprintf_command_list (struct breakpoint *b) > printf_cmd_line->next = NULL; > printf_cmd_line->line = printf_line; > > + if (flush_line) if (flush_line != NULL) > + { > + struct command_line *flush_cmd_line = XNEW (struct command_line); > + > + flush_cmd_line->control_type = simple_control; > + flush_cmd_line->body_count = 0; > + flush_cmd_line->body_list = NULL; > + flush_cmd_line->next = NULL; > + flush_cmd_line->line = flush_line; > + > + printf_cmd_line->next = flush_cmd_line; > + } > + > breakpoint_set_commands (b, printf_cmd_line); > } > } > @@ -16440,6 +16521,14 @@ Show the function to use for dynamic printf"), NULL, > update_dprintf_commands, NULL, > &setlist, &showlist); > > + dprintf_flush_function = xstrdup ("fflush"); > + add_setshow_string_cmd ("dprintf-flush-function", class_support, > + &dprintf_flush_function, _("\ > +Set the function to use for flushing after a \"call\" dynamic printf"), _("\ > +Show the function to use for flushing after a \"call\" dynamic printf"), NULL, > + update_dprintf_commands, NULL, > + &setlist, &showlist); > + > dprintf_channel = xstrdup (""); > add_setshow_string_cmd ("dprintf-channel", class_support, > &dprintf_channel, _("\ > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index cd0abad..2a8eca5 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -4886,6 +4886,17 @@ default its value is @code{printf}. You may set it to any expression. > that @value{GDBN} can evaluate to a function, as per the @code{call} > command. > > +@item set dprintf-flush-function @var{function} > +Set the function to call after @code{dprintf-function} if the dprintf > +style is @code{call}. This function can be used to force the flushing > +of the channel used by the dprintf mechanism, so that debugging > +statements are not buffered. Setting @code{dprintf-flush-function} to > +an empty value disables the feature. By default its value is @code{fflush}. > + > +The @code{dprintf-flush-function} must accept a single argument. If > +@code{dprintf-channel} is set, its value is passed as the argument. > +Otherwise, @code{stdout} is used. > + > @item set dprintf-channel @var{channel} > Set a ``channel'' for dprintf. If set to a non-empty value, > @value{GDBN} will evaluate it as an expression and pass the result as > diff --git a/gdb/testsuite/gdb.base/dprintf-flush.c b/gdb/testsuite/gdb.base/dprintf-flush.c > new file mode 100644 > index 0000000..409b412 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/dprintf-flush.c > @@ -0,0 +1,58 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2015 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#include > + > +static int myflush_called = 0; > +static FILE *myflush_arg = NULL; > + > +static void myflush (FILE *f) Formatting. > +{ > + myflush_called++; > + myflush_arg = f; > + fflush (f); > +} > + > +int > +main (int argc, char *argv[]) > +{ > + volatile int a = 0; > + int i; > + > + for (i = 0; i < 10; i++) > + { > + a++; /* dprintf here */ > + a++; /* breakpoint here */ > + } > + > + return a; > +} > + > +#include Includes at the top. > +/* Make sure function 'malloc' is linked into program. One some bare-metal "On" > + port, if we don't use 'malloc', it will not be linked in program. 'malloc' "ports". > + is needed, otherwise we'll see such error message > + > + evaluation of this expression requires the program to have a function > + "malloc". */ I can't seem to parse this sentence. > +void > +bar (void) > +{ > + void *p = malloc (16); > + > + free (p); > +} Thanks, Pedro Alves