Re: [PATCH v3] gdb: Improve debuginfod progress updates

[Aaron Merey <amerey@redhat.com>]

Hi Tom,

Thanks for the review.

On Fri, Mar 4, 2022 at 1:17 PM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> The new function build_message will try to fit as much of the
> Aaron> untruncated message as possible.  For example if the original
> Aaron> message is:
> Aaron>   Downloading XX MB separate debug info for /aa/bb/cc/dd/ee
>
> Instead of this, which seems like it can hide useful info from the user,
> what do you think about a "cylon"-style display:
>
> Downloading mumble.so:
> [      #             ]
>
> ... where the '#' moves back and forth inside the [].

I'd like to restrict the update messages to a single line so that an
entire message can be overwritten with \r.  Then when many downloads
occur we avoid filling the terminal with "Downloading XY MB separate
debuginfo for libxyz" messages.

One drawback of using \r is the output becomes garbled if an update message
exceeds the width of the terminal.  This creates the need for message
truncation which can hide useful information, like you said.

However the important parts of the update message are present until the
terminal is very narrow (usually <80 chars).  And if a user wants to ensure
they see all the details even with a narrow terminal, 'set debuginfod
verbose 2' will print a persistent, untruncated message for each downloaded
file regardless of terminal width:

    Retrieved XY MB separate debug info for /usr/lib/libxyz.so

> Aaron>    if (!stream->isatty ())
> Aaron>      {
> Aaron> -      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
> Aaron> +      fprintf_unfiltered (stream, "%s\n", info.name.c_str ());
>
> I suspect the flush can be removed here, it's normally only needed when
> the previous print doesn't end in a newline.

Fixed.

> Aaron> +      info.state = should_print ? progress_update::START
> Aaron> +                                : progress_update::NO_PRINT;
>
> gdb style is to parenthesize the RHS of the assignment in a case like
> this; something like:
>
>       info.state = (should_print
>                     ? progress_update::START
>                     : progress_update::NO_PRINT);

Fixed.

> Aaron> +/* Get the current state of the most recent progress update.  */
> Aaron> +
> Aaron> +cli_ui_out::progress_update::state
> Aaron> +cli_ui_out::get_progress_state ()
> Aaron> +{
>
> I didn't really get this part of the design.
>
> It seems to me that the debuginfod support code just wants to notify the
> user of some progress.  It might know how much data to expect, or it
> might not -- so when first starting the notification, it seems like it
> can just explain this.  After that, it could just say "got xyz", and the
> progress meter itself can be responsible for all state changes, UI
> display, etc.  That is, I don't understand why this needs to be exported
> from the cli-out.

Yes I think this can be reworked to avoid exporting the state information.
Although it's useful to be able to check for the WORKING state in order
to return early and avoid calling any update_progress_* functions.  This
is used when !isatty or interpreter=mi.  In these cases we want only a
single message to be printed when the progress_meter is created.  But
these conditions can be checked instead of the state.  I'll change this.

>
> Aaron> +      if (data->progress.has_value ())
> Aaron> +        data->progress.reset ();
>
> In retrospect I also don't understand why the progress meter is even an
> optional.  It seems like one could be made unconditionally and then the
> display (or not) left to the ui-out implementation.

The meter sometimes prints a progress message when it's created but this
could be done through the update_progress_* functions.  Maybe this change
should be implemented in another patch.

> Aaron> +  else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT)
> Aaron> +    printf_filtered (_("Download failed: %s. " \
> Aaron> +                       "Continuing without %s %s.\n"),
> Aaron> +                     safe_strerror (-fd), data.desc,
> Aaron> +                     styled_fname.c_str ());
>
> It seems like a download failure should be reported even in non-verbose
> mode.

Fixed.

> Aaron> +void
> Aaron> +mi_ui_out::do_progress_start (const std::string &name, bool should_print)
>
> I tend to doubt that MI should be changed.
>
> Technically MI does have a progress notification approach, see
> mi_load_progress.  I don't know if anything MI consumer actually uses
> this, though, and so I'm not sure if it makes sense to try to wire this
> up to debuginfo downloads.

These MI implementations are the easiest way to see progress updates when
using gdb+debuginfod with IDEs, for instance.  Otherwise I think each IDE
would have to learn how to parse and display the mi_load_progress output,
preferably in a way that's consistent with the CLI progress messages.

Aaron