Re: [PATCH] [PR breakpoints/18275] Introduce dprintf-flush-function for call-style dprintfs

[Pedro Alves <palves at redhat dot com>]

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 <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +
> +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 <stdlib.h>

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