Re: [PATCH v6] gdb: Buffer output streams during events that might download debuginfo

[Aaron Merey <amerey@redhat.com>]

On Tue, Oct 24, 2023 at 7:23 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> Unless I'm missing something, none of this cur_prefix_state stuff can
> trigger after this commit; the cur_prefix_state starts as
> prefix_state_t::NEWLINE_OFF and can never change from that state.
>
> I know this changes after later patches in this series, but it feels
> like this work has ended up in this patch by mistake.
>
> Could we remove these changes from this commit and push them into a
> later commit in this series?

Moved to patch 4/4.

> > +void
> > +buffer_group::output_unit::flush () const
> > +{
> > +  m_stream->puts (m_msg.c_str ());
>
> Maybe we should do:
>
>   if (!m_msg.empty ())
>     m_stream->puts (m_msg.c_str ());
>
> I guess it doesn't make much difference, but when we queue up a
> wrap_here call we add an empty string.

Done.

> > +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));
> > +      }
>
> When I was thinking about this problem, I imagined replacing this
> current_uiout code with the code you've added below.  My thinking is
> that M_UIOUT is what we are about to pass to the printing routine,
> right?  So if we are actually using CURRENT_UIOUT within the printing
> routine, then something is going horribly wrong.
>
> Caching CURRENT_UIOUT feels like a work around for possibly broken code
> elsewhere in GDB.
>
> Have you tried this patch without the CURRENT_UIOUT buffering?  Only
> buffer M_UIOUT?  Hopefully that works fine, but if it throws up some
> errors, maybe we should look into those and see if we can fix them.

It does not work without buffering both CURRENT_UIOUT and M_UIOUT but
I don't think this is an indication that something has gone wrong.

Before this patch, some of the functions we now apply output buffering
to (via do_with_buffered_output) would set CURRENT_UIOUT to a local
variable and print to that CURRENT_UIOUT alias for their duration.
Other buffered functions take a uiout argument that may or may not
be the CURRENT_UIOUT.

I think the current approach of buffering both CURRENT_UIOUT
and M_UIOUT is a simple way to cover all of these cases without
having to tweak very much within the buffered functions.

>
> > +
> > +    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.  */ }
> > +
> > +buffer_group::~buffer_group ()
> > +{ /* 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..45d8007587b 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
> >    {
> > @@ -293,6 +296,21 @@ class ui_out
> >        BAR
> >      };
> >
> > +    /* Used to communicate the status of a newline prefix for the next progress
> > +       update message.  */
> > +    enum prefix_state
> > +    {
> > +      /* Do not modify the next progress update message.  */
> > +      NEWLINE_OFF,
> > +
> > +      /* The next progress update message should include a newline prefix.  */
> > +      NEWLINE_NEEDED,
> > +
> > +      /* A newline prefix was included in a debuginfod progress update
> > +      message.  */
> > +      NEWLINE_PRINTED
> > +    };
> > +
> >      /* SHOULD_PRINT indicates whether something should be printed for a tty.  */
> >      progress_update ()
> >      {
> > @@ -390,6 +408,11 @@ class ui_out
> >    ui_out_level *current_level () const;
> >  };
> >
> > +typedef ui_out::progress_update::prefix_state prefix_state_t;
> > +
> > +/* Current state of the newline prefix.  */
> > +extern prefix_state_t cur_prefix_state;
> > +
> >  /* Start a new tuple or list on construction, and end it on
> >     destruction.  Normally this is used via the typedefs
> >     ui_out_emit_tuple and ui_out_emit_list.  */
> > @@ -470,4 +493,181 @@ 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);
> > +
> > +  ~buffer_group ();
> > +
> > +  /* 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);
> > +
> > +private:
> > +
> > +  struct output_unit
> > +  {
> > +    output_unit (std::string msg, int wrap_hint = -1)
> > +      : m_msg (msg), m_wrap_hint (wrap_hint)
> > +    {}
> > +
> > +    /* 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
> > +       at appropriate times during flush.  */
> > +    int m_wrap_hint;
> > +  };
> > +
> > +  /* 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)
> > +    {
> > +      /* 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));
> > +    }
> > +
> > +  /* 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_stream->flush ();
> > +  }
>
> This doesn't seem right.  Some caller is going to perform a series of
> write, wrap_here, and flush calls, and we should be queuing them up,
> then replaying them later, right?
>
> We queue up the write and wrap_here calls, but the flush calls we send
> through immediately.
>
> I'm not sure we actually need to add the flush as a separate output_unit
> in the buffer_group though, maybe all that's needed is a single bool
> flag per-stream somewhere?  Then in buffer_group::flush, we can somehow
> arrange to call flush on those streams that have a queued flush pending?

Fixed.  I ended up recording flush with a separate output_unit because
this functionality fits well within the design of output_unit.  Plus it
ensures that the flush occurs exactly at the intended points in the output.

The remaining patches in this series have been reposted here with these
changes:
https://sourceware.org/pipermail/gdb-patches/2023-October/203589.html

Aaron