[3/6,v4] gdb: Buffer output streams during events that might download debuginfo
Commit Message
v3: https://sourceware.org/pipermail/gdb-patches/2023-June/199986.html
v4 adds output buffering for gdb_stderr. This fixes a regression
introduced by v3 where messages printed to gdb_stderr appeared out of
order with respect to buffered gdb_stdout messages.
To ensure that writes to gdb_stderr and gdb_stdout occur in the same
order that they were written to the buffers, ID numbers are assigned
to each output_unit recorded by a buffer_file. IDs are ordered and
unique across all output_units currently in use in any buffer_file.
output_units represent a string to be written to a buffer_file's
underlying stream. These strings terminate in either a newline or a
call to wrap_here.
When two buffer_files are simultaneously flushed, IDs for the next
output_units in each buffer_file are compared to determine which
should be written first.
Tests for this patch include the existing gdb.base/solib-display.exp
'after rerun' tests, in which messages are printed to both gdb_stdout
and gdb_stderr while both streams are buffered. Tests that check
the correctness of debuginfod progress messages during output stream
buffering are included in patches 5/6 and 6/6 in this series.
Commit message:
Introduce new ui_file buffer_file to temporarily collect output
written to gdb_stdout and gdb_stderr during print_thread,
print_frame_info and print_stop_event.
This ensures that output from 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 accumulate writes to gdb_stdout and gdb_stderr in
buffer_file objects. Debuginfod progress messages skip the buffers
and print to gdb_stdout immediately. This ensures 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>...
---
gdb/cli-out.c | 34 +++++++-
gdb/cli-out.h | 3 +
gdb/debuginfod-support.c | 15 ++--
gdb/infrun.c | 17 +++-
gdb/mi/mi-out.c | 37 +++++++++
gdb/mi/mi-out.h | 3 +
gdb/python/py-mi.c | 3 +
gdb/stack.c | 38 ++++++---
gdb/thread.c | 174 +++++++++++++++++++++++----------------
gdb/ui-file.c | 136 ++++++++++++++++++++++++++++++
gdb/ui-file.h | 92 ++++++++++++++++++++-
gdb/ui-out.c | 36 ++++++++
gdb/ui-out.h | 79 ++++++++++++++++++
13 files changed, 573 insertions(+), 94 deletions(-)
Comments
Ping
Thanks,
Aaron
On Thu, Jun 8, 2023 at 8:03 PM Aaron Merey <amerey@redhat.com> wrote:
>
> v3: https://sourceware.org/pipermail/gdb-patches/2023-June/199986.html
>
> v4 adds output buffering for gdb_stderr. This fixes a regression
> introduced by v3 where messages printed to gdb_stderr appeared out of
> order with respect to buffered gdb_stdout messages.
>
> To ensure that writes to gdb_stderr and gdb_stdout occur in the same
> order that they were written to the buffers, ID numbers are assigned
> to each output_unit recorded by a buffer_file. IDs are ordered and
> unique across all output_units currently in use in any buffer_file.
> output_units represent a string to be written to a buffer_file's
> underlying stream. These strings terminate in either a newline or a
> call to wrap_here.
>
> When two buffer_files are simultaneously flushed, IDs for the next
> output_units in each buffer_file are compared to determine which
> should be written first.
>
> Tests for this patch include the existing gdb.base/solib-display.exp
> 'after rerun' tests, in which messages are printed to both gdb_stdout
> and gdb_stderr while both streams are buffered. Tests that check
> the correctness of debuginfod progress messages during output stream
> buffering are included in patches 5/6 and 6/6 in this series.
>
> Commit message:
>
> Introduce new ui_file buffer_file to temporarily collect output
> written to gdb_stdout and gdb_stderr during print_thread,
> print_frame_info and print_stop_event.
>
> This ensures that output from 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 accumulate writes to gdb_stdout and gdb_stderr in
> buffer_file objects. Debuginfod progress messages skip the buffers
> and print to gdb_stdout immediately. This ensures 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>...
> ---
> gdb/cli-out.c | 34 +++++++-
> gdb/cli-out.h | 3 +
> gdb/debuginfod-support.c | 15 ++--
> gdb/infrun.c | 17 +++-
> gdb/mi/mi-out.c | 37 +++++++++
> gdb/mi/mi-out.h | 3 +
> gdb/python/py-mi.c | 3 +
> gdb/stack.c | 38 ++++++---
> gdb/thread.c | 174 +++++++++++++++++++++++----------------
> gdb/ui-file.c | 136 ++++++++++++++++++++++++++++++
> gdb/ui-file.h | 92 ++++++++++++++++++++-
> gdb/ui-out.c | 36 ++++++++
> gdb/ui-out.h | 79 ++++++++++++++++++
> 13 files changed, 573 insertions(+), 94 deletions(-)
>
> diff --git a/gdb/cli-out.c b/gdb/cli-out.c
> index 20d3d93f1ad..fc9f81118c6 100644
> --- a/gdb/cli-out.c
> +++ b/gdb/cli-out.c
> @@ -263,6 +263,31 @@ cli_ui_out::do_redirect (ui_file *outstream)
> m_streams.pop_back ();
> }
>
> +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> +
> +void
> +cli_ui_out::do_redirect_to_buffer (ui_file *stream, buffer_file *buf_file)
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + if (*it == stream)
> + *it = buf_file;
> +}
> +
> +/* Replace any buffer_files in m_streams with the buffer_file's
> + underlying stream. */
> +
> +void
> +cli_ui_out::do_remove_buffers ()
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + buffer_file *buf = dynamic_cast<buffer_file *> (*it);
> +
> + if (buf != nullptr)
> + *it = buf->get_stream ();
> + }
> +}
> +
> /* Initialize a progress update to be displayed with
> cli_ui_out::do_progress_notify. */
>
> @@ -299,7 +324,8 @@ 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
> + = buffer_file::get_unbuffered_stream (m_streams.back ());
> cli_progress_info &info (m_progress_info.back ());
>
> if (chars_per_line > MAX_CHARS_PER_LINE)
> @@ -384,7 +410,8 @@ 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
> + = buffer_file::get_unbuffered_stream (m_streams.back ());
> int chars_per_line = get_chars_per_line ();
>
> scoped_restore save_pagination
> @@ -413,10 +440,11 @@ void
> cli_ui_out::do_progress_end ()
> {
> struct ui_file *stream = m_streams.back ();
> - m_progress_info.pop_back ();
>
> if (stream->isatty ())
> clear_progress_notify ();
> +
> + m_progress_info.pop_back ();
> }
>
> /* local functions */
> diff --git a/gdb/cli-out.h b/gdb/cli-out.h
> index 34016182269..ae4e650e7eb 100644
> --- a/gdb/cli-out.h
> +++ b/gdb/cli-out.h
> @@ -71,6 +71,9 @@ class cli_ui_out : public ui_out
> virtual void do_wrap_hint (int indent) override;
> virtual void do_flush () override;
> virtual void do_redirect (struct ui_file *outstream) override;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) override;
> + virtual void do_remove_buffers () override;
>
> virtual void do_progress_start () override;
> virtual void do_progress_notify (const std::string &, const char *,
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 66b58e8b673..90371afe907 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -168,7 +168,8 @@ progressfn (debuginfod_client *c, long cur, long total)
>
> if (check_quit_flag ())
> {
> - gdb_printf ("Cancelling download of %s %s...\n",
> + ui_file *outstream = buffer_file::get_unbuffered_stream (gdb_stdout);
> + gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> data->desc, styled_fname.c_str ());
> return 1;
> }
> @@ -284,10 +285,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 = buffer_file::get_unbuffered_stream (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 4d6df757d23..f719a4aad49 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8706,10 +8706,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;
> @@ -8738,6 +8738,17 @@ 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<void (ui_out *, bool)>
> + (do_print_stop_event, uiout, displays);
> +}
> +
> /* See infrun.h. */
>
> void
> diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
> index 29a416a426d..91bcd1216e2 100644
> --- a/gdb/mi/mi-out.c
> +++ b/gdb/mi/mi-out.c
> @@ -194,6 +194,43 @@ mi_ui_out::do_redirect (ui_file *outstream)
> m_streams.pop_back ();
> }
>
> +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> +
> +void
> +mi_ui_out::do_redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + if (*it != stream)
> + continue;
> +
> + string_file *sstream = dynamic_cast<string_file *> (*it);
> + if (sstream != nullptr)
> + {
> + /* Forward the contents of SSTREAM to BUF_FILE. */
> + buf_file->write (sstream->data (), sstream->size ());
> + sstream->clear ();
> + }
> +
> + *it = buf_file;
> + }
> +}
> +
> +/* Replace any buffer_files in m_streams with the buffer_file's
> + underlying stream. */
> +
> +void
> +mi_ui_out::do_remove_buffers ()
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + buffer_file *buf = dynamic_cast<buffer_file *> (*it);
> +
> + if (buf != nullptr)
> + *it = buf->get_stream ();
> + }
> +}
> +
> void
> mi_ui_out::field_separator ()
> {
> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
> index 10c9f8a4585..ee089abfdef 100644
> --- a/gdb/mi/mi-out.h
> +++ b/gdb/mi/mi-out.h
> @@ -79,6 +79,9 @@ class mi_ui_out : public ui_out
> virtual void do_wrap_hint (int indent) override;
> virtual void do_flush () override;
> virtual void do_redirect (struct ui_file *outstream) override;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) override;
> + virtual void do_remove_buffers () override;
>
> virtual bool do_is_mi_like_p () const override
> { return true; }
> diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> index 0fcd57844e7..0d672155265 100644
> --- a/gdb/python/py-mi.c
> +++ b/gdb/python/py-mi.c
> @@ -62,6 +62,9 @@ class py_ui_out : public ui_out
> void do_progress_notify (const std::string &, const char *, double, double)
> override
> { }
> + void do_redirect_to_buffer (struct ui_file *, buffer_file *) override
> + { }
> + void do_remove_buffers () override { }
>
> void do_table_begin (int nbrofcols, int nr_rows, const char *tblid) override
> {
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 002bf580634..608bb5fc1f0 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 (gdb::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,23 @@ 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)
> +{
> + using ftype = void (ui_out *, const frame_print_options &, frame_info_ptr,
> + int, enum print_what, int, int);
> +
> + do_with_buffered_output<ftype> (do_print_frame_info, current_uiout,
> + fp_opts, frame, print_level, print_what,
> + print_args, set_current_sal);
> +}
> +
> /* See stack.h. */
>
> void
> @@ -1309,13 +1327,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 7f7f035b5ab..03105fbd4ce 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1040,6 +1040,106 @@ 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;
> +
> + 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)
> +
> +{
> + using ftype = void (ui_out *, const char *, int, int, int,
> + int, thread_info *, thread_info *);
> +
> + do_with_buffered_output<ftype>
> + (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. */
> @@ -1123,82 +1223,12 @@ 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;
>
> - 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.c b/gdb/ui-file.c
> index e0814fe2b2d..c18e6d288c8 100644
> --- a/gdb/ui-file.c
> +++ b/gdb/ui-file.c
> @@ -234,6 +234,142 @@ string_file::can_emit_style_escape ()
>
>
>
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::wrap_here (int indent)
> +{
> + m_output_units.emplace_back (m_string, indent);
> + m_string.clear ();
> +}
> +
> +/* See ui-file.h. */
> +
> +int buffer_file::num_buffers = 0;
> +
> +/* See ui-file.h. */
> +
> +unsigned long buffer_file::output_unit::id_counter = 0;
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::write (const char *buf, long length_buf)
> +{
> + /* 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);
> +
> + /* Prepend a partial line if one was already written to
> + this buffer_file. */
> + if (!m_string.empty ())
> + {
> + msg.insert (0, m_string);
> + m_string.clear ();
> + }
> +
> + m_output_units.emplace_back (msg);
> + prev = cur + 1;
> + }
> +}
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::flush_next_unit ()
> +{
> + if (m_output_units.empty ())
> + return;
> +
> + output_unit unit = std::move (m_output_units.front ());
> +
> + m_output_units.pop_front ();
> + m_stream->puts (unit.m_msg.c_str ());
> +
> + if (unit.m_wrap_hint >= 0)
> + m_stream->wrap_here (unit.m_wrap_hint);
> +}
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::flush_streams (buffer_file *other)
> +{
> + /* Make sure all string contents of these buffer_files are
> + contained in their output_units. */
> + this->wrap_here (-1);
> + other->wrap_here (-1);
> +
> + while (!this->m_output_units.empty ()
> + || !other->m_output_units.empty ())
> + {
> + output_unit *out1 = nullptr;
> + output_unit *out2 = nullptr;
> +
> + if (!this->m_output_units.empty ())
> + out1 = &this->m_output_units.front ();
> + if (!other->m_output_units.empty ())
> + out2 = &other->m_output_units.front ();
> +
> + bool flush_this_next = output_unit::compare (out1, out2);
> +
> + /* out1 or out2 will be invalidated by flush_next_unit. */
> + if (flush_this_next)
> + this->flush_next_unit ();
> + else
> + other->flush_next_unit ();
> + }
> +}
> +
> +/* See ui-file.h. */
> +
> +ui_file *
> +buffer_file::get_unbuffered_stream (ui_file *stream)
> +{
> + buffer_file *buf = dynamic_cast<buffer_file *> (stream);
> +
> + if (buf == nullptr)
> + return stream;
> +
> + return get_unbuffered_stream (buf->m_stream);
> +}
> +
> +/* See ui-file.h. */
> +
> +bool
> +buffer_file::isatty ()
> +{
> + return m_stream->isatty ();
> +}
> +
> +/* See ui-file.h. */
> +
> +bool
> +buffer_file::output_unit::compare (output_unit *out1,
> + output_unit *out2)
> +{
> + gdb_assert (out1 != nullptr || out2 != nullptr);
> +
> + if (out1 != nullptr && out2 == nullptr)
> + return true;
> + if (out1 == nullptr && out2 != nullptr)
> + return false;
> +
> + return out1->m_id < out2->m_id;
> +}
> +
> +buffer_file::output_unit::output_unit (std::string msg, int wrap_hint)
> + : m_msg (msg), m_wrap_hint (wrap_hint), m_id (++id_counter)
> +{
> + /* IDs are assigned starting at 1. 0 indicates ID_COUNTER overflow. */
> + if (m_id == 0)
> + error (_("Message ID overflow"));
> +}
> +
> +
> +
> stdio_file::stdio_file (FILE *file, bool close_p)
> {
> set_stream (file);
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index de24620e247..e045750c4ef 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -19,6 +19,7 @@
> #ifndef UI_FILE_H
> #define UI_FILE_H
>
> +#include <list>
> #include <string>
> #include "ui-style.h"
>
> @@ -220,13 +221,102 @@ class string_file : public ui_file
> bool empty () const { return m_string.empty (); }
> void clear () { return m_string.clear (); }
>
> -private:
> +protected:
> /* The internal buffer. */
> std::string m_string;
>
> bool m_term_out;
> };
>
> +/* A string_file implementation that collects output on behalf of a
> + given ui_file. Provides access to the underlying ui_file so
> + that buffering can be selectively bypassed. */
> +
> +class buffer_file : public string_file
> +{
> +public:
> + explicit buffer_file (bool term_out, ui_file *stream)
> + : string_file (term_out), m_stream (stream)
> + {
> + ++num_buffers;
> + }
> +
> + ~buffer_file ()
> + {
> + /* Reset ID_COUNTER if possible to help prevent overflow. */
> + if (--num_buffers == 0)
> + output_unit::id_counter = 0;
> + }
> +
> + /* Return the stream associated with this buffer_file. */
> + ui_file *get_stream ()
> + {
> + return m_stream;
> + }
> +
> + /* Record the wrap hint. When flushing the buffer, the underlying
> + ui_file's wrap_here will be called at the current place in the
> + output. */
> + void wrap_here (int indent) override;
> +
> + /* Flush collected output from this buffer_file as well as OTHER to their
> + underlying ui_files. Output is written to the underlying files in the
> + same order in which it was written to this buffer_file or OTHER. */
> + void flush_streams (buffer_file *other);
> +
> + /* Return true if the underlying stream is a tty. */
> + bool isatty () override;
> +
> + /* Record BUF and schedule it to be written to the underlying stream
> + during flush_streams. */
> + void write (const char *buf, long length_buf) override;
> +
> + /* Return a pointer to STREAM's underlying ui_file. Recursively called
> + until a non-buffer_file is found. */
> + static ui_file *get_unbuffered_stream (ui_file *stream);
> +
> + /* Counter indicating the number of buffer_files currently in use. */
> + static int num_buffers;
> +
> +private:
> +
> + /* The underlying output stream. */
> + ui_file *m_stream;
> +
> + struct output_unit
> + {
> + output_unit (std::string msg, int wrap_hint = -1);
> +
> + /* 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 buffer_file flush. */
> + int m_wrap_hint;
> +
> + /* ID for this output_unit. */
> + unsigned long m_id;
> +
> + /* Return true if OUT1 should be flushed before OUT2 and false if
> + otherwise. At least one of OUT1 and OUT1 should be non-nullptr. */
> + static bool compare (output_unit *out1, output_unit *out2);
> +
> + /* Counter used to generate output_unit IDs. IDs are unique across
> + all current output_units. IDs are used to ensure that output_units
> + across multiple buffer_files are flushed in the same order in which
> + they were written. See buffer_file::flush_streams. */
> + static unsigned long id_counter;
> + };
> +
> + /* Write one output_unit to the underlying stream. output_unit is
> + selected via FIFO and is removed from m_output_units after being
> + written. */
> + void flush_next_unit ();
> +
> + /* output_units scheduled to be flushed to the underlying output stream. */
> + std::list<output_unit> m_output_units;
> +};
> +
> /* A ui_file implementation that maps directly onto <stdio.h>'s FILE.
> A stdio_file can either own its underlying file, or not. If it
> owns the file, then destroying the stdio_file closes the underlying
> diff --git a/gdb/ui-out.c b/gdb/ui-out.c
> index 9380630c320..14bb5068b4c 100644
> --- a/gdb/ui-out.c
> +++ b/gdb/ui-out.c
> @@ -799,6 +799,18 @@ ui_out::redirect (ui_file *outstream)
> do_redirect (outstream);
> }
>
> +void
> +ui_out::redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
> +{
> + do_redirect_to_buffer (stream, buf_file);
> +}
> +
> +void
> +ui_out::remove_buffers ()
> +{
> + do_remove_buffers ();
> +}
> +
> /* Test the flags against the mask given. */
> ui_out_flags
> ui_out::test_flags (ui_out_flags mask)
> @@ -871,3 +883,27 @@ ui_out::ui_out (ui_out_flags flags)
> ui_out::~ui_out ()
> {
> }
> +
> +ui_out_buffer_pop::ui_out_buffer_pop (ui_out *uiout)
> +: m_uiout (uiout),
> + m_stdout_buf (uiout->can_emit_style_escape (), gdb_stdout),
> + m_stderr_buf (uiout->can_emit_style_escape (), gdb_stderr),
> + m_prev_stdout (gdb_stdout),
> + m_prev_stderr (gdb_stderr)
> +{
> + m_uiout->redirect_to_buffer (gdb_stdout, &m_stdout_buf);
> + gdb_stdout = &m_stdout_buf;
> +
> + if (m_prev_stdout != m_prev_stderr)
> + {
> + m_uiout->redirect_to_buffer (gdb_stderr, &m_stderr_buf);
> + gdb_stderr = &m_stderr_buf;
> + }
> +}
> +
> +ui_out_buffer_pop::~ui_out_buffer_pop ()
> +{
> + m_uiout->remove_buffers ();
> + gdb_stdout = m_prev_stdout;
> + gdb_stderr = m_prev_stderr;
> +}
> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index ba5b1de68ab..964e6e3a8a4 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -262,6 +262,12 @@ class ui_out
> /* Redirect the output of a ui_out object temporarily. */
> void redirect (ui_file *outstream);
>
> + /* Redirect STREAM to BUF_FILE temporarily. */
> + void redirect_to_buffer (ui_file *stream, buffer_file *buf_file);
> +
> + /* Remove all buffer_files from this ui_out. */
> + void remove_buffers ();
> +
> ui_out_flags test_flags (ui_out_flags mask);
>
> /* HACK: Code in GDB is currently checking to see the type of ui_out
> @@ -360,6 +366,9 @@ class ui_out
> virtual void do_wrap_hint (int indent) = 0;
> virtual void do_flush () = 0;
> virtual void do_redirect (struct ui_file *outstream) = 0;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) = 0;
> + virtual void do_remove_buffers () = 0;
>
> virtual void do_progress_start () = 0;
> virtual void do_progress_notify (const std::string &, const char *,
> @@ -470,4 +479,74 @@ class ui_out_redirect_pop
> struct ui_out *m_uiout;
> };
>
> +/* On construction, redirect gdb_stdout and gdb_stderr to buffer_files.
> + Replace occurances of gdb_stdout and gdb_stderr in M_UIOUT with these
> + buffer_files. On destruction restore M_UIOUT, gdb_stdout and gdb_stderr. */
> +
> +class ui_out_buffer_pop
> +{
> +public:
> + ui_out_buffer_pop (ui_out *uiout);
> +
> + ~ui_out_buffer_pop ();
> +
> + /* Flush buffered output to the underlying output streams. */
> + void flush ()
> + {
> + m_stdout_buf.flush_streams (&m_stderr_buf);
> + }
> +
> + ui_out_buffer_pop (const ui_out_buffer_pop &) = delete;
> + ui_out_buffer_pop &operator= (const ui_out_buffer_pop &) = delete;
> +
> +private:
> + /* ui_out being temporarily redirected. */
> + struct ui_out *m_uiout;
> +
> + /* Buffer to which stdout is temporarily redirected to for the lifetime
> + of this object. */
> + buffer_file m_stdout_buf;
> +
> + /* Buffer to which stderr is temporarily redirected to for the lifetime
> + of this object. */
> + buffer_file m_stderr_buf;
> +
> + /* Original gdb_stdout at the time of this object's construction. */
> + ui_file *m_prev_stdout;
> +
> + /* Original gdb_stdout at the time of this object's construction. */
> + ui_file *m_prev_stderr;
> +};
> +
> +/* Redirect output for the duration of FUNC. */
> +
> +template<typename F, typename... Arg>
> +void
> +do_with_buffered_output (F func, ui_out *uiout, Arg... args)
> +{
> + ui_out_buffer_pop buf (uiout);
> +
> + try
> + {
> + func (uiout, std::forward<Arg> (args)...);
> + }
> + catch (gdb_exception &ex)
> + {
> + /* Ideally flush would be called in the destructor of buf,
> + however flushing might cause an exception to be thrown.
> + Catch it and ensure the first exception propagates. */
> + try
> + {
> + buf.flush ();
> + }
> + catch (const gdb_exception &ignore)
> + {
> + }
> +
> + throw_exception (std::move (ex));
> + }
> +
> + /* Try was successful. Let any further exceptions propagate. */
> + buf.flush ();
> +}
> #endif /* UI_OUT_H */
> --
> 2.40.1
>
Aaron Merey <amerey@redhat.com> writes:
> v3: https://sourceware.org/pipermail/gdb-patches/2023-June/199986.html
>
> v4 adds output buffering for gdb_stderr. This fixes a regression
> introduced by v3 where messages printed to gdb_stderr appeared out of
> order with respect to buffered gdb_stdout messages.
>
> To ensure that writes to gdb_stderr and gdb_stdout occur in the same
> order that they were written to the buffers, ID numbers are assigned
> to each output_unit recorded by a buffer_file. IDs are ordered and
> unique across all output_units currently in use in any buffer_file.
> output_units represent a string to be written to a buffer_file's
> underlying stream. These strings terminate in either a newline or a
> call to wrap_here.
>
> When two buffer_files are simultaneously flushed, IDs for the next
> output_units in each buffer_file are compared to determine which
> should be written first.
>
> Tests for this patch include the existing gdb.base/solib-display.exp
> 'after rerun' tests, in which messages are printed to both gdb_stdout
> and gdb_stderr while both streams are buffered. Tests that check
> the correctness of debuginfod progress messages during output stream
> buffering are included in patches 5/6 and 6/6 in this series.
>
> Commit message:
>
> Introduce new ui_file buffer_file to temporarily collect output
> written to gdb_stdout and gdb_stderr during print_thread,
> print_frame_info and print_stop_event.
>
> This ensures that output from 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 accumulate writes to gdb_stdout and gdb_stderr in
> buffer_file objects. Debuginfod progress messages skip the buffers
> and print to gdb_stdout immediately. This ensures 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>...
I have started taking a look at this, but I'm not going to finish today,
so I thought I send the minor nits I'd spotted so far,
> ---
> gdb/cli-out.c | 34 +++++++-
> gdb/cli-out.h | 3 +
> gdb/debuginfod-support.c | 15 ++--
> gdb/infrun.c | 17 +++-
> gdb/mi/mi-out.c | 37 +++++++++
> gdb/mi/mi-out.h | 3 +
> gdb/python/py-mi.c | 3 +
> gdb/stack.c | 38 ++++++---
> gdb/thread.c | 174 +++++++++++++++++++++++----------------
> gdb/ui-file.c | 136 ++++++++++++++++++++++++++++++
> gdb/ui-file.h | 92 ++++++++++++++++++++-
> gdb/ui-out.c | 36 ++++++++
> gdb/ui-out.h | 79 ++++++++++++++++++
> 13 files changed, 573 insertions(+), 94 deletions(-)
>
> diff --git a/gdb/cli-out.c b/gdb/cli-out.c
> index 20d3d93f1ad..fc9f81118c6 100644
> --- a/gdb/cli-out.c
> +++ b/gdb/cli-out.c
> @@ -263,6 +263,31 @@ cli_ui_out::do_redirect (ui_file *outstream)
> m_streams.pop_back ();
> }
>
> +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> +
> +void
> +cli_ui_out::do_redirect_to_buffer (ui_file *stream, buffer_file *buf_file)
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + if (*it == stream)
> + *it = buf_file;
> +}
> +
> +/* Replace any buffer_files in m_streams with the buffer_file's
> + underlying stream. */
> +
> +void
> +cli_ui_out::do_remove_buffers ()
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + buffer_file *buf = dynamic_cast<buffer_file *> (*it);
> +
> + if (buf != nullptr)
> + *it = buf->get_stream ();
> + }
> +}
> +
> /* Initialize a progress update to be displayed with
> cli_ui_out::do_progress_notify. */
>
> @@ -299,7 +324,8 @@ 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
> + = buffer_file::get_unbuffered_stream (m_streams.back ());
> cli_progress_info &info (m_progress_info.back ());
>
> if (chars_per_line > MAX_CHARS_PER_LINE)
> @@ -384,7 +410,8 @@ 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
> + = buffer_file::get_unbuffered_stream (m_streams.back ());
> int chars_per_line = get_chars_per_line ();
>
> scoped_restore save_pagination
> @@ -413,10 +440,11 @@ void
> cli_ui_out::do_progress_end ()
> {
> struct ui_file *stream = m_streams.back ();
> - m_progress_info.pop_back ();
>
> if (stream->isatty ())
> clear_progress_notify ();
> +
> + m_progress_info.pop_back ();
> }
>
> /* local functions */
> diff --git a/gdb/cli-out.h b/gdb/cli-out.h
> index 34016182269..ae4e650e7eb 100644
> --- a/gdb/cli-out.h
> +++ b/gdb/cli-out.h
> @@ -71,6 +71,9 @@ class cli_ui_out : public ui_out
> virtual void do_wrap_hint (int indent) override;
> virtual void do_flush () override;
> virtual void do_redirect (struct ui_file *outstream) override;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) override;
> + virtual void do_remove_buffers () override;
>
> virtual void do_progress_start () override;
> virtual void do_progress_notify (const std::string &, const char *,
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 66b58e8b673..90371afe907 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -168,7 +168,8 @@ progressfn (debuginfod_client *c, long cur, long total)
>
> if (check_quit_flag ())
> {
> - gdb_printf ("Cancelling download of %s %s...\n",
> + ui_file *outstream = buffer_file::get_unbuffered_stream (gdb_stdout);
> + gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> data->desc, styled_fname.c_str ());
> return 1;
> }
> @@ -284,10 +285,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 = buffer_file::get_unbuffered_stream (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 4d6df757d23..f719a4aad49 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8706,10 +8706,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;
> @@ -8738,6 +8738,17 @@ 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<void (ui_out *, bool)>
> + (do_print_stop_event, uiout, displays);
You don't need to provide the template argument here, the compiler is
able to figure that out by itself. This doesn't make much difference
here maybe, but in other places you've created a typedef for the
function signature, and that can all be removed.
With that simplification done, you might find you don't even need the
print_stop_event (and other) wrapper functions any more.
> +}
> +
> /* See infrun.h. */
>
> void
> diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
> index 29a416a426d..91bcd1216e2 100644
> --- a/gdb/mi/mi-out.c
> +++ b/gdb/mi/mi-out.c
> @@ -194,6 +194,43 @@ mi_ui_out::do_redirect (ui_file *outstream)
> m_streams.pop_back ();
> }
>
> +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> +
> +void
> +mi_ui_out::do_redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + if (*it != stream)
> + continue;
> +
> + string_file *sstream = dynamic_cast<string_file *> (*it);
> + if (sstream != nullptr)
> + {
> + /* Forward the contents of SSTREAM to BUF_FILE. */
> + buf_file->write (sstream->data (), sstream->size ());
> + sstream->clear ();
> + }
> +
> + *it = buf_file;
> + }
> +}
> +
> +/* Replace any buffer_files in m_streams with the buffer_file's
> + underlying stream. */
> +
> +void
> +mi_ui_out::do_remove_buffers ()
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + buffer_file *buf = dynamic_cast<buffer_file *> (*it);
> +
> + if (buf != nullptr)
> + *it = buf->get_stream ();
> + }
> +}
> +
> void
> mi_ui_out::field_separator ()
> {
> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
> index 10c9f8a4585..ee089abfdef 100644
> --- a/gdb/mi/mi-out.h
> +++ b/gdb/mi/mi-out.h
> @@ -79,6 +79,9 @@ class mi_ui_out : public ui_out
> virtual void do_wrap_hint (int indent) override;
> virtual void do_flush () override;
> virtual void do_redirect (struct ui_file *outstream) override;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) override;
> + virtual void do_remove_buffers () override;
>
> virtual bool do_is_mi_like_p () const override
> { return true; }
> diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> index 0fcd57844e7..0d672155265 100644
> --- a/gdb/python/py-mi.c
> +++ b/gdb/python/py-mi.c
> @@ -62,6 +62,9 @@ class py_ui_out : public ui_out
> void do_progress_notify (const std::string &, const char *, double, double)
> override
> { }
> + void do_redirect_to_buffer (struct ui_file *, buffer_file *) override
> + { }
> + void do_remove_buffers () override { }
>
> void do_table_begin (int nbrofcols, int nr_rows, const char *tblid) override
> {
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 002bf580634..608bb5fc1f0 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 (gdb::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,23 @@ 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)
> +{
> + using ftype = void (ui_out *, const frame_print_options &, frame_info_ptr,
> + int, enum print_what, int, int);
> +
> + do_with_buffered_output<ftype> (do_print_frame_info, current_uiout,
> + fp_opts, frame, print_level, print_what,
> + print_args, set_current_sal);
> +}
> +
> /* See stack.h. */
>
> void
> @@ -1309,13 +1327,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 7f7f035b5ab..03105fbd4ce 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1040,6 +1040,106 @@ 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;
> +
> + 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)
> +
> +{
> + using ftype = void (ui_out *, const char *, int, int, int,
> + int, thread_info *, thread_info *);
> +
> + do_with_buffered_output<ftype>
> + (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. */
> @@ -1123,82 +1223,12 @@ 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;
>
> - 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.c b/gdb/ui-file.c
> index e0814fe2b2d..c18e6d288c8 100644
> --- a/gdb/ui-file.c
> +++ b/gdb/ui-file.c
> @@ -234,6 +234,142 @@ string_file::can_emit_style_escape ()
>
>
>
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::wrap_here (int indent)
> +{
> + m_output_units.emplace_back (m_string, indent);
> + m_string.clear ();
> +}
> +
> +/* See ui-file.h. */
> +
> +int buffer_file::num_buffers = 0;
> +
> +/* See ui-file.h. */
> +
> +unsigned long buffer_file::output_unit::id_counter = 0;
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::write (const char *buf, long length_buf)
> +{
> + /* 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);
> +
> + /* Prepend a partial line if one was already written to
> + this buffer_file. */
> + if (!m_string.empty ())
> + {
> + msg.insert (0, m_string);
> + m_string.clear ();
> + }
> +
> + m_output_units.emplace_back (msg);
> + prev = cur + 1;
> + }
> +}
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::flush_next_unit ()
> +{
> + if (m_output_units.empty ())
> + return;
> +
> + output_unit unit = std::move (m_output_units.front ());
> +
> + m_output_units.pop_front ();
> + m_stream->puts (unit.m_msg.c_str ());
> +
> + if (unit.m_wrap_hint >= 0)
> + m_stream->wrap_here (unit.m_wrap_hint);
> +}
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::flush_streams (buffer_file *other)
> +{
> + /* Make sure all string contents of these buffer_files are
> + contained in their output_units. */
> + this->wrap_here (-1);
> + other->wrap_here (-1);
> +
> + while (!this->m_output_units.empty ()
> + || !other->m_output_units.empty ())
> + {
> + output_unit *out1 = nullptr;
> + output_unit *out2 = nullptr;
> +
> + if (!this->m_output_units.empty ())
> + out1 = &this->m_output_units.front ();
> + if (!other->m_output_units.empty ())
> + out2 = &other->m_output_units.front ();
> +
> + bool flush_this_next = output_unit::compare (out1, out2);
> +
> + /* out1 or out2 will be invalidated by flush_next_unit. */
> + if (flush_this_next)
> + this->flush_next_unit ();
> + else
> + other->flush_next_unit ();
> + }
> +}
> +
> +/* See ui-file.h. */
> +
> +ui_file *
> +buffer_file::get_unbuffered_stream (ui_file *stream)
> +{
> + buffer_file *buf = dynamic_cast<buffer_file *> (stream);
> +
> + if (buf == nullptr)
> + return stream;
> +
> + return get_unbuffered_stream (buf->m_stream);
> +}
> +
> +/* See ui-file.h. */
> +
> +bool
> +buffer_file::isatty ()
> +{
> + return m_stream->isatty ();
> +}
> +
> +/* See ui-file.h. */
> +
> +bool
> +buffer_file::output_unit::compare (output_unit *out1,
> + output_unit *out2)
> +{
> + gdb_assert (out1 != nullptr || out2 != nullptr);
> +
> + if (out1 != nullptr && out2 == nullptr)
> + return true;
> + if (out1 == nullptr && out2 != nullptr)
> + return false;
> +
> + return out1->m_id < out2->m_id;
> +}
> +
> +buffer_file::output_unit::output_unit (std::string msg, int wrap_hint)
> + : m_msg (msg), m_wrap_hint (wrap_hint), m_id (++id_counter)
> +{
> + /* IDs are assigned starting at 1. 0 indicates ID_COUNTER overflow. */
> + if (m_id == 0)
> + error (_("Message ID overflow"));
> +}
> +
> +
> +
> stdio_file::stdio_file (FILE *file, bool close_p)
> {
> set_stream (file);
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index de24620e247..e045750c4ef 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -19,6 +19,7 @@
> #ifndef UI_FILE_H
> #define UI_FILE_H
>
> +#include <list>
> #include <string>
> #include "ui-style.h"
>
> @@ -220,13 +221,102 @@ class string_file : public ui_file
> bool empty () const { return m_string.empty (); }
> void clear () { return m_string.clear (); }
>
> -private:
> +protected:
> /* The internal buffer. */
> std::string m_string;
>
> bool m_term_out;
> };
>
> +/* A string_file implementation that collects output on behalf of a
> + given ui_file. Provides access to the underlying ui_file so
> + that buffering can be selectively bypassed. */
> +
> +class buffer_file : public string_file
> +{
> +public:
> + explicit buffer_file (bool term_out, ui_file *stream)
> + : string_file (term_out), m_stream (stream)
> + {
> + ++num_buffers;
> + }
> +
> + ~buffer_file ()
> + {
> + /* Reset ID_COUNTER if possible to help prevent overflow. */
> + if (--num_buffers == 0)
> + output_unit::id_counter = 0;
> + }
> +
> + /* Return the stream associated with this buffer_file. */
> + ui_file *get_stream ()
> + {
> + return m_stream;
> + }
> +
> + /* Record the wrap hint. When flushing the buffer, the underlying
> + ui_file's wrap_here will be called at the current place in the
> + output. */
> + void wrap_here (int indent) override;
> +
> + /* Flush collected output from this buffer_file as well as OTHER to their
> + underlying ui_files. Output is written to the underlying files in the
> + same order in which it was written to this buffer_file or OTHER. */
> + void flush_streams (buffer_file *other);
> +
> + /* Return true if the underlying stream is a tty. */
> + bool isatty () override;
> +
> + /* Record BUF and schedule it to be written to the underlying stream
> + during flush_streams. */
> + void write (const char *buf, long length_buf) override;
> +
> + /* Return a pointer to STREAM's underlying ui_file. Recursively called
> + until a non-buffer_file is found. */
> + static ui_file *get_unbuffered_stream (ui_file *stream);
> +
> + /* Counter indicating the number of buffer_files currently in use. */
> + static int num_buffers;
> +
> +private:
> +
> + /* The underlying output stream. */
> + ui_file *m_stream;
> +
> + struct output_unit
> + {
> + output_unit (std::string msg, int wrap_hint = -1);
> +
> + /* 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 buffer_file flush. */
> + int m_wrap_hint;
> +
> + /* ID for this output_unit. */
> + unsigned long m_id;
> +
> + /* Return true if OUT1 should be flushed before OUT2 and false if
> + otherwise. At least one of OUT1 and OUT1 should be non-nullptr. */
> + static bool compare (output_unit *out1, output_unit *out2);
> +
> + /* Counter used to generate output_unit IDs. IDs are unique across
> + all current output_units. IDs are used to ensure that output_units
> + across multiple buffer_files are flushed in the same order in which
> + they were written. See buffer_file::flush_streams. */
> + static unsigned long id_counter;
> + };
> +
> + /* Write one output_unit to the underlying stream. output_unit is
> + selected via FIFO and is removed from m_output_units after being
> + written. */
> + void flush_next_unit ();
> +
> + /* output_units scheduled to be flushed to the underlying output stream. */
> + std::list<output_unit> m_output_units;
> +};
> +
> /* A ui_file implementation that maps directly onto <stdio.h>'s FILE.
> A stdio_file can either own its underlying file, or not. If it
> owns the file, then destroying the stdio_file closes the underlying
> diff --git a/gdb/ui-out.c b/gdb/ui-out.c
> index 9380630c320..14bb5068b4c 100644
> --- a/gdb/ui-out.c
> +++ b/gdb/ui-out.c
> @@ -799,6 +799,18 @@ ui_out::redirect (ui_file *outstream)
> do_redirect (outstream);
> }
>
> +void
> +ui_out::redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
> +{
> + do_redirect_to_buffer (stream, buf_file);
> +}
> +
> +void
> +ui_out::remove_buffers ()
> +{
> + do_remove_buffers ();
> +}
> +
> /* Test the flags against the mask given. */
> ui_out_flags
> ui_out::test_flags (ui_out_flags mask)
> @@ -871,3 +883,27 @@ ui_out::ui_out (ui_out_flags flags)
> ui_out::~ui_out ()
> {
> }
> +
> +ui_out_buffer_pop::ui_out_buffer_pop (ui_out *uiout)
> +: m_uiout (uiout),
> + m_stdout_buf (uiout->can_emit_style_escape (), gdb_stdout),
> + m_stderr_buf (uiout->can_emit_style_escape (), gdb_stderr),
> + m_prev_stdout (gdb_stdout),
> + m_prev_stderr (gdb_stderr)
> +{
> + m_uiout->redirect_to_buffer (gdb_stdout, &m_stdout_buf);
> + gdb_stdout = &m_stdout_buf;
> +
> + if (m_prev_stdout != m_prev_stderr)
> + {
> + m_uiout->redirect_to_buffer (gdb_stderr, &m_stderr_buf);
> + gdb_stderr = &m_stderr_buf;
> + }
> +}
> +
> +ui_out_buffer_pop::~ui_out_buffer_pop ()
> +{
> + m_uiout->remove_buffers ();
> + gdb_stdout = m_prev_stdout;
> + gdb_stderr = m_prev_stderr;
> +}
> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index ba5b1de68ab..964e6e3a8a4 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -262,6 +262,12 @@ class ui_out
> /* Redirect the output of a ui_out object temporarily. */
> void redirect (ui_file *outstream);
>
> + /* Redirect STREAM to BUF_FILE temporarily. */
> + void redirect_to_buffer (ui_file *stream, buffer_file *buf_file);
> +
> + /* Remove all buffer_files from this ui_out. */
> + void remove_buffers ();
> +
> ui_out_flags test_flags (ui_out_flags mask);
>
> /* HACK: Code in GDB is currently checking to see the type of ui_out
> @@ -360,6 +366,9 @@ class ui_out
> virtual void do_wrap_hint (int indent) = 0;
> virtual void do_flush () = 0;
> virtual void do_redirect (struct ui_file *outstream) = 0;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) = 0;
> + virtual void do_remove_buffers () = 0;
>
> virtual void do_progress_start () = 0;
> virtual void do_progress_notify (const std::string &, const char *,
> @@ -470,4 +479,74 @@ class ui_out_redirect_pop
> struct ui_out *m_uiout;
> };
>
> +/* On construction, redirect gdb_stdout and gdb_stderr to buffer_files.
> + Replace occurances of gdb_stdout and gdb_stderr in M_UIOUT with these
> + buffer_files. On destruction restore M_UIOUT, gdb_stdout and gdb_stderr. */
> +
> +class ui_out_buffer_pop
I confess that I've confused by this name. What does the 'pop' mean here?
> +{
> +public:
Instead of 'class' and then 'public', I usually just declare as
'struct', but that's just a preference, we have plenty of code that does
class/public.
> + ui_out_buffer_pop (ui_out *uiout);
> +
> + ~ui_out_buffer_pop ();
> +
> + /* Flush buffered output to the underlying output streams. */
> + void flush ()
> + {
> + m_stdout_buf.flush_streams (&m_stderr_buf);
> + }
> +
> + ui_out_buffer_pop (const ui_out_buffer_pop &) = delete;
> + ui_out_buffer_pop &operator= (const ui_out_buffer_pop &) = delete;
> +
> +private:
> + /* ui_out being temporarily redirected. */
> + struct ui_out *m_uiout;
> +
> + /* Buffer to which stdout is temporarily redirected to for the lifetime
> + of this object. */
> + buffer_file m_stdout_buf;
> +
> + /* Buffer to which stderr is temporarily redirected to for the lifetime
> + of this object. */
> + buffer_file m_stderr_buf;
> +
> + /* Original gdb_stdout at the time of this object's construction. */
> + ui_file *m_prev_stdout;
> +
> + /* Original gdb_stdout at the time of this object's construction. */
> + ui_file *m_prev_stderr;
> +};
> +
> +/* Redirect output for the duration of FUNC. */
> +
I'm not sure 'Redirect' is the right word to use here, at least, not
without some supporting text to explain where the output is redirected
too.
Thanks,
Andrew
> +template<typename F, typename... Arg>
> +void
> +do_with_buffered_output (F func, ui_out *uiout, Arg... args)
> +{
> + ui_out_buffer_pop buf (uiout);
> +
> + try
> + {
> + func (uiout, std::forward<Arg> (args)...);
> + }
> + catch (gdb_exception &ex)
> + {
> + /* Ideally flush would be called in the destructor of buf,
> + however flushing might cause an exception to be thrown.
> + Catch it and ensure the first exception propagates. */
> + try
> + {
> + buf.flush ();
> + }
> + catch (const gdb_exception &ignore)
> + {
> + }
> +
> + throw_exception (std::move (ex));
> + }
> +
> + /* Try was successful. Let any further exceptions propagate. */
> + buf.flush ();
> +}
> #endif /* UI_OUT_H */
> --
> 2.40.1
Thanks Andrew, I'll post a revision with the tweaks you suggested.
On Fri, Jul 7, 2023 at 10:19 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> > +/* On construction, redirect gdb_stdout and gdb_stderr to buffer_files.
> > + Replace occurances of gdb_stdout and gdb_stderr in M_UIOUT with these
> > + buffer_files. On destruction restore M_UIOUT, gdb_stdout and gdb_stderr. */
> > +
> > +class ui_out_buffer_pop
>
> I confess that I've confused by this name. What does the 'pop' mean here?
I was trying to convey that this object handles the insertion and
removal (pop) of buffer_files from the given ui_out. There is a similar
object ui_out_redirect_pop that I modelled this name after.
Aaron
On Fri, Jul 7, 2023 at 10:19 AM Andrew Burgess <aburgess@redhat.com> wrote:
> > To fix this we accumulate writes to gdb_stdout and gdb_stderr in
> > buffer_file objects. Debuginfod progress messages skip the buffers
> > and print to gdb_stdout immediately. This ensures 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>...
>
> I have started taking a look at this, but I'm not going to finish today,
> so I thought I send the minor nits I'd spotted so far,
Ping
Thanks,
Aaron
Aaron Merey <amerey@redhat.com> writes:
> v3: https://sourceware.org/pipermail/gdb-patches/2023-June/199986.html
>
> v4 adds output buffering for gdb_stderr. This fixes a regression
> introduced by v3 where messages printed to gdb_stderr appeared out of
> order with respect to buffered gdb_stdout messages.
>
> To ensure that writes to gdb_stderr and gdb_stdout occur in the same
> order that they were written to the buffers, ID numbers are assigned
> to each output_unit recorded by a buffer_file. IDs are ordered and
> unique across all output_units currently in use in any buffer_file.
> output_units represent a string to be written to a buffer_file's
> underlying stream. These strings terminate in either a newline or a
> call to wrap_here.
>
> When two buffer_files are simultaneously flushed, IDs for the next
> output_units in each buffer_file are compared to determine which
> should be written first.
>
> Tests for this patch include the existing gdb.base/solib-display.exp
> 'after rerun' tests, in which messages are printed to both gdb_stdout
> and gdb_stderr while both streams are buffered. Tests that check
> the correctness of debuginfod progress messages during output stream
> buffering are included in patches 5/6 and 6/6 in this series.
>
> Commit message:
>
> Introduce new ui_file buffer_file to temporarily collect output
> written to gdb_stdout and gdb_stderr during print_thread,
> print_frame_info and print_stop_event.
>
> This ensures that output from 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 accumulate writes to gdb_stdout and gdb_stderr in
> buffer_file objects. Debuginfod progress messages skip the buffers
> and print to gdb_stdout immediately. This ensures 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>...
> ---
> gdb/cli-out.c | 34 +++++++-
> gdb/cli-out.h | 3 +
> gdb/debuginfod-support.c | 15 ++--
> gdb/infrun.c | 17 +++-
> gdb/mi/mi-out.c | 37 +++++++++
> gdb/mi/mi-out.h | 3 +
> gdb/python/py-mi.c | 3 +
> gdb/stack.c | 38 ++++++---
> gdb/thread.c | 174 +++++++++++++++++++++++----------------
> gdb/ui-file.c | 136 ++++++++++++++++++++++++++++++
> gdb/ui-file.h | 92 ++++++++++++++++++++-
> gdb/ui-out.c | 36 ++++++++
> gdb/ui-out.h | 79 ++++++++++++++++++
> 13 files changed, 573 insertions(+), 94 deletions(-)
>
> diff --git a/gdb/cli-out.c b/gdb/cli-out.c
> index 20d3d93f1ad..fc9f81118c6 100644
> --- a/gdb/cli-out.c
> +++ b/gdb/cli-out.c
> @@ -263,6 +263,31 @@ cli_ui_out::do_redirect (ui_file *outstream)
> m_streams.pop_back ();
> }
>
> +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> +
> +void
> +cli_ui_out::do_redirect_to_buffer (ui_file *stream, buffer_file *buf_file)
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + if (*it == stream)
> + *it = buf_file;
> +}
I would love to understand more about why the existing redirect
mechanism doesn't work for you. As I understand it, the existing
redirect adds a new ui_file object to the current_ui's stream stack, and
all output after that point will be sent to that stream.
In contrast, your approach seems to indicate you're seeing writes to
lower level streams ... which seems ... odd.
> +
> +/* Replace any buffer_files in m_streams with the buffer_file's
> + underlying stream. */
> +
> +void
> +cli_ui_out::do_remove_buffers ()
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + buffer_file *buf = dynamic_cast<buffer_file *> (*it);
> +
> + if (buf != nullptr)
> + *it = buf->get_stream ();
> + }
> +}
> +
> /* Initialize a progress update to be displayed with
> cli_ui_out::do_progress_notify. */
>
> @@ -299,7 +324,8 @@ 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
> + = buffer_file::get_unbuffered_stream (m_streams.back ());
> cli_progress_info &info (m_progress_info.back ());
>
> if (chars_per_line > MAX_CHARS_PER_LINE)
> @@ -384,7 +410,8 @@ 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
> + = buffer_file::get_unbuffered_stream (m_streams.back ());
> int chars_per_line = get_chars_per_line ();
>
> scoped_restore save_pagination
> @@ -413,10 +440,11 @@ void
> cli_ui_out::do_progress_end ()
> {
> struct ui_file *stream = m_streams.back ();
> - m_progress_info.pop_back ();
>
> if (stream->isatty ())
> clear_progress_notify ();
> +
> + m_progress_info.pop_back ();
> }
>
> /* local functions */
> diff --git a/gdb/cli-out.h b/gdb/cli-out.h
> index 34016182269..ae4e650e7eb 100644
> --- a/gdb/cli-out.h
> +++ b/gdb/cli-out.h
> @@ -71,6 +71,9 @@ class cli_ui_out : public ui_out
> virtual void do_wrap_hint (int indent) override;
> virtual void do_flush () override;
> virtual void do_redirect (struct ui_file *outstream) override;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) override;
> + virtual void do_remove_buffers () override;
>
> virtual void do_progress_start () override;
> virtual void do_progress_notify (const std::string &, const char *,
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 66b58e8b673..90371afe907 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -168,7 +168,8 @@ progressfn (debuginfod_client *c, long cur, long total)
>
> if (check_quit_flag ())
> {
> - gdb_printf ("Cancelling download of %s %s...\n",
> + ui_file *outstream = buffer_file::get_unbuffered_stream (gdb_stdout);
> + gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
> data->desc, styled_fname.c_str ());
> return 1;
> }
> @@ -284,10 +285,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 = buffer_file::get_unbuffered_stream (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 4d6df757d23..f719a4aad49 100644
> --- a/gdb/infrun.c
> +++ b/gdb/infrun.c
> @@ -8706,10 +8706,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;
> @@ -8738,6 +8738,17 @@ 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<void (ui_out *, bool)>
> + (do_print_stop_event, uiout, displays);
> +}
> +
> /* See infrun.h. */
>
> void
> diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
> index 29a416a426d..91bcd1216e2 100644
> --- a/gdb/mi/mi-out.c
> +++ b/gdb/mi/mi-out.c
> @@ -194,6 +194,43 @@ mi_ui_out::do_redirect (ui_file *outstream)
> m_streams.pop_back ();
> }
>
> +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> +
> +void
> +mi_ui_out::do_redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + if (*it != stream)
> + continue;
> +
> + string_file *sstream = dynamic_cast<string_file *> (*it);
> + if (sstream != nullptr)
> + {
> + /* Forward the contents of SSTREAM to BUF_FILE. */
> + buf_file->write (sstream->data (), sstream->size ());
> + sstream->clear ();
> + }
> +
> + *it = buf_file;
> + }
> +}
> +
> +/* Replace any buffer_files in m_streams with the buffer_file's
> + underlying stream. */
> +
> +void
> +mi_ui_out::do_remove_buffers ()
> +{
> + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> + {
> + buffer_file *buf = dynamic_cast<buffer_file *> (*it);
> +
> + if (buf != nullptr)
> + *it = buf->get_stream ();
> + }
> +}
> +
> void
> mi_ui_out::field_separator ()
> {
> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
> index 10c9f8a4585..ee089abfdef 100644
> --- a/gdb/mi/mi-out.h
> +++ b/gdb/mi/mi-out.h
> @@ -79,6 +79,9 @@ class mi_ui_out : public ui_out
> virtual void do_wrap_hint (int indent) override;
> virtual void do_flush () override;
> virtual void do_redirect (struct ui_file *outstream) override;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) override;
> + virtual void do_remove_buffers () override;
>
> virtual bool do_is_mi_like_p () const override
> { return true; }
> diff --git a/gdb/python/py-mi.c b/gdb/python/py-mi.c
> index 0fcd57844e7..0d672155265 100644
> --- a/gdb/python/py-mi.c
> +++ b/gdb/python/py-mi.c
> @@ -62,6 +62,9 @@ class py_ui_out : public ui_out
> void do_progress_notify (const std::string &, const char *, double, double)
> override
> { }
> + void do_redirect_to_buffer (struct ui_file *, buffer_file *) override
> + { }
> + void do_remove_buffers () override { }
>
> void do_table_begin (int nbrofcols, int nr_rows, const char *tblid) override
> {
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 002bf580634..608bb5fc1f0 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 (gdb::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,23 @@ 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)
> +{
> + using ftype = void (ui_out *, const frame_print_options &, frame_info_ptr,
> + int, enum print_what, int, int);
> +
> + do_with_buffered_output<ftype> (do_print_frame_info, current_uiout,
> + fp_opts, frame, print_level, print_what,
> + print_args, set_current_sal);
> +}
> +
> /* See stack.h. */
>
> void
> @@ -1309,13 +1327,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 7f7f035b5ab..03105fbd4ce 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -1040,6 +1040,106 @@ 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;
> +
> + 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)
> +
> +{
> + using ftype = void (ui_out *, const char *, int, int, int,
> + int, thread_info *, thread_info *);
I think I already said in another email, but these typedefs are not
needed, the compiler can figure this stuff out.
> +
> + do_with_buffered_output<ftype>
> + (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. */
> @@ -1123,82 +1223,12 @@ 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;
>
> - 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.c b/gdb/ui-file.c
> index e0814fe2b2d..c18e6d288c8 100644
> --- a/gdb/ui-file.c
> +++ b/gdb/ui-file.c
> @@ -234,6 +234,142 @@ string_file::can_emit_style_escape ()
>
>
>
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::wrap_here (int indent)
> +{
> + m_output_units.emplace_back (m_string, indent);
> + m_string.clear ();
> +}
> +
> +/* See ui-file.h. */
> +
> +int buffer_file::num_buffers = 0;
> +
> +/* See ui-file.h. */
> +
> +unsigned long buffer_file::output_unit::id_counter = 0;
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::write (const char *buf, long length_buf)
> +{
> + /* 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);
> +
> + /* Prepend a partial line if one was already written to
> + this buffer_file. */
> + if (!m_string.empty ())
> + {
> + msg.insert (0, m_string);
> + m_string.clear ();
> + }
> +
> + m_output_units.emplace_back (msg);
> + prev = cur + 1;
> + }
> +}
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::flush_next_unit ()
> +{
> + if (m_output_units.empty ())
> + return;
> +
> + output_unit unit = std::move (m_output_units.front ());
> +
> + m_output_units.pop_front ();
> + m_stream->puts (unit.m_msg.c_str ());
> +
> + if (unit.m_wrap_hint >= 0)
> + m_stream->wrap_here (unit.m_wrap_hint);
> +}
> +
> +/* See ui-file.h. */
> +
> +void
> +buffer_file::flush_streams (buffer_file *other)
> +{
> + /* Make sure all string contents of these buffer_files are
> + contained in their output_units. */
> + this->wrap_here (-1);
> + other->wrap_here (-1);
> +
> + while (!this->m_output_units.empty ()
> + || !other->m_output_units.empty ())
> + {
> + output_unit *out1 = nullptr;
> + output_unit *out2 = nullptr;
> +
> + if (!this->m_output_units.empty ())
> + out1 = &this->m_output_units.front ();
> + if (!other->m_output_units.empty ())
> + out2 = &other->m_output_units.front ();
> +
> + bool flush_this_next = output_unit::compare (out1, out2);
> +
> + /* out1 or out2 will be invalidated by flush_next_unit. */
> + if (flush_this_next)
> + this->flush_next_unit ();
> + else
> + other->flush_next_unit ();
> + }
> +}
> +
> +/* See ui-file.h. */
> +
> +ui_file *
> +buffer_file::get_unbuffered_stream (ui_file *stream)
> +{
> + buffer_file *buf = dynamic_cast<buffer_file *> (stream);
> +
> + if (buf == nullptr)
> + return stream;
> +
> + return get_unbuffered_stream (buf->m_stream);
> +}
> +
> +/* See ui-file.h. */
> +
> +bool
> +buffer_file::isatty ()
> +{
> + return m_stream->isatty ();
> +}
> +
> +/* See ui-file.h. */
> +
> +bool
> +buffer_file::output_unit::compare (output_unit *out1,
> + output_unit *out2)
> +{
> + gdb_assert (out1 != nullptr || out2 != nullptr);
> +
> + if (out1 != nullptr && out2 == nullptr)
> + return true;
> + if (out1 == nullptr && out2 != nullptr)
> + return false;
> +
> + return out1->m_id < out2->m_id;
> +}
> +
> +buffer_file::output_unit::output_unit (std::string msg, int wrap_hint)
> + : m_msg (msg), m_wrap_hint (wrap_hint), m_id (++id_counter)
> +{
> + /* IDs are assigned starting at 1. 0 indicates ID_COUNTER overflow. */
> + if (m_id == 0)
> + error (_("Message ID overflow"));
> +}
> +
> +
> +
> stdio_file::stdio_file (FILE *file, bool close_p)
> {
> set_stream (file);
> diff --git a/gdb/ui-file.h b/gdb/ui-file.h
> index de24620e247..e045750c4ef 100644
> --- a/gdb/ui-file.h
> +++ b/gdb/ui-file.h
> @@ -19,6 +19,7 @@
> #ifndef UI_FILE_H
> #define UI_FILE_H
>
> +#include <list>
> #include <string>
> #include "ui-style.h"
>
> @@ -220,13 +221,102 @@ class string_file : public ui_file
> bool empty () const { return m_string.empty (); }
> void clear () { return m_string.clear (); }
>
> -private:
> +protected:
> /* The internal buffer. */
> std::string m_string;
>
> bool m_term_out;
> };
>
> +/* A string_file implementation that collects output on behalf of a
> + given ui_file. Provides access to the underlying ui_file so
> + that buffering can be selectively bypassed. */
> +
> +class buffer_file : public string_file
> +{
> +public:
> + explicit buffer_file (bool term_out, ui_file *stream)
> + : string_file (term_out), m_stream (stream)
I think you should write:
explicit buffer_file (ui_file *stream)
: string_file (stream->term_out ()),
m_stream (stream)
here as that would better reflect what we are actually trying to do.
But I'm not really sure why you are inheriting from string_file at all.
You do reference m_string in a few places, which is a member of
string_file, but, as far as I can tell, you never actually set
m_string -- I think you can delete all of your code that references
m_string, and nothing will change -- in which case, you can probably
just inherit from ui_file directly.
> + {
> + ++num_buffers;
> + }
> +
> + ~buffer_file ()
> + {
> + /* Reset ID_COUNTER if possible to help prevent overflow. */
> + if (--num_buffers == 0)
> + output_unit::id_counter = 0;
> + }
> +
> + /* Return the stream associated with this buffer_file. */
> + ui_file *get_stream ()
> + {
> + return m_stream;
> + }
> +
> + /* Record the wrap hint. When flushing the buffer, the underlying
> + ui_file's wrap_here will be called at the current place in the
> + output. */
> + void wrap_here (int indent) override;
> +
> + /* Flush collected output from this buffer_file as well as OTHER to their
> + underlying ui_files. Output is written to the underlying files in the
> + same order in which it was written to this buffer_file or OTHER. */
> + void flush_streams (buffer_file *other);
> +
> + /* Return true if the underlying stream is a tty. */
> + bool isatty () override;
> +
> + /* Record BUF and schedule it to be written to the underlying stream
> + during flush_streams. */
> + void write (const char *buf, long length_buf) override;
> +
> + /* Return a pointer to STREAM's underlying ui_file. Recursively called
> + until a non-buffer_file is found. */
> + static ui_file *get_unbuffered_stream (ui_file *stream);
> +
> + /* Counter indicating the number of buffer_files currently in use. */
> + static int num_buffers;
> +
> +private:
> +
> + /* The underlying output stream. */
> + ui_file *m_stream;
> +
> + struct output_unit
> + {
> + output_unit (std::string msg, int wrap_hint = -1);
> +
> + /* 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 buffer_file flush. */
> + int m_wrap_hint;
> +
> + /* ID for this output_unit. */
> + unsigned long m_id;
> +
> + /* Return true if OUT1 should be flushed before OUT2 and false if
> + otherwise. At least one of OUT1 and OUT1 should be non-nullptr. */
> + static bool compare (output_unit *out1, output_unit *out2);
> +
> + /* Counter used to generate output_unit IDs. IDs are unique across
> + all current output_units. IDs are used to ensure that output_units
> + across multiple buffer_files are flushed in the same order in which
> + they were written. See buffer_file::flush_streams. */
> + static unsigned long id_counter;
> + };
> +
> + /* Write one output_unit to the underlying stream. output_unit is
> + selected via FIFO and is removed from m_output_units after being
> + written. */
> + void flush_next_unit ();
> +
> + /* output_units scheduled to be flushed to the underlying output stream. */
> + std::list<output_unit> m_output_units;
Honestly, I think the approach of storing multiple output_unit lists
within different buffers, and then resolving the order later, is the
wrong approach.
I think a better approach would be to have some new object into which
all of the buffer_file objects store their output_unit objects. Thus,
there would be a single order defined by the output_unit order within
this single object.
I worry that the approach you have here only covers gdb_stdout and
gdb_stderr, so the order of things written to gdb_stdlog (or
gdb_stdtarg, or gdb_stdtargerr) will now be different. I can see that
the current approach is going to become harder as we try to deal with >2
streams, but an approach with a single buffer would, I think, resolve
this issue.
> +};
> +
> /* A ui_file implementation that maps directly onto <stdio.h>'s FILE.
> A stdio_file can either own its underlying file, or not. If it
> owns the file, then destroying the stdio_file closes the underlying
> diff --git a/gdb/ui-out.c b/gdb/ui-out.c
> index 9380630c320..14bb5068b4c 100644
> --- a/gdb/ui-out.c
> +++ b/gdb/ui-out.c
> @@ -799,6 +799,18 @@ ui_out::redirect (ui_file *outstream)
> do_redirect (outstream);
> }
>
> +void
> +ui_out::redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
> +{
> + do_redirect_to_buffer (stream, buf_file);
> +}
> +
> +void
> +ui_out::remove_buffers ()
> +{
> + do_remove_buffers ();
> +}
> +
> /* Test the flags against the mask given. */
> ui_out_flags
> ui_out::test_flags (ui_out_flags mask)
> @@ -871,3 +883,27 @@ ui_out::ui_out (ui_out_flags flags)
> ui_out::~ui_out ()
> {
> }
> +
> +ui_out_buffer_pop::ui_out_buffer_pop (ui_out *uiout)
> +: m_uiout (uiout),
> + m_stdout_buf (uiout->can_emit_style_escape (), gdb_stdout),
> + m_stderr_buf (uiout->can_emit_style_escape (), gdb_stderr),
I'm not sure about this. Using `uiout->can_emit_style_escape ()` here
is asking whatever stream the ui_out is pointing at whether it can
handle styling -- this might be the same or different to either
gdb_stdout, and/or gdb_stderr. If you see my earlier notes on the
buffer_file constructor, then the can_emit_style_escape calls here can
be dropped anyway.
> + m_prev_stdout (gdb_stdout),
> + m_prev_stderr (gdb_stderr)
> +{
> + m_uiout->redirect_to_buffer (gdb_stdout, &m_stdout_buf);
> + gdb_stdout = &m_stdout_buf;
> +
> + if (m_prev_stdout != m_prev_stderr)
> + {
> + m_uiout->redirect_to_buffer (gdb_stderr, &m_stderr_buf);
> + gdb_stderr = &m_stderr_buf;
> + }
> +}
> +
> +ui_out_buffer_pop::~ui_out_buffer_pop ()
> +{
> + m_uiout->remove_buffers ();
> + gdb_stdout = m_prev_stdout;
> + gdb_stderr = m_prev_stderr;
> +}
> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index ba5b1de68ab..964e6e3a8a4 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -262,6 +262,12 @@ class ui_out
> /* Redirect the output of a ui_out object temporarily. */
> void redirect (ui_file *outstream);
>
> + /* Redirect STREAM to BUF_FILE temporarily. */
> + void redirect_to_buffer (ui_file *stream, buffer_file *buf_file);
> +
> + /* Remove all buffer_files from this ui_out. */
> + void remove_buffers ();
> +
> ui_out_flags test_flags (ui_out_flags mask);
>
> /* HACK: Code in GDB is currently checking to see the type of ui_out
> @@ -360,6 +366,9 @@ class ui_out
> virtual void do_wrap_hint (int indent) = 0;
> virtual void do_flush () = 0;
> virtual void do_redirect (struct ui_file *outstream) = 0;
> + virtual void do_redirect_to_buffer (struct ui_file *stream,
> + buffer_file *buf_file) = 0;
> + virtual void do_remove_buffers () = 0;
>
> virtual void do_progress_start () = 0;
> virtual void do_progress_notify (const std::string &, const char *,
> @@ -470,4 +479,74 @@ class ui_out_redirect_pop
> struct ui_out *m_uiout;
> };
>
> +/* On construction, redirect gdb_stdout and gdb_stderr to buffer_files.
> + Replace occurances of gdb_stdout and gdb_stderr in M_UIOUT with these
> + buffer_files. On destruction restore M_UIOUT, gdb_stdout and gdb_stderr. */
> +
> +class ui_out_buffer_pop
> +{
> +public:
> + ui_out_buffer_pop (ui_out *uiout);
> +
> + ~ui_out_buffer_pop ();
> +
> + /* Flush buffered output to the underlying output streams. */
> + void flush ()
> + {
> + m_stdout_buf.flush_streams (&m_stderr_buf);
> + }
> +
> + ui_out_buffer_pop (const ui_out_buffer_pop &) = delete;
> + ui_out_buffer_pop &operator= (const ui_out_buffer_pop &) = delete;
> +
> +private:
> + /* ui_out being temporarily redirected. */
> + struct ui_out *m_uiout;
> +
> + /* Buffer to which stdout is temporarily redirected to for the lifetime
> + of this object. */
> + buffer_file m_stdout_buf;
> +
> + /* Buffer to which stderr is temporarily redirected to for the lifetime
> + of this object. */
> + buffer_file m_stderr_buf;
> +
> + /* Original gdb_stdout at the time of this object's construction. */
> + ui_file *m_prev_stdout;
> +
> + /* Original gdb_stdout at the time of this object's construction. */
> + ui_file *m_prev_stderr;
> +};
> +
> +/* Redirect output for the duration of FUNC. */
> +
> +template<typename F, typename... Arg>
> +void
> +do_with_buffered_output (F func, ui_out *uiout, Arg... args)
> +{
> + ui_out_buffer_pop buf (uiout);
> +
> + try
> + {
> + func (uiout, std::forward<Arg> (args)...);
> + }
> + catch (gdb_exception &ex)
> + {
> + /* Ideally flush would be called in the destructor of buf,
> + however flushing might cause an exception to be thrown.
> + Catch it and ensure the first exception propagates. */
> + try
> + {
> + buf.flush ();
> + }
> + catch (const gdb_exception &ignore)
> + {
> + }
> +
> + throw_exception (std::move (ex));
> + }
> +
> + /* Try was successful. Let any further exceptions propagate. */
> + buf.flush ();
> +}
> #endif /* UI_OUT_H */
> --
> 2.40.1
While I was thinking about how I would approach the file buffering
problem I hacked together the code below. This patch applies on top of
your existing series, but is really an alternative to this patch.
I include this code here _only_ as a mechanism to better explain my
idea. This is not intended as a drop in replacement for what you've
written. This code still includes debug output, and has a weird
structure which exists only to reduce the changes needed in .h files
(thus speeding up repeated compiles). This is intended to aid in a
conversation, rather than be a suggestion for "better code".
Thanks,
Andrew
---
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index ae4e650e7eb..130ccee9abc 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/mi/mi-out.h b/gdb/mi/mi-out.h
index ee089abfdef..f010da59f7d 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-mi.c b/gdb/python/py-mi.c
index 0d672155265..821f393491f 100644
--- a/gdb/python/py-mi.c
+++ b/gdb/python/py-mi.c
@@ -55,6 +55,9 @@ class py_ui_out : public ui_out
return current ().obj.release ();
}
+ ui_file *current_stream () const override
+ { return nullptr; }
+
protected:
void do_progress_end () override { }
diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index dc883a33f33..5099bc18bc9 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -323,17 +323,14 @@ buffer_file::flush_streams (buffer_file *other)
}
}
+extern ui_file *get_unbuffered (ui_file * file);
+
/* See ui-file.h. */
ui_file *
buffer_file::get_unbuffered_stream (ui_file *stream)
{
- buffer_file *buf = dynamic_cast<buffer_file *> (stream);
-
- if (buf == nullptr)
- return stream;
-
- return get_unbuffered_stream (buf->m_stream);
+ return get_unbuffered (stream);
}
/* See ui-file.h. */
diff --git a/gdb/ui-out.c b/gdb/ui-out.c
index 14bb5068b4c..4c10dc6358d 100644
--- a/gdb/ui-out.c
+++ b/gdb/ui-out.c
@@ -27,6 +27,7 @@
#include "gdbsupport/format.h"
#include "cli/cli-style.h"
#include "diagnostics.h"
+#include "ui-file.h"
#include <vector>
#include <memory>
@@ -907,3 +908,204 @@ ui_out_buffer_pop::~ui_out_buffer_pop ()
gdb_stdout = m_prev_stdout;
gdb_stderr = m_prev_stderr;
}
+
+buffer_group::output_unit::output_unit (std::string msg, int wrap_hint)
+ : m_msg (msg),
+ m_wrap_hint (wrap_hint)
+{ /* Nothing. */ }
+
+void
+buffer_group::output_unit::flush () const
+{
+ stream->puts (m_msg.c_str ());
+
+ if (m_wrap_hint >= 0)
+ stream->wrap_here (m_wrap_hint);
+}
+
+void
+buffer_group::write (const char *buf, long length_buf, ui_file *stream)
+{
+ if (getenv ("APB_DEBUG") != nullptr)
+ fprintf (stderr, "APB: buffer_group::write '%*s'\n",
+ (int) length_buf, buf);
+
+ /* 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 ().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 ().stream = stream;
+ }
+ prev = cur + 1;
+ }
+}
+
+void
+buffer_group::wrap_here (int indent, ui_file *stream)
+{
+ m_buffered_output.emplace_back ("", indent);
+ m_buffered_output.back ().stream = stream;
+}
+
+struct buffering_file : public ui_file
+{
+ buffering_file (buffer_group *group, ui_file *stream)
+ : m_group (group),
+ m_stream (stream)
+ { /* Nothing. */ }
+
+ ui_file *
+ stream () const
+ {
+ return m_stream;
+ }
+
+ /* ... */
+ void write (const char *buf, long length_buf) override
+ {
+ m_group->write (buf, length_buf, m_stream);
+ }
+
+ /* ... */
+ void wrap_here (int indent) override
+ {
+ m_group->wrap_here (indent, m_stream);
+ }
+
+ /* ... */
+ bool isatty () override
+ {
+ return m_stream->isatty ();
+ }
+
+ /* true if ANSI escapes can be used on STREAM. */
+ bool can_emit_style_escape () override
+ {
+ return m_stream->can_emit_style_escape ();
+ }
+
+ void flush () override
+ {
+ if (getenv ("APB_DEBUG") != nullptr)
+ fprintf (stderr, "APB: flush()\n");
+ return m_stream->flush ();
+ }
+
+private:
+ buffer_group *m_group;
+ ui_file *m_stream;
+};
+
+extern ui_file *get_unbuffered (ui_file * file);
+
+ui_file *
+get_unbuffered (ui_file * stream)
+{
+ buffering_file *buf = dynamic_cast<buffering_file *> (stream);
+
+ if (buf == nullptr)
+ return stream;
+
+ return buf->stream ();
+}
+
+
+struct buffered_streams
+{
+ buffered_streams (buffer_group *group)
+ : 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)
+ {
+ if (getenv ("APB_DEBUG") != nullptr)
+ fprintf (stderr, "APB: Creating a new buffered_streams object: %p\n", this);
+
+ 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_uiout.emplace (group, stream);
+ current_uiout->redirect (&(*m_buffered_uiout));
+ }
+
+ m_buffers_in_place = true;
+ };
+
+ ~buffered_streams ()
+ {
+ this->remove_buffers ();
+
+ if (getenv ("APB_DEBUG") != nullptr)
+ fprintf (stderr, "APB: Deleted a buffered_streams object: %p\n", this);
+ }
+
+ void 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_uiout.has_value ())
+ current_uiout->redirect (nullptr);
+ }
+
+private:
+
+ bool m_buffers_in_place;
+
+ 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;
+
+ gdb::optional<buffering_file> m_buffered_uiout;
+};
+
+buffer_group::buffer_group ()
+ : m_buffered_streams (new buffered_streams (this))
+{ /* Nothing. */ }
+
+buffer_group::~buffer_group ()
+{
+}
+
+void
+buffer_group::flush () const
+{
+ m_buffered_streams->remove_buffers ();
+
+ if (getenv ("APB_DEBUG") != nullptr)
+ fprintf (stderr, "APB: Flushing all buffered output -----\n");
+
+ for (const output_unit &ou : m_buffered_output)
+ ou.flush ();
+
+ if (getenv ("APB_DEBUG") != nullptr)
+ fprintf (stderr, "APB: Flushing completed -----\n");
+}
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 964e6e3a8a4..4147e0bd758 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -284,6 +284,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
{
@@ -518,13 +521,54 @@ class ui_out_buffer_pop
ui_file *m_prev_stderr;
};
+struct buffered_streams;
+
+struct buffer_group
+{
+ buffer_group ();
+
+ ~buffer_group ();
+
+ void flush () const;
+
+ void write (const char *buf, long length_buf, ui_file *stream);
+
+ void wrap_here (int indent, ui_file *stream);
+
+private:
+
+ struct output_unit
+ {
+ output_unit (std::string msg, int wrap_hint = -1);
+
+ void flush () const;
+
+ ui_file *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 buffer_file flush. */
+ int m_wrap_hint;
+ };
+
+ std::vector<output_unit> m_buffered_output;
+
+ std::unique_ptr<buffered_streams> m_buffered_streams;
+};
+
+extern void buffer_everything (buffer_group *g);
+
/* Redirect output for the duration of FUNC. */
template<typename F, typename... Arg>
void
do_with_buffered_output (F func, ui_out *uiout, Arg... args)
{
- ui_out_buffer_pop buf (uiout);
+ buffer_group g;
+
+ //ui_out_buffer_pop buf (uiout);
try
{
@@ -537,7 +581,8 @@ do_with_buffered_output (F func, ui_out *uiout, Arg... args)
Catch it and ensure the first exception propagates. */
try
{
- buf.flush ();
+ //buf.flush ();
+ g.flush ();
}
catch (const gdb_exception &ignore)
{
@@ -547,6 +592,7 @@ do_with_buffered_output (F func, ui_out *uiout, Arg... args)
}
/* Try was successful. Let any further exceptions propagate. */
- buf.flush ();
+ //buf.flush ();
+ g.flush ();
}
#endif /* UI_OUT_H */
Hi Andrew,
Thanks for reviewing this patch series.
On Wed, Aug 2, 2023 at 11:50 AM Andrew Burgess <aburgess@redhat.com> wrote:
>
> Aaron Merey <amerey@redhat.com> writes:
>
> > +/* If STREAM is in m_streams, then replace it with BUF_FILE. */
> > +
> > +void
> > +cli_ui_out::do_redirect_to_buffer (ui_file *stream, buffer_file *buf_file)
> > +{
> > + for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
> > + if (*it == stream)
> > + *it = buf_file;
> > +}
>
> I would love to understand more about why the existing redirect
> mechanism doesn't work for you. As I understand it, the existing
> redirect adds a new ui_file object to the current_ui's stream stack, and
> all output after that point will be sent to that stream.
>
> In contrast, your approach seems to indicate you're seeing writes to
> lower level streams ... which seems ... odd.
Initially I used the existing redirect but switched to this
implementation to help isolate logic specifically for the purpose
of redirecting to buffer_files (mi_ui_out::do_redirect_to_buffer
and do_remove_buffers contained more of this than cli_ui_out::
do_redirect_to_buffer).
I also went with this design out of a concern that during buffering
another component of gdb either pushes a new stream to current_ui's
stream stack or else pop the buffer_file off the stack. If these
issues will not happen then the existing redirect should be fine.
> While I was thinking about how I would approach the file buffering
> problem I hacked together the code below. This patch applies on top of
> your existing series, but is really an alternative to this patch.
>
> I include this code here _only_ as a mechanism to better explain my
> idea. This is not intended as a drop in replacement for what you've
> written. This code still includes debug output, and has a weird
> structure which exists only to reduce the changes needed in .h files
> (thus speeding up repeated compiles). This is intended to aid in a
> conversation, rather than be a suggestion for "better code".
I like this implementation and it didn't introduce any regressions
that I could find. We really should be buffering all gdb_std* streams
and your approach simplifies how to address this. I will integrate
your code into the rest of the patch and remove the debug messages, etc.
Aaron
@@ -263,6 +263,31 @@ cli_ui_out::do_redirect (ui_file *outstream)
m_streams.pop_back ();
}
+/* If STREAM is in m_streams, then replace it with BUF_FILE. */
+
+void
+cli_ui_out::do_redirect_to_buffer (ui_file *stream, buffer_file *buf_file)
+{
+ for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
+ if (*it == stream)
+ *it = buf_file;
+}
+
+/* Replace any buffer_files in m_streams with the buffer_file's
+ underlying stream. */
+
+void
+cli_ui_out::do_remove_buffers ()
+{
+ for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
+ {
+ buffer_file *buf = dynamic_cast<buffer_file *> (*it);
+
+ if (buf != nullptr)
+ *it = buf->get_stream ();
+ }
+}
+
/* Initialize a progress update to be displayed with
cli_ui_out::do_progress_notify. */
@@ -299,7 +324,8 @@ 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
+ = buffer_file::get_unbuffered_stream (m_streams.back ());
cli_progress_info &info (m_progress_info.back ());
if (chars_per_line > MAX_CHARS_PER_LINE)
@@ -384,7 +410,8 @@ 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
+ = buffer_file::get_unbuffered_stream (m_streams.back ());
int chars_per_line = get_chars_per_line ();
scoped_restore save_pagination
@@ -413,10 +440,11 @@ void
cli_ui_out::do_progress_end ()
{
struct ui_file *stream = m_streams.back ();
- m_progress_info.pop_back ();
if (stream->isatty ())
clear_progress_notify ();
+
+ m_progress_info.pop_back ();
}
/* local functions */
@@ -71,6 +71,9 @@ class cli_ui_out : public ui_out
virtual void do_wrap_hint (int indent) override;
virtual void do_flush () override;
virtual void do_redirect (struct ui_file *outstream) override;
+ virtual void do_redirect_to_buffer (struct ui_file *stream,
+ buffer_file *buf_file) override;
+ virtual void do_remove_buffers () override;
virtual void do_progress_start () override;
virtual void do_progress_notify (const std::string &, const char *,
@@ -168,7 +168,8 @@ progressfn (debuginfod_client *c, long cur, long total)
if (check_quit_flag ())
{
- gdb_printf ("Cancelling download of %s %s...\n",
+ ui_file *outstream = buffer_file::get_unbuffered_stream (gdb_stdout);
+ gdb_printf (outstream, _("Cancelling download of %s %s...\n"),
data->desc, styled_fname.c_str ());
return 1;
}
@@ -284,10 +285,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 = buffer_file::get_unbuffered_stream (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 */
@@ -8706,10 +8706,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;
@@ -8738,6 +8738,17 @@ 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<void (ui_out *, bool)>
+ (do_print_stop_event, uiout, displays);
+}
+
/* See infrun.h. */
void
@@ -194,6 +194,43 @@ mi_ui_out::do_redirect (ui_file *outstream)
m_streams.pop_back ();
}
+/* If STREAM is in m_streams, then replace it with BUF_FILE. */
+
+void
+mi_ui_out::do_redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
+{
+ for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
+ {
+ if (*it != stream)
+ continue;
+
+ string_file *sstream = dynamic_cast<string_file *> (*it);
+ if (sstream != nullptr)
+ {
+ /* Forward the contents of SSTREAM to BUF_FILE. */
+ buf_file->write (sstream->data (), sstream->size ());
+ sstream->clear ();
+ }
+
+ *it = buf_file;
+ }
+}
+
+/* Replace any buffer_files in m_streams with the buffer_file's
+ underlying stream. */
+
+void
+mi_ui_out::do_remove_buffers ()
+{
+ for (auto it = m_streams.begin (); it != m_streams.end (); ++it)
+ {
+ buffer_file *buf = dynamic_cast<buffer_file *> (*it);
+
+ if (buf != nullptr)
+ *it = buf->get_stream ();
+ }
+}
+
void
mi_ui_out::field_separator ()
{
@@ -79,6 +79,9 @@ class mi_ui_out : public ui_out
virtual void do_wrap_hint (int indent) override;
virtual void do_flush () override;
virtual void do_redirect (struct ui_file *outstream) override;
+ virtual void do_redirect_to_buffer (struct ui_file *stream,
+ buffer_file *buf_file) override;
+ virtual void do_remove_buffers () override;
virtual bool do_is_mi_like_p () const override
{ return true; }
@@ -62,6 +62,9 @@ class py_ui_out : public ui_out
void do_progress_notify (const std::string &, const char *, double, double)
override
{ }
+ void do_redirect_to_buffer (struct ui_file *, buffer_file *) override
+ { }
+ void do_remove_buffers () override { }
void do_table_begin (int nbrofcols, int nr_rows, const char *tblid) override
{
@@ -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 (gdb::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,23 @@ 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)
+{
+ using ftype = void (ui_out *, const frame_print_options &, frame_info_ptr,
+ int, enum print_what, int, int);
+
+ do_with_buffered_output<ftype> (do_print_frame_info, current_uiout,
+ fp_opts, frame, print_level, print_what,
+ print_args, set_current_sal);
+}
+
/* See stack.h. */
void
@@ -1309,13 +1327,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;
@@ -1040,6 +1040,106 @@ 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;
+
+ 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)
+
+{
+ using ftype = void (ui_out *, const char *, int, int, int,
+ int, thread_info *, thread_info *);
+
+ do_with_buffered_output<ftype>
+ (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. */
@@ -1123,82 +1223,12 @@ 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;
- 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
@@ -234,6 +234,142 @@ string_file::can_emit_style_escape ()
+/* See ui-file.h. */
+
+void
+buffer_file::wrap_here (int indent)
+{
+ m_output_units.emplace_back (m_string, indent);
+ m_string.clear ();
+}
+
+/* See ui-file.h. */
+
+int buffer_file::num_buffers = 0;
+
+/* See ui-file.h. */
+
+unsigned long buffer_file::output_unit::id_counter = 0;
+
+/* See ui-file.h. */
+
+void
+buffer_file::write (const char *buf, long length_buf)
+{
+ /* 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);
+
+ /* Prepend a partial line if one was already written to
+ this buffer_file. */
+ if (!m_string.empty ())
+ {
+ msg.insert (0, m_string);
+ m_string.clear ();
+ }
+
+ m_output_units.emplace_back (msg);
+ prev = cur + 1;
+ }
+}
+
+/* See ui-file.h. */
+
+void
+buffer_file::flush_next_unit ()
+{
+ if (m_output_units.empty ())
+ return;
+
+ output_unit unit = std::move (m_output_units.front ());
+
+ m_output_units.pop_front ();
+ m_stream->puts (unit.m_msg.c_str ());
+
+ if (unit.m_wrap_hint >= 0)
+ m_stream->wrap_here (unit.m_wrap_hint);
+}
+
+/* See ui-file.h. */
+
+void
+buffer_file::flush_streams (buffer_file *other)
+{
+ /* Make sure all string contents of these buffer_files are
+ contained in their output_units. */
+ this->wrap_here (-1);
+ other->wrap_here (-1);
+
+ while (!this->m_output_units.empty ()
+ || !other->m_output_units.empty ())
+ {
+ output_unit *out1 = nullptr;
+ output_unit *out2 = nullptr;
+
+ if (!this->m_output_units.empty ())
+ out1 = &this->m_output_units.front ();
+ if (!other->m_output_units.empty ())
+ out2 = &other->m_output_units.front ();
+
+ bool flush_this_next = output_unit::compare (out1, out2);
+
+ /* out1 or out2 will be invalidated by flush_next_unit. */
+ if (flush_this_next)
+ this->flush_next_unit ();
+ else
+ other->flush_next_unit ();
+ }
+}
+
+/* See ui-file.h. */
+
+ui_file *
+buffer_file::get_unbuffered_stream (ui_file *stream)
+{
+ buffer_file *buf = dynamic_cast<buffer_file *> (stream);
+
+ if (buf == nullptr)
+ return stream;
+
+ return get_unbuffered_stream (buf->m_stream);
+}
+
+/* See ui-file.h. */
+
+bool
+buffer_file::isatty ()
+{
+ return m_stream->isatty ();
+}
+
+/* See ui-file.h. */
+
+bool
+buffer_file::output_unit::compare (output_unit *out1,
+ output_unit *out2)
+{
+ gdb_assert (out1 != nullptr || out2 != nullptr);
+
+ if (out1 != nullptr && out2 == nullptr)
+ return true;
+ if (out1 == nullptr && out2 != nullptr)
+ return false;
+
+ return out1->m_id < out2->m_id;
+}
+
+buffer_file::output_unit::output_unit (std::string msg, int wrap_hint)
+ : m_msg (msg), m_wrap_hint (wrap_hint), m_id (++id_counter)
+{
+ /* IDs are assigned starting at 1. 0 indicates ID_COUNTER overflow. */
+ if (m_id == 0)
+ error (_("Message ID overflow"));
+}
+
+
+
stdio_file::stdio_file (FILE *file, bool close_p)
{
set_stream (file);
@@ -19,6 +19,7 @@
#ifndef UI_FILE_H
#define UI_FILE_H
+#include <list>
#include <string>
#include "ui-style.h"
@@ -220,13 +221,102 @@ class string_file : public ui_file
bool empty () const { return m_string.empty (); }
void clear () { return m_string.clear (); }
-private:
+protected:
/* The internal buffer. */
std::string m_string;
bool m_term_out;
};
+/* A string_file implementation that collects output on behalf of a
+ given ui_file. Provides access to the underlying ui_file so
+ that buffering can be selectively bypassed. */
+
+class buffer_file : public string_file
+{
+public:
+ explicit buffer_file (bool term_out, ui_file *stream)
+ : string_file (term_out), m_stream (stream)
+ {
+ ++num_buffers;
+ }
+
+ ~buffer_file ()
+ {
+ /* Reset ID_COUNTER if possible to help prevent overflow. */
+ if (--num_buffers == 0)
+ output_unit::id_counter = 0;
+ }
+
+ /* Return the stream associated with this buffer_file. */
+ ui_file *get_stream ()
+ {
+ return m_stream;
+ }
+
+ /* Record the wrap hint. When flushing the buffer, the underlying
+ ui_file's wrap_here will be called at the current place in the
+ output. */
+ void wrap_here (int indent) override;
+
+ /* Flush collected output from this buffer_file as well as OTHER to their
+ underlying ui_files. Output is written to the underlying files in the
+ same order in which it was written to this buffer_file or OTHER. */
+ void flush_streams (buffer_file *other);
+
+ /* Return true if the underlying stream is a tty. */
+ bool isatty () override;
+
+ /* Record BUF and schedule it to be written to the underlying stream
+ during flush_streams. */
+ void write (const char *buf, long length_buf) override;
+
+ /* Return a pointer to STREAM's underlying ui_file. Recursively called
+ until a non-buffer_file is found. */
+ static ui_file *get_unbuffered_stream (ui_file *stream);
+
+ /* Counter indicating the number of buffer_files currently in use. */
+ static int num_buffers;
+
+private:
+
+ /* The underlying output stream. */
+ ui_file *m_stream;
+
+ struct output_unit
+ {
+ output_unit (std::string msg, int wrap_hint = -1);
+
+ /* 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 buffer_file flush. */
+ int m_wrap_hint;
+
+ /* ID for this output_unit. */
+ unsigned long m_id;
+
+ /* Return true if OUT1 should be flushed before OUT2 and false if
+ otherwise. At least one of OUT1 and OUT1 should be non-nullptr. */
+ static bool compare (output_unit *out1, output_unit *out2);
+
+ /* Counter used to generate output_unit IDs. IDs are unique across
+ all current output_units. IDs are used to ensure that output_units
+ across multiple buffer_files are flushed in the same order in which
+ they were written. See buffer_file::flush_streams. */
+ static unsigned long id_counter;
+ };
+
+ /* Write one output_unit to the underlying stream. output_unit is
+ selected via FIFO and is removed from m_output_units after being
+ written. */
+ void flush_next_unit ();
+
+ /* output_units scheduled to be flushed to the underlying output stream. */
+ std::list<output_unit> m_output_units;
+};
+
/* A ui_file implementation that maps directly onto <stdio.h>'s FILE.
A stdio_file can either own its underlying file, or not. If it
owns the file, then destroying the stdio_file closes the underlying
@@ -799,6 +799,18 @@ ui_out::redirect (ui_file *outstream)
do_redirect (outstream);
}
+void
+ui_out::redirect_to_buffer (struct ui_file *stream, buffer_file *buf_file)
+{
+ do_redirect_to_buffer (stream, buf_file);
+}
+
+void
+ui_out::remove_buffers ()
+{
+ do_remove_buffers ();
+}
+
/* Test the flags against the mask given. */
ui_out_flags
ui_out::test_flags (ui_out_flags mask)
@@ -871,3 +883,27 @@ ui_out::ui_out (ui_out_flags flags)
ui_out::~ui_out ()
{
}
+
+ui_out_buffer_pop::ui_out_buffer_pop (ui_out *uiout)
+: m_uiout (uiout),
+ m_stdout_buf (uiout->can_emit_style_escape (), gdb_stdout),
+ m_stderr_buf (uiout->can_emit_style_escape (), gdb_stderr),
+ m_prev_stdout (gdb_stdout),
+ m_prev_stderr (gdb_stderr)
+{
+ m_uiout->redirect_to_buffer (gdb_stdout, &m_stdout_buf);
+ gdb_stdout = &m_stdout_buf;
+
+ if (m_prev_stdout != m_prev_stderr)
+ {
+ m_uiout->redirect_to_buffer (gdb_stderr, &m_stderr_buf);
+ gdb_stderr = &m_stderr_buf;
+ }
+}
+
+ui_out_buffer_pop::~ui_out_buffer_pop ()
+{
+ m_uiout->remove_buffers ();
+ gdb_stdout = m_prev_stdout;
+ gdb_stderr = m_prev_stderr;
+}
@@ -262,6 +262,12 @@ class ui_out
/* Redirect the output of a ui_out object temporarily. */
void redirect (ui_file *outstream);
+ /* Redirect STREAM to BUF_FILE temporarily. */
+ void redirect_to_buffer (ui_file *stream, buffer_file *buf_file);
+
+ /* Remove all buffer_files from this ui_out. */
+ void remove_buffers ();
+
ui_out_flags test_flags (ui_out_flags mask);
/* HACK: Code in GDB is currently checking to see the type of ui_out
@@ -360,6 +366,9 @@ class ui_out
virtual void do_wrap_hint (int indent) = 0;
virtual void do_flush () = 0;
virtual void do_redirect (struct ui_file *outstream) = 0;
+ virtual void do_redirect_to_buffer (struct ui_file *stream,
+ buffer_file *buf_file) = 0;
+ virtual void do_remove_buffers () = 0;
virtual void do_progress_start () = 0;
virtual void do_progress_notify (const std::string &, const char *,
@@ -470,4 +479,74 @@ class ui_out_redirect_pop
struct ui_out *m_uiout;
};
+/* On construction, redirect gdb_stdout and gdb_stderr to buffer_files.
+ Replace occurances of gdb_stdout and gdb_stderr in M_UIOUT with these
+ buffer_files. On destruction restore M_UIOUT, gdb_stdout and gdb_stderr. */
+
+class ui_out_buffer_pop
+{
+public:
+ ui_out_buffer_pop (ui_out *uiout);
+
+ ~ui_out_buffer_pop ();
+
+ /* Flush buffered output to the underlying output streams. */
+ void flush ()
+ {
+ m_stdout_buf.flush_streams (&m_stderr_buf);
+ }
+
+ ui_out_buffer_pop (const ui_out_buffer_pop &) = delete;
+ ui_out_buffer_pop &operator= (const ui_out_buffer_pop &) = delete;
+
+private:
+ /* ui_out being temporarily redirected. */
+ struct ui_out *m_uiout;
+
+ /* Buffer to which stdout is temporarily redirected to for the lifetime
+ of this object. */
+ buffer_file m_stdout_buf;
+
+ /* Buffer to which stderr is temporarily redirected to for the lifetime
+ of this object. */
+ buffer_file m_stderr_buf;
+
+ /* Original gdb_stdout at the time of this object's construction. */
+ ui_file *m_prev_stdout;
+
+ /* Original gdb_stdout at the time of this object's construction. */
+ ui_file *m_prev_stderr;
+};
+
+/* Redirect output for the duration of FUNC. */
+
+template<typename F, typename... Arg>
+void
+do_with_buffered_output (F func, ui_out *uiout, Arg... args)
+{
+ ui_out_buffer_pop buf (uiout);
+
+ try
+ {
+ func (uiout, std::forward<Arg> (args)...);
+ }
+ catch (gdb_exception &ex)
+ {
+ /* Ideally flush would be called in the destructor of buf,
+ however flushing might cause an exception to be thrown.
+ Catch it and ensure the first exception propagates. */
+ try
+ {
+ buf.flush ();
+ }
+ catch (const gdb_exception &ignore)
+ {
+ }
+
+ throw_exception (std::move (ex));
+ }
+
+ /* Try was successful. Let any further exceptions propagate. */
+ buf.flush ();
+}
#endif /* UI_OUT_H */