Re: [PATCH 1/4 v8] gdb: Buffer output streams during events that might download debuginfo

[Andrew Burgess <aburgess@redhat.com>]

Aaron Merey <amerey@redhat.com> writes:

> v8 fixes a few style issues suggested by Thiago:
> https://sourceware.org/pipermail/gdb-patches/2023-December/205504.html
>
> Introduce new ui_file buffering_file to temporarily collect output
> written to gdb_std* output streams during print_thread, print_frame_info
> and print_stop_event.
>
> This ensures that output during these functions is not interrupted
> by debuginfod progress messages.
>
> With the addition of deferred debuginfo downloading it is possible
> for download progress messages to print during these events.
> Without any intervention we can end up with poorly formatted output:
>
>     (gdb) backtrace
>     [...]
>     #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (Downloading separate debug info for /lib64/libpython3.11.so.1.0
>     function_cache=0x561221b224d0, state=<optimized out>...
>
> To fix this we buffer writes to gdb_std* output streams while allowing
> debuginfod progress messages to skip the buffers and print to the
> underlying output streams immediately.  Buffered output is then written
> to the output streams.  This ensures that progress messages print first,
> followed by uninterrupted frame/thread/stop info:
>
>     (gdb) backtrace
>     [...]
>     Downloading separate debug info for /lib64/libpython3.11.so.1.0
>     #8  0x00007fbe8af7d7cf in pygi_invoke_c_callable (function_cache=0x561221b224d0, state=<optimized out>...
>
> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
> ---
>  gdb/cli-out.c            |  10 ++-
>  gdb/cli-out.h            |   3 +
>  gdb/debuginfod-support.c |  15 ++--
>  gdb/infrun.c             |  16 +++-
>  gdb/mi/mi-out.h          |   3 +
>  gdb/python/py-uiout.h    |   3 +
>  gdb/stack.c              |  35 +++++---
>  gdb/thread.c             | 180 ++++++++++++++++++++++----------------
>  gdb/ui-file.h            |   2 +-
>  gdb/ui-out.c             | 141 ++++++++++++++++++++++++++++++
>  gdb/ui-out.h             | 184 +++++++++++++++++++++++++++++++++++++++
>  11 files changed, 493 insertions(+), 99 deletions(-)
>
> diff --git a/gdb/cli-out.c b/gdb/cli-out.c
> index a1cde54feda..b4c7a657423 100644
> --- a/gdb/cli-out.c
> +++ b/gdb/cli-out.c
> @@ -299,7 +299,7 @@ cli_ui_out::do_progress_notify (const std::string &msg,
>  				double howmuch, double total)
>  {
>    int chars_per_line = get_chars_per_line ();
> -  struct ui_file *stream = m_streams.back ();
> +  struct ui_file *stream = get_unbuffered (m_streams.back ());
>    cli_progress_info &info (m_progress_info.back ());
>  
>    if (chars_per_line > MAX_CHARS_PER_LINE)
> @@ -384,7 +384,7 @@ cli_ui_out::do_progress_notify (const std::string &msg,
>  void
>  cli_ui_out::clear_progress_notify ()
>  {
> -  struct ui_file *stream = m_streams.back ();
> +  struct ui_file *stream = get_unbuffered (m_streams.back ());
>    int chars_per_line = get_chars_per_line ();
>  
>    scoped_restore save_pagination
> @@ -413,10 +413,12 @@ void
>  cli_ui_out::do_progress_end ()
>  {
>    struct ui_file *stream = m_streams.back ();
> -  m_progress_info.pop_back ();
> +  cli_progress_info &info (m_progress_info.back ());
>  
> -  if (stream->isatty ())
> +  if (stream->isatty () && info.state != progress_update::START)
>      clear_progress_notify ();
> +
> +  m_progress_info.pop_back ();
>  }
>  
>  /* local functions */
> diff --git a/gdb/cli-out.h b/gdb/cli-out.h
> index 34016182269..89b4aa40870 100644
> --- a/gdb/cli-out.h
> +++ b/gdb/cli-out.h
> @@ -35,6 +35,9 @@ class cli_ui_out : public ui_out
>  
>    bool can_emit_style_escape () const override;
>  
> +  ui_file *current_stream () const override
> +  { return m_streams.back (); }
> +
>  protected:
>  
>    virtual void do_table_begin (int nbrofcols, int nr_rows,
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index eb88c406ad6..35dcf56cb7e 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -155,7 +155,8 @@ progressfn (debuginfod_client *c, long cur, long total)
>  
>    if (check_quit_flag ())
>      {
> -      gdb_printf ("Cancelling download of %s %s...\n",
> +      ui_file *outstream = get_unbuffered (gdb_stdout);
> +      gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
>  		  data->desc, styled_fname.c_str ());
>        return 1;
>      }
> @@ -295,10 +296,14 @@ static void
>  print_outcome (int fd, const char *desc, const char *fname)
>  {
>    if (fd < 0 && fd != -ENOENT)
> -    gdb_printf (_("Download failed: %s.  Continuing without %s %ps.\n"),
> -		safe_strerror (-fd),
> -		desc,
> -		styled_string (file_name_style.style (), fname));
> +    {
> +      ui_file *outstream = get_unbuffered (gdb_stdout);
> +      gdb_printf (outstream,
> +		  _("Download failed: %s.  Continuing without %s %ps.\n"),
> +		  safe_strerror (-fd),
> +		  desc,
> +		  styled_string (file_name_style.style (), fname));
> +    }
>  }
>  
>  /* See debuginfod-support.h  */
> diff --git a/gdb/infrun.c b/gdb/infrun.c
> index c769e972f49..b977ba815ff 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -9277,10 +9277,10 @@ print_stop_location (const target_waitstatus &ws)
>      print_stack_frame (get_selected_frame (nullptr), 0, source_flag, 1);
>  }
>  
> -/* See infrun.h.  */
> +/* See `print_stop_event` in infrun.h.  */
>  
> -void
> -print_stop_event (struct ui_out *uiout, bool displays)
> +static void
> +do_print_stop_event (struct ui_out *uiout, bool displays)
>  {
>    struct target_waitstatus last;
>    struct thread_info *tp;
> @@ -9309,6 +9309,16 @@ print_stop_event (struct ui_out *uiout, bool displays)
>      }
>  }
>  
> +/* See infrun.h.  This function itself sets up buffered output for the
> +   duration of do_print_stop_event, which performs the actual event
> +   printing.  */
> +
> +void
> +print_stop_event (struct ui_out *uiout, bool displays)
> +{
> +  do_with_buffered_output (do_print_stop_event, uiout, displays);
> +}
> +
>  /* See infrun.h.  */
>  
>  void
> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
> index 0dd7479a52f..68ff5faf632 100644
> --- a/gdb/mi/mi-out.h
> +++ b/gdb/mi/mi-out.h
> @@ -45,6 +45,9 @@ class mi_ui_out : public ui_out
>      return false;
>    }
>  
> +  ui_file *current_stream () const override
> +  { return m_streams.back (); }
> +
>  protected:
>  
>    virtual void do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
> diff --git a/gdb/python/py-uiout.h b/gdb/python/py-uiout.h
> index 9c9617ea831..5944d1b6e34 100644
> --- a/gdb/python/py-uiout.h
> +++ b/gdb/python/py-uiout.h
> @@ -56,6 +56,9 @@ class py_ui_out : public ui_out
>      return std::move (current ().obj);
>    }
>  
> +  ui_file *current_stream () const override
> +  { return nullptr; }
> +
>  protected:
>  
>    void do_progress_end () override { }
> diff --git a/gdb/stack.c b/gdb/stack.c
> index fad4b62c6f7..42e919e1078 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -220,7 +220,8 @@ static void print_frame_local_vars (frame_info_ptr frame,
>  				    const char *regexp, const char *t_regexp,
>  				    int num_tabs, struct ui_file *stream);
>  
> -static void print_frame (const frame_print_options &opts,
> +static void print_frame (struct ui_out *uiout,
> +			 const frame_print_options &opts,
>  			 frame_info_ptr frame, int print_level,
>  			 enum print_what print_what,  int print_args,
>  			 struct symtab_and_line sal);
> @@ -1020,16 +1021,15 @@ get_user_print_what_frame_info (std::optional<enum print_what> *what)
>     Used in "where" output, and to emit breakpoint or step
>     messages.  */
>  
> -void
> -print_frame_info (const frame_print_options &fp_opts,
> -		  frame_info_ptr frame, int print_level,
> -		  enum print_what print_what, int print_args,
> -		  int set_current_sal)
> +static void
> +do_print_frame_info (struct ui_out *uiout, const frame_print_options &fp_opts,
> +		     frame_info_ptr frame, int print_level,
> +		     enum print_what print_what, int print_args,
> +		     int set_current_sal)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
>    int source_print;
>    int location_print;
> -  struct ui_out *uiout = current_uiout;
>  
>    if (!current_uiout->is_mi_like_p ()
>        && fp_opts.print_frame_info != print_frame_info_auto)
> @@ -1105,7 +1105,8 @@ print_frame_info (const frame_print_options &fp_opts,
>  		    || print_what == LOC_AND_ADDRESS
>  		    || print_what == SHORT_LOCATION);
>    if (location_print || !sal.symtab)
> -    print_frame (fp_opts, frame, print_level, print_what, print_args, sal);
> +    print_frame (uiout, fp_opts, frame, print_level,
> +		 print_what, print_args, sal);
>  
>    source_print = (print_what == SRC_LINE || print_what == SRC_AND_LOC);
>  
> @@ -1185,6 +1186,20 @@ print_frame_info (const frame_print_options &fp_opts,
>    gdb_flush (gdb_stdout);
>  }
>  
> +/* Redirect output to a temporary buffer for the duration
> +   of do_print_frame_info.  */
> +
> +void
> +print_frame_info (const frame_print_options &fp_opts,
> +		  frame_info_ptr frame, int print_level,
> +		  enum print_what print_what, int print_args,
> +		  int set_current_sal)
> +{
> +  do_with_buffered_output (do_print_frame_info, current_uiout,
> +			   fp_opts, frame, print_level, print_what,
> +			   print_args, set_current_sal);
> +}
> +
>  /* See stack.h.  */
>  
>  void
> @@ -1309,13 +1324,13 @@ find_frame_funname (frame_info_ptr frame, enum language *funlang,
>  }
>  
>  static void
> -print_frame (const frame_print_options &fp_opts,
> +print_frame (struct ui_out *uiout,
> +	     const frame_print_options &fp_opts,
>  	     frame_info_ptr frame, int print_level,
>  	     enum print_what print_what, int print_args,
>  	     struct symtab_and_line sal)
>  {
>    struct gdbarch *gdbarch = get_frame_arch (frame);
> -  struct ui_out *uiout = current_uiout;
>    enum language funlang = language_unknown;
>    struct value_print_options opts;
>    struct symbol *func;
> diff --git a/gdb/thread.c b/gdb/thread.c
> index f03a60c4d01..08c156d6687 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1089,6 +1089,107 @@ thread_target_id_str (thread_info *tp)
>      return target_id;
>  }
>  
> +/* Print thread TP.  GLOBAL_IDS indicates whether REQUESTED_THREADS
> +   is a list of global or per-inferior thread ids.  */
> +
> +static void
> +do_print_thread (ui_out *uiout, const char *requested_threads,
> +		 int global_ids, int pid, int show_global_ids,
> +		 int default_inf_num, thread_info *tp,
> +		 thread_info *current_thread)
> +{
> +  int core;
> +
> +  /* In case REQUESTED_THREADS contains $_thread.  */
> +  if (current_thread != nullptr)
> +    switch_to_thread (current_thread);
> +
> +  if (!should_print_thread (requested_threads, default_inf_num,
> +			    global_ids, pid, tp))
> +    return;
> +
> +  ui_out_emit_tuple tuple_emitter (uiout, NULL);
> +
> +  if (!uiout->is_mi_like_p ())
> +    {
> +      if (tp == current_thread)
> +	uiout->field_string ("current", "*");
> +      else
> +	uiout->field_skip ("current");
> +
> +      uiout->field_string ("id-in-tg", print_thread_id (tp));
> +    }
> +
> +  if (show_global_ids || uiout->is_mi_like_p ())
> +    uiout->field_signed ("id", tp->global_num);
> +
> +  /* Switch to the thread (and inferior / target).  */
> +  switch_to_thread (tp);
> +
> +  /* For the CLI, we stuff everything into the target-id field.
> +     This is a gross hack to make the output come out looking
> +     correct.  The underlying problem here is that ui-out has no
> +     way to specify that a field's space allocation should be
> +     shared by several fields.  For MI, we do the right thing
> +     instead.  */
> +
> +  if (uiout->is_mi_like_p ())
> +    {
> +      uiout->field_string ("target-id", target_pid_to_str (tp->ptid));
> +
> +      const char *extra_info = target_extra_thread_info (tp);
> +      if (extra_info != nullptr)
> +	uiout->field_string ("details", extra_info);
> +
> +      const char *name = thread_name (tp);
> +      if (name != NULL)
> +	uiout->field_string ("name", name);
> +    }
> +  else
> +    {
> +      uiout->field_string ("target-id", thread_target_id_str (tp));
> +    }
> +
> +  if (tp->state == THREAD_RUNNING)
> +    uiout->text ("(running)\n");
> +  else
> +    {
> +      /* The switch above put us at the top of the stack (leaf
> +	 frame).  */
> +      print_stack_frame (get_selected_frame (NULL),
> +			 /* For MI output, print frame level.  */
> +			 uiout->is_mi_like_p (),
> +			 LOCATION, 0);
> +    }
> +
> +  if (uiout->is_mi_like_p ())
> +    {
> +      const char *state = "stopped";
> +
> +      if (tp->state == THREAD_RUNNING)
> +	state = "running";
> +      uiout->field_string ("state", state);
> +    }
> +
> +  core = target_core_of_thread (tp->ptid);
> +  if (uiout->is_mi_like_p () && core != -1)
> +    uiout->field_signed ("core", core);
> +}
> +
> +/* Redirect output to a temporary buffer for the duration
> +   of do_print_thread.  */
> +
> +static void
> +print_thread (ui_out *uiout, const char *requested_threads,
> +	      int global_ids, int pid, int show_global_ids,
> +	      int default_inf_num, thread_info *tp, thread_info *current_thread)
> +
> +{
> +  do_with_buffered_output (do_print_thread, uiout, requested_threads,
> +			   global_ids, pid, show_global_ids,
> +			   default_inf_num, tp, current_thread);
> +}
> +
>  /* Like print_thread_info, but in addition, GLOBAL_IDS indicates
>     whether REQUESTED_THREADS is a list of global or per-inferior
>     thread ids.  */
> @@ -1176,86 +1277,13 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
>      for (inferior *inf : all_inferiors ())
>        for (thread_info *tp : inf->threads ())
>  	{
> -	  int core;
> -
>  	  any_thread = true;
> +
>  	  if (tp == current_thread && tp->state == THREAD_EXITED)
>  	    current_exited = true;
>  
> -	  /* In case REQUESTED_THREADS contains $_thread.  */
> -	  if (current_thread != nullptr)
> -	    switch_to_thread (current_thread);
> -
> -	  if (!should_print_thread (requested_threads, default_inf_num,
> -				    global_ids, pid, tp))
> -	    continue;
> -
> -	  ui_out_emit_tuple tuple_emitter (uiout, NULL);
> -
> -	  if (!uiout->is_mi_like_p ())
> -	    {
> -	      if (tp == current_thread)
> -		uiout->field_string ("current", "*");
> -	      else
> -		uiout->field_skip ("current");
> -
> -	      uiout->field_string ("id-in-tg", print_thread_id (tp));
> -	    }
> -
> -	  if (show_global_ids || uiout->is_mi_like_p ())
> -	    uiout->field_signed ("id", tp->global_num);
> -
> -	  /* Switch to the thread (and inferior / target).  */
> -	  switch_to_thread (tp);
> -
> -	  /* For the CLI, we stuff everything into the target-id field.
> -	     This is a gross hack to make the output come out looking
> -	     correct.  The underlying problem here is that ui-out has no
> -	     way to specify that a field's space allocation should be
> -	     shared by several fields.  For MI, we do the right thing
> -	     instead.  */
> -
> -	  if (uiout->is_mi_like_p ())
> -	    {
> -	      uiout->field_string ("target-id", target_pid_to_str (tp->ptid));
> -
> -	      const char *extra_info = target_extra_thread_info (tp);
> -	      if (extra_info != nullptr)
> -		uiout->field_string ("details", extra_info);
> -
> -	      const char *name = thread_name (tp);
> -	      if (name != NULL)
> -		uiout->field_string ("name", name);
> -	    }
> -	  else
> -	    {
> -	      uiout->field_string ("target-id", thread_target_id_str (tp));
> -	    }
> -
> -	  if (tp->state == THREAD_RUNNING)
> -	    uiout->text ("(running)\n");
> -	  else
> -	    {
> -	      /* The switch above put us at the top of the stack (leaf
> -		 frame).  */
> -	      print_stack_frame (get_selected_frame (NULL),
> -				 /* For MI output, print frame level.  */
> -				 uiout->is_mi_like_p (),
> -				 LOCATION, 0);
> -	    }
> -
> -	  if (uiout->is_mi_like_p ())
> -	    {
> -	      const char *state = "stopped";
> -
> -	      if (tp->state == THREAD_RUNNING)
> -		state = "running";
> -	      uiout->field_string ("state", state);
> -	    }
> -
> -	  core = target_core_of_thread (tp->ptid);
> -	  if (uiout->is_mi_like_p () && core != -1)
> -	    uiout->field_signed ("core", core);
> +	  print_thread (uiout, requested_threads, global_ids, pid,
> +			show_global_ids, default_inf_num, tp, current_thread);
>  	}
>  
>      /* This end scope restores the current thread and the frame
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index 31f87ffd51d..8385033b441 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -224,7 +224,7 @@ class string_file : public ui_file
>    bool empty () const { return m_string.empty (); }
>    void clear () { return m_string.clear (); }
>  
> -private:
> +protected:

Is this needed?  Everything seems to compile fine if this is left as
private.

>    /* The internal buffer.  */
>    std::string m_string;
>  
> diff --git a/gdb/ui-out.c b/gdb/ui-out.c
> index e8653c6c127..75f9f1612cb 100644
> --- a/gdb/ui-out.c
> +++ b/gdb/ui-out.c
> @@ -871,3 +871,144 @@ ui_out::ui_out (ui_out_flags flags)
>  ui_out::~ui_out ()
>  {
>  }
> +
> +/* See ui-out.h.  */
> +
> +void
> +buffer_group::output_unit::flush () const
> +{
> +  if (!m_msg.empty ())
> +    m_stream->puts (m_msg.c_str ());
> +
> +  if (m_wrap_hint >= 0)
> +    m_stream->wrap_here (m_wrap_hint);
> +
> +  if (m_flush)
> +    m_stream->flush ();
> +}
> +
> +/* See ui-out.h.  */
> +
> +void
> +buffer_group::write (const char *buf, long length_buf, ui_file *stream)
> +{
> +  /* Record each line separately.  */
> +  for (size_t prev = 0, cur = 0; cur < length_buf; ++cur)
> +    if (buf[cur] == '\n' || cur == length_buf - 1)
> +      {
> +	std::string msg (buf + prev, cur - prev + 1);
> +
> +	if (m_buffered_output.size () > 0
> +	    && m_buffered_output.back ().m_wrap_hint == -1
> +	    && m_buffered_output.back ().m_stream == stream
> +	    && m_buffered_output.back ().m_msg.size () > 0
> +	    && m_buffered_output.back ().m_msg.back () != '\n')
> +	  m_buffered_output.back ().m_msg.append (msg);
> +	else
> +	  {
> +	    m_buffered_output.emplace_back (msg);
> +	    m_buffered_output.back ().m_stream = stream;
> +	  }
> +	prev = cur + 1;
> +      }
> +}
> +
> +/* See ui-out.h.  */
> +
> +void
> +buffer_group::wrap_here (int indent, ui_file *stream)
> +{
> +  m_buffered_output.emplace_back ("", indent);
> +  m_buffered_output.back ().m_stream = stream;
> +}
> +
> +/* See ui-out.h.  */
> +
> +void
> +buffer_group::flush_here (ui_file *stream)
> +{
> +  m_buffered_output.emplace_back ("", -1, true);
> +  m_buffered_output.back ().m_stream = stream;
> +}
> +
> +/* See ui-out.h.  */
> +
> +ui_file *
> +get_unbuffered (ui_file * stream)

Should be 'ui_file *stream'.

> +{
> +  buffering_file *buf = dynamic_cast<buffering_file *> (stream);
> +
> +  if (buf == nullptr)
> +    return stream;
> +
> +  return get_unbuffered (buf->stream ());
> +}
> +
> +buffered_streams::buffered_streams (buffer_group *group, ui_out *uiout)
> +  : m_buffered_stdout (group, gdb_stdout),
> +    m_buffered_stderr (group, gdb_stderr),
> +    m_buffered_stdlog (group, gdb_stdlog),
> +    m_buffered_stdtarg (group, gdb_stdtarg),
> +    m_buffered_stdtargerr (group, gdb_stdtargerr),
> +    m_uiout (uiout)
> +{
> +  gdb_stdout = &m_buffered_stdout;
> +  gdb_stderr = &m_buffered_stderr;
> +  gdb_stdlog = &m_buffered_stdlog;
> +  gdb_stdtarg = &m_buffered_stdtarg;
> +  gdb_stdtargerr = &m_buffered_stdtargerr;
> +
> +  ui_file *stream = current_uiout->current_stream ();
> +  if (stream != nullptr)
> +    {
> +      m_buffered_current_uiout.emplace (group, stream);
> +      current_uiout->redirect (&(*m_buffered_current_uiout));
> +    }
> +
> +  stream = m_uiout->current_stream ();
> +  if (stream != nullptr && current_uiout != m_uiout)
> +    {
> +      m_buffered_uiout.emplace (group, stream);
> +      m_uiout->redirect (&(*m_buffered_uiout));
> +    }
> +
> +  m_buffers_in_place = true;
> +}
> +
> +/* See ui-out.h.  */
> +
> +void
> +buffered_streams::remove_buffers ()
> +{
> +  if (!m_buffers_in_place)
> +    return;
> +
> +  m_buffers_in_place = false;
> +
> +  gdb_stdout = m_buffered_stdout.stream ();
> +  gdb_stderr = m_buffered_stderr.stream ();
> +  gdb_stdlog = m_buffered_stdlog.stream ();
> +  gdb_stdtarg = m_buffered_stdtarg.stream ();
> +  gdb_stdtargerr = m_buffered_stdtargerr.stream ();
> +
> +  if (m_buffered_current_uiout.has_value ())
> +    current_uiout->redirect (nullptr);
> +
> +  if (m_buffered_uiout.has_value ())
> +    m_uiout->redirect (nullptr);
> +}
> +
> +buffer_group::buffer_group (ui_out *uiout)
> +  : m_buffered_streams (new buffered_streams (this, uiout))
> +{ /* Nothing.  */ }
> +
> +/* See ui-out.h.  */
> +
> +void
> +buffer_group::flush () const
> +{
> +  m_buffered_streams->remove_buffers ();
> +
> +  for (const output_unit &ou : m_buffered_output)
> +    ou.flush ();
> +}
> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index 07567a1df35..2934d3a6b34 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -278,6 +278,9 @@ class ui_out
>       escapes.  */
>    virtual bool can_emit_style_escape () const = 0;
>  
> +  /* Return the ui_file currently used for output.  */
> +  virtual ui_file *current_stream () const = 0;
> +
>    /* An object that starts and finishes displaying progress updates.  */
>    class progress_update
>    {
> @@ -470,4 +473,185 @@ class ui_out_redirect_pop
>    struct ui_out *m_uiout;
>  };
>  
> +struct buffered_streams;
> +
> +/* Organizes writes to a collection of buffered output streams
> +   so that when flushed, output is written to all streams in
> +   chronological order.  */
> +
> +struct buffer_group
> +{
> +  buffer_group (ui_out *uiout);
> +
> +  /* Flush all buffered writes to the underlying output streams.  */
> +  void flush () const;
> +
> +  /* Record contents of BUF and associate it with STREAM.  */
> +  void write (const char *buf, long length_buf, ui_file *stream);
> +
> +  /* Record a wrap_here and associate it with STREAM.  */
> +  void wrap_here (int indent, ui_file *stream);
> +
> +  /* Record a call to flush and associate it with STREAM.  */
> +  void flush_here (ui_file *stream);
> +
> +private:
> +
> +  struct output_unit
> +  {
> +    output_unit (std::string msg, int wrap_hint = -1, bool flush = false)
> +      : m_msg (msg), m_wrap_hint (wrap_hint), m_flush (flush)
> +    {}
> +
> +    /* Write contents of this output_unit to the underlying stream.  */
> +    void flush () const;
> +
> +    /* Underlying stream for which this output unit will be written to.  */
> +    ui_file *m_stream;
> +
> +    /* String to be written to underlying buffer.  */
> +    std::string m_msg;
> +
> +    /* Argument to wrap_here.  -1 indicates no wrap.  Used to call wrap_here
> +       during buffer flush.  */
> +    int m_wrap_hint;
> +
> +    /* Indicate that the underlying output stream's flush should be called.  */
> +    bool m_flush;
> +  };
> +
> +  /* Output_units to be written to buffered output streams.  */
> +  std::vector<output_unit> m_buffered_output;
> +
> +  /* Buffered output streams.  */
> +  std::unique_ptr<buffered_streams> m_buffered_streams;
> +};
> +
> +/* If FILE is a buffering_file, return it's underlying stream.  */
> +
> +extern ui_file *get_unbuffered (ui_file *file);
> +
> +/* Buffer output to gdb_stdout and gdb_stderr for the duration of FUNC.  */
> +
> +template<typename F, typename... Arg>
> +void
> +do_with_buffered_output (F func, ui_out *uiout, Arg... args)
> +{
> +  buffer_group g (uiout);
> +
> +  try
> +    {
> +      func (uiout, std::forward<Arg> (args)...);
> +    }
> +  catch (gdb_exception &ex)

This should be 'const gdb_exception &ex'.

> +    {
> +      /* Ideally flush would be called in the destructor of buffer_group,
> +	 however flushing might cause an exception to be thrown.  Catch it
> +	 and ensure the first exception propagates.  */
> +      try
> +	{
> +	  g.flush ();
> +	}
> +      catch (const gdb_exception &)
> +	{
> +	}
> +
> +      throw_exception (std::move (ex));

This can be replaced with just 'throw;'.  Object slicing has occurred
when we caught the exception as a gdb_exception, but the current
exception will still be the correct, full object, so we're OK to just
rethrow that.

> +    }
> +
> +  /* Try was successful.  Let any further exceptions propagate.  */
> +  g.flush ();
> +}
> +
> +/* Accumulate writes to an underlying ui_file.  Output to the
> +   underlying file is deferred until required.  */
> +
> +struct buffering_file : public ui_file
> +{
> +  buffering_file (buffer_group *group, ui_file *stream)
> +    : m_group (group),
> +      m_stream (stream)
> +  { /* Nothing.  */ }
> +
> +  /* Return the underlying output stream.  */
> +  ui_file *stream () const
> +  {
> +    return m_stream;
> +  }
> +
> +  /* Record the contents of BUF.  */
> +  void write (const char *buf, long length_buf) override
> +  {
> +    m_group->write (buf, length_buf, m_stream);
> +  }
> +
> +  /* Record a wrap_here call with argument INDENT.  */
> +  void wrap_here (int indent) override
> +  {
> +    m_group->wrap_here (indent, m_stream);
> +  }
> +
> +  /* Return true if the underlying stream is a tty.  */
> +  bool isatty () override
> +  {
> +    return m_stream->isatty ();
> +  }
> +
> +  /* Return true if ANSI escapes can be used on the underlying stream.  */
> +  bool can_emit_style_escape () override
> +  {
> +    return m_stream->can_emit_style_escape ();
> +  }
> +
> +  /* Flush the underlying output stream.  */
> +  void flush () override
> +  {
> +    return m_group->flush_here (m_stream);
> +  }
> +
> +private:
> +
> +  /* Coordinates buffering across multiple buffering_files.  */
> +  buffer_group *m_group;
> +
> +  /* The underlying output stream.  */
> +  ui_file *m_stream;
> +};
> +
> +/* Attaches and detaches buffers for each of the gdb_std* streams.  */
> +
> +struct buffered_streams
> +{
> +  buffered_streams (buffer_group *group, ui_out *uiout);
> +
> +  ~buffered_streams ()
> +  {
> +    this->remove_buffers ();
> +  }
> +
> +  /* Remove buffering_files from all underlying streams.  */
> +  void remove_buffers ();
> +
> +private:
> +
> +  /* True if buffers are still attached to each underlying output stream.  */
> +  bool m_buffers_in_place;
> +
> +  /* Buffers for each gdb_std* output stream.  */
> +  buffering_file m_buffered_stdout;
> +  buffering_file m_buffered_stderr;
> +  buffering_file m_buffered_stdlog;
> +  buffering_file m_buffered_stdtarg;
> +  buffering_file m_buffered_stdtargerr;
> +
> +  /* Buffer for current_uiout's output stream.  */
> +  std::optional<buffering_file> m_buffered_current_uiout;
> +
> +  /* Additional ui_out being buffered.  */
> +  ui_out *m_uiout;
> +
> +  /* Buffer for m_uiout's output stream.  */
> +  std::optional<buffering_file> m_buffered_uiout;
> +};

I really dislike GDB's I/O sub-system.  Every time I look at the code it
always feels like it's far too complicated, and there must be a better
way to do things.

But I've not yet managed to come up with a plan that would actually make
things simpler.

This is adding yet more complexity on top, which isn't ideal.  But I
don't have any better suggestions, and I can see why this is needed.
And I know the actual point of this series, the debuginfod work, is
going to be super valuable.

So, given nobody who knows the I/O stuff better has commented, I propose
that, with the fixes I've pointed out above:

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> +
>  #endif /* UI_OUT_H */
> -- 
> 2.43.0