Re: [PATCH] Unbuffer stdout and stderr on windows

[Pedro Alves <palves@redhat.com>]

On 08/23/2013 03:19 AM, Yao Qi wrote:
> 2013-08-23  Yao Qi  <yao@codesourcery.com>
> 
> 	* main.c (captured_main) [__MINGW32__]: Set stderr unbuffered.
> 	and all update_stderr_ui_file.
> 	* ui-file.c (stderr_file_write): New function.
> 	(stderr_file_fputs): New function.
> 	(update_stderr_ui_file): New function.
> 	* ui-file.h (update_stderr_ui_file): Declare.
> ---
>  gdb/main.c    |   20 ++++++++++++++++++++
>  gdb/ui-file.c |   32 ++++++++++++++++++++++++++++++++
>  gdb/ui-file.h |    3 +++
>  3 files changed, 55 insertions(+), 0 deletions(-)
> 
> diff --git a/gdb/main.c b/gdb/main.c
> index 1c240e4..332df9e 100644
> --- a/gdb/main.c
> +++ b/gdb/main.c
> @@ -375,8 +375,28 @@ captured_main (void *data)
>    saved_command_line[0] = '\0';
>    instream = stdin;
>  
> +#ifdef __MINGW32__
> +  /* Ensure stderr is unbuffered.  A Cygwin pty or pipe is implemented
> +     as a Windows pipe, and Windows buffers on pipes.  */
> +  setvbuf (stderr, NULL, _IONBF, BUFSIZ);
> +#endif
> +
>    gdb_stdout = stdio_fileopen (stdout);
>    gdb_stderr = stdio_fileopen (stderr);
> +#ifdef __MINGW32__
> +  /* There is no real line-buffering on Windows, see
> +     http://msdn.microsoft.com/en-us/library/86cebhfs%28v=vs.71%29.aspx
> +     so the stdout is either fully-buffered or non-buffered.  We can't
> +     make stdout non-buffered, because of two concerns,
> +     1.  non-buffering hurts performance,
> +     2.  non-buffering may change GDB's behavior when it is interacting
> +     with front-end, such as Emacs.
> +
> +     We decided to leave stdout as fully buffered, but flush it first
> +     when something is written to stderr.  */
> +  update_stderr_ui_file (gdb_stderr);
> +#endif
> +
>    gdb_stdlog = gdb_stderr;	/* for moment */
>    gdb_stdtarg = gdb_stderr;	/* for moment */
>    gdb_stdin = stdio_fileopen (stdin);
> diff --git a/gdb/ui-file.c b/gdb/ui-file.c
> index cf5a86d..4b26b78 100644
> --- a/gdb/ui-file.c
> +++ b/gdb/ui-file.c
> @@ -654,6 +654,38 @@ stdio_file_fseek (struct ui_file *file, long offset, int whence)
>    return fseek (stdio->file, offset, whence);
>  }
>  
> +/* This is the implementation of ui_file method to_write for stderr.
> +   gdb_stdout is flushed before writing to gdb_stderr.  */
> +
> +static void
> +stderr_file_write (struct ui_file *file, const char *buf, long length_buf)
> +{
> +  gdb_flush (gdb_stdout);
> +  stdio_file_write (file, buf, length_buf);
> +}
> +
> +/* This is the implementation of ui_file method to_fputs for stderr.
> +   gdb_stdout is flushed before writing to gdb_stderr.  */
> +
> +static void
> +stderr_file_fputs (const char *linebuffer, struct ui_file *file)
> +{
> +  gdb_flush (gdb_stdout);
> +  stdio_file_fputs (linebuffer, file);
> +}
> +
> +/* Overwrite UI_FILE's methods 'to_write' and 'to_fputs' by function
> +   in which gdb_stdout is flushed.  */
> +
> +void
> +update_stderr_ui_file (struct ui_file *ui_file)
> +{
> +  /* Method 'to_write_async_safe' is not overwritten, because it is
> +     only used on linux host.  */

Better say that "it is not used on Windows hosts".  But I think
it'd be better even to say:

  /* Method 'to_write_async_safe' is not overwritten, because
     there's no way to flushing a stream in an async-safe manner.
     Fortunately, it doesn't really matter, because:
       - that method is only used for printing internal debug output
	 from signal handlers.
       - Windows hosts don't have a concept of async-safeness.  Signal
	 handlers run in a separate thread, so they can call
         the regular non-async-safe output routines freely.  */

I just noticed there's another place that creates gdb_stderr from
stderr in event-top.c:gdb_setup_readline.  That's called from
several interpreter_resume methods.  Though, I don't really understand
why that isn't constantly leaking gdb_stderr objects, cause it just
looks like it does.  Ah, gdb_disable_readline.  Urgh.:

/* Disable command input through the standard CLI channels.  Used in
   the suspend proc for interpreters that use the standard gdb readline
   interface, like the cli & the mi.  */
void
gdb_disable_readline (void)
{
  /* FIXME - It is too heavyweight to delete and remake these every
     time you run an interpreter that needs readline.  It is probably
     better to have the interpreters cache these, which in turn means
     that this needs to be moved into interpreter specific code.  */

#if 0
  ui_file_delete (gdb_stdout);
  ui_file_delete (gdb_stderr);
  gdb_stdlog = NULL;
  gdb_stdtarg = NULL;
  gdb_stdtargerr = NULL;
#endif

  rl_callback_handler_remove ();
  delete_file_handler (input_fd);
}

This means that after an "interpreter-exec" sequence, the
update_stderr_ui_file hack is lost...

Even if that were fixed, thinking ahead in terms of C++, I think it'd
be better to not expose to common code outside ui-file.c that you can
tweak ui_file's virtual methods at runtime like that, but instead to
hide that in ui-file.c itself.  So e.g., we'd have a new stderr_fileopen
method, and then both places that create gdb_stderr would use it, like:

-gdb_stderr = stdio_fileopen (stderr);
+gdb_stderr = stderr_fileopen ();

We could also that whole set of Windows-specific comments there
too.

-- 
Pedro Alves