[PING*3,v4] gdb: Improve debuginfod progress updates

  Hi,

I made a couple small adjustments to this patch to ensure it builds
on the main branch.  Otherwise the original v4 patch follow:

v3 can be found here:
https://sourceware.org/pipermail/gdb-patches/2022-February/185798.html

Changes from v3 include a simplified progress update API and display.
Truncation of the progress update message has been removed as well.

If the download size is known, a progress bar is displayed along with
the percentage of completion and the total download size.

  Downloading separate debug info for /lib/libxyz.so
  [############                                      ]  25% (10.01 MB)

If the download size is not known, a progress indicator is displayed
with a ticker ("###") that moves across the screen at a rate of 1 tick
every 0.5 seconds.

  Downloading separate debug info for /lib/libxyz.so
  [         ###                                                      ]

If the output stream is not a tty, batch mode is enabled or the screen
is too narrow then only a static description of the download is printed.
No bar or ticker is displayed.

  Downloading separate debug info for /lib/libxyz.so...

In any case, if the size of the download is known at the time the
description is printed then it will be included in the description.

  Downloading 10.01 MB separate debug info for /lib/libxyz.so...
---
 gdb/cli-out.c            | 194 +++++++++++++++++++++++++--------------
 gdb/cli-out.h            |  40 +++-----
 gdb/debuginfod-support.c | 130 +++++++++++++++++---------
 gdb/mi/mi-out.c          |  28 ++++++
 gdb/mi/mi-out.h          |  21 +++--
 gdb/ui-out.h             |  53 +++++++----
 6 files changed, 305 insertions(+), 161 deletions(-)
  

On Wed, Oct 12, 2022 at 8:54 PM Aaron Merey via Gdb-patches <
gdb-patches@sourceware.org> wrote:

> Hi,
>
> I made a couple small adjustments to this patch to ensure it builds
> on the main branch.  Otherwise the original v4 patch follow:
>
> v3 can be found here:
> https://sourceware.org/pipermail/gdb-patches/2022-February/185798.html
>
> Changes from v3 include a simplified progress update API and display.
> Truncation of the progress update message has been removed as well.
>
> If the download size is known, a progress bar is displayed along with
> the percentage of completion and the total download size.
>
>   Downloading separate debug info for /lib/libxyz.so
>   [############                                      ]  25% (10.01 MB)
>
> If the download size is not known, a progress indicator is displayed
> with a ticker ("###") that moves across the screen at a rate of 1 tick
> every 0.5 seconds.
>
>   Downloading separate debug info for /lib/libxyz.so
>   [         ###                                                      ]
>
> I think this version addresses all the concernes from the previous
> iterations and I
>
confirm it applies on top of the current master cleanly, it should be good
to go.
  

Ping

Thanks,
Aaron

On Wed, Oct 12, 2022 at 2:53 PM Aaron Merey <amerey@redhat.com> wrote:
>
> Hi,
>
> I made a couple small adjustments to this patch to ensure it builds
> on the main branch.  Otherwise the original v4 patch follow:
>
> v3 can be found here:
> https://sourceware.org/pipermail/gdb-patches/2022-February/185798.html
>
> Changes from v3 include a simplified progress update API and display.
> Truncation of the progress update message has been removed as well.
>
> If the download size is known, a progress bar is displayed along with
> the percentage of completion and the total download size.
>
>   Downloading separate debug info for /lib/libxyz.so
>   [############                                      ]  25% (10.01 MB)
>
> If the download size is not known, a progress indicator is displayed
> with a ticker ("###") that moves across the screen at a rate of 1 tick
> every 0.5 seconds.
>
>   Downloading separate debug info for /lib/libxyz.so
>   [         ###                                                      ]
>
> If the output stream is not a tty, batch mode is enabled or the screen
> is too narrow then only a static description of the download is printed.
> No bar or ticker is displayed.
>
>   Downloading separate debug info for /lib/libxyz.so...
>
> In any case, if the size of the download is known at the time the
> description is printed then it will be included in the description.
>
>   Downloading 10.01 MB separate debug info for /lib/libxyz.so...
> ---
>  gdb/cli-out.c            | 194 +++++++++++++++++++++++++--------------
>  gdb/cli-out.h            |  40 +++-----
>  gdb/debuginfod-support.c | 130 +++++++++++++++++---------
>  gdb/mi/mi-out.c          |  28 ++++++
>  gdb/mi/mi-out.h          |  21 +++--
>  gdb/ui-out.h             |  53 +++++++----
>  6 files changed, 305 insertions(+), 161 deletions(-)
>
> diff --git a/gdb/cli-out.c b/gdb/cli-out.c
> index fdbed6f5e91..1cc9143980e 100644
> --- a/gdb/cli-out.c
> +++ b/gdb/cli-out.c
> @@ -26,6 +26,7 @@
>  #include "completer.h"
>  #include "readline/readline.h"
>  #include "cli/cli-style.h"
> +#include "top.h"
>
>  /* These are the CLI output functions */
>
> @@ -262,105 +263,164 @@ cli_ui_out::do_redirect (ui_file *outstream)
>      m_streams.pop_back ();
>  }
>
> -/* The cli_ui_out::do_progress_* functions result in the following:
> -   - printed for tty, SHOULD_PRINT == true:
> -     <NAME
> -      [#####                      ]\r>
> -   - printed for tty, SHOULD_PRINT == false:
> -     <>
> +/* Initialize a progress update to be displayed with
> +   cli_ui_out::do_progress_notify.  */
> +
> +void
> +cli_ui_out::do_progress_start ()
> +{
> +  cli_progress_info info;
> +
> +  info.pos = 0;
> +  info.state = progress_update::START;
> +  m_progress_info.push_back (std::move (info));
> +}
> +
> +/* Print a progress update.  MSG is a string to be printed on the line above
> +   the progress bar.  TOTAL is the size of the download whose progress is
> +   being displayed.  UNIT should be the unit of TOTAL (ex. "KB"). If HOWMUCH
> +   is between 0.0 and 1.0, a progress bar is displayed indicating the percentage
> +   of completion and the download size.  If HOWMUCH is negative, a progress
> +   indicator will tick across the screen.  If the output stream is not a tty
> +   then only MSG is printed.
> +
> +   - printed for tty, HOWMUCH between 0.0 and 1.0:
> +       <MSG
> +       [#########                  ]  HOWMUCH*100% (TOTAL UNIT)\r>
> +   - printed for tty, HOWMUCH < 0.0:
> +       <MSG
> +       [    ###                    ]\r>
>     - printed for not-a-tty:
> -     <NAME...
> -     >
> +       <MSG...\n>
>  */
>
>  void
> -cli_ui_out::do_progress_start (const std::string &name, bool should_print)
> +cli_ui_out::do_progress_notify (const std::string &msg,
> +                               const std::string &unit,
> +                               double howmuch, double total)
>  {
> +  int chars_per_line = get_chars_per_line ();
>    struct ui_file *stream = m_streams.back ();
> -  cli_progress_info meter;
> +  cli_progress_info &info (m_progress_info.back ());
> +
> +#define MIN_CHARS_PER_LINE 50
> +#define MAX_CHARS_PER_LINE 4096
> +
> +  if (chars_per_line > MAX_CHARS_PER_LINE
> +      || chars_per_line == -1)
> +    chars_per_line = MAX_CHARS_PER_LINE;
> +
> +  if (info.state == progress_update::START)
> +    {
> +      if (stream->isatty ()
> +         && current_ui->input_interactive_p ()
> +         && chars_per_line >= MIN_CHARS_PER_LINE)
> +       {
> +         gdb_printf (stream, "%s\n", msg.c_str ());
> +         info.state = progress_update::BAR;
> +       }
> +      else
> +       {
> +         gdb_printf (stream, "%s...\n", msg.c_str ());
> +         info.state = progress_update::WORKING;
> +       }
> +    }
>
> -  meter.last_value = 0;
> -  meter.name = name;
> -  if (!stream->isatty ())
> +  if (info.state != progress_update::BAR
> +      || chars_per_line < MIN_CHARS_PER_LINE)
> +    return;
> +
> +  if (total > 0 && howmuch >= 0 && howmuch <= 1.0)
>      {
> -      gdb_printf (stream, "%s...", meter.name.c_str ());
> +      std::string progress = string_printf (" %3.f%% (%.2f %s)",
> +                                           howmuch * 100, total,
> +                                           unit.c_str ());
> +      int width = chars_per_line - progress.size () - 3;
> +      int max = width * howmuch;
> +
> +      std::string display = "\r[";
> +
> +      for (int i = 0; i < width; ++i)
> +       if (i < max)
> +         display += "#";
> +       else
> +         display += " ";
> +
> +      display += "]" + progress;
> +      gdb_printf (stream, "%s", display.c_str ());
>        gdb_flush (stream);
> -      meter.printing = WORKING;
>      }
>    else
>      {
> -      /* Don't actually emit anything until the first call notifies us
> -        of progress.  This makes it so a second progress message can
> -        be started before the first one has been notified, without
> -        messy output.  */
> -      meter.printing = should_print ? START : NO_PRINT;
> +      using namespace std::chrono;
> +      milliseconds diff = duration_cast<milliseconds>
> +       (steady_clock::now () - info.last_update);
> +
> +      /* Advance the progress indicator at a rate of 1 tick every
> +        every 0.5 seconds.  */
> +      if (diff.count () >= 500)
> +       {
> +         int width = chars_per_line - 3;
> +
> +         gdb_printf (stream, "\r[");
> +         for (int i = 0; i < width; ++i)
> +           {
> +             if (i == info.pos % width
> +                 || i == (info.pos + 1) % width
> +                 || i == (info.pos + 2) % width)
> +               gdb_printf (stream, "#");
> +             else
> +               gdb_printf (stream, " ");
> +           }
> +
> +         gdb_printf (stream, "]");
> +         gdb_flush (stream);
> +         info.last_update = steady_clock::now ();
> +         info.pos++;
> +       }
>      }
>
> -  m_meters.push_back (std::move (meter));
> +  return;
>  }
>
> +/* Clear the current line of the most recent progress update.  Overwrites
> +   the current line with whitespace.  */
> +
>  void
> -cli_ui_out::do_progress_notify (double howmuch)
> +cli_ui_out::clear_current_line ()
>  {
>    struct ui_file *stream = m_streams.back ();
> -  cli_progress_info &meter (m_meters.back ());
> +  int chars_per_line = get_chars_per_line ();
>
> -  if (meter.printing == NO_PRINT)
> +  if (!stream->isatty ()
> +      || !current_ui->input_interactive_p ())
>      return;
>
> -  if (meter.printing == START)
> -    {
> -      gdb_printf (stream, "%s\n", meter.name.c_str ());
> -      gdb_flush (stream);
> -      meter.printing = WORKING;
> -    }
> +  if (chars_per_line > MAX_CHARS_PER_LINE
> +      || chars_per_line == -1)
> +    chars_per_line = MAX_CHARS_PER_LINE;
>
> -  if (meter.printing == WORKING && howmuch >= 1.0)
> -    return;
> +  int width = chars_per_line;
>
> -  if (!stream->isatty ())
> -    return;
> +  gdb_printf (stream, "\r");
> +  for (int i = 0; i < width; ++i)
> +    gdb_printf (stream, " ");
> +  gdb_printf (stream, "\r");
>
> -  int chars_per_line = get_chars_per_line ();
> -  if (chars_per_line > 0)
> -    {
> -      int i, max;
> -      int width = chars_per_line - 3;
> -
> -      max = width * howmuch;
> -      gdb_printf (stream, "\r[");
> -      for (i = 0; i < width; ++i)
> -       gdb_printf (stream, i < max ? "#" : " ");
> -      gdb_printf (stream, "]");
> -      gdb_flush (stream);
> -      meter.printing = PROGRESS;
> -    }
> +  gdb_flush (stream);
>  }
>
> +/* Remove the most recent progress update from the progress_info stack
> +   and overwrite the current line with whitespace.  */
> +
>  void
>  cli_ui_out::do_progress_end ()
>  {
>    struct ui_file *stream = m_streams.back ();
> -  cli_progress_info &meter = m_meters.back ();
> -
> -  if (!stream->isatty ())
> -    {
> -      gdb_printf (stream, "\n");
> -      gdb_flush (stream);
> -    }
> -  else if (meter.printing == PROGRESS)
> -    {
> -      int i;
> -      int width = get_chars_per_line () - 3;
> -
> -      gdb_printf (stream, "\r");
> -      for (i = 0; i < width + 2; ++i)
> -       gdb_printf (stream, " ");
> -      gdb_printf (stream, "\r");
> -      gdb_flush (stream);
> -    }
> +  m_progress_info.pop_back ();
>
> -  m_meters.pop_back ();
> +  if (stream->isatty ())
> +    clear_current_line ();
>  }
>
>  /* local functions */
> diff --git a/gdb/cli-out.h b/gdb/cli-out.h
> index 3f01fe0db6d..01aed00bdf0 100644
> --- a/gdb/cli-out.h
> +++ b/gdb/cli-out.h
> @@ -21,6 +21,7 @@
>  #define CLI_OUT_H
>
>  #include "ui-out.h"
> +#include <chrono>
>  #include <vector>
>
>  class cli_ui_out : public ui_out
> @@ -71,8 +72,9 @@ class cli_ui_out : public ui_out
>    virtual void do_flush () override;
>    virtual void do_redirect (struct ui_file *outstream) override;
>
> -  virtual void do_progress_start (const std::string &, bool) override;
> -  virtual void do_progress_notify (double) override;
> +  virtual void do_progress_start () override;
> +  virtual void do_progress_notify (const std::string &, const std::string &,
> +                                  double, double) override;
>    virtual void do_progress_end () override;
>
>    bool suppress_output ()
> @@ -85,32 +87,20 @@ class cli_ui_out : public ui_out
>    std::vector<ui_file *> m_streams;
>    bool m_suppress_output;
>
> -  /* Represents the printing state of a progress meter.  */
> -  enum meter_state
> -  {
> -    /* Printing will start with the next output.  */
> -    START,
> -    /* Printing has already started.  */
> -    WORKING,
> -    /* Progress printing has already started.  */
> -    PROGRESS,
> -    /* Printing should not be done.  */
> -    NO_PRINT
> -  };
> -
> -  /* The state of a recent progress meter.  */
> +  /* The state of a recent progress update.  */
>    struct cli_progress_info
> -  {
> -    /* The current state.  */
> -    enum meter_state printing;
> -    /* The name to print.  */
> -    std::string name;
> -    /* The last notification value.  */
> -    double last_value;
> +  {
> +    /* Position of the progress indicator.  */
> +    int pos;
> +    /* The current state.  */
> +    progress_update::state state;
> +    /* Progress indicator's time of last update.  */
> +    std::chrono::steady_clock::time_point last_update;
>    };
>
> -  /* Stack of progress meters.  */
> -  std::vector<cli_progress_info> m_meters;
> +  /* Stack of progress info.  */
> +  std::vector<cli_progress_info> m_progress_info;
> +  void clear_current_line ();
>  };
>
>  extern void cli_display_match_list (char **matches, int len, int max);
> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
> index 5f04a2b38ca..fc62412fff2 100644
> --- a/gdb/debuginfod-support.c
> +++ b/gdb/debuginfod-support.c
> @@ -24,7 +24,9 @@
>  #include "gdbsupport/gdb_optional.h"
>  #include "cli/cli-cmds.h"
>  #include "cli/cli-style.h"
> +#include "cli-out.h"
>  #include "target.h"
> +#include <sstream>
>
>  /* Set/show debuginfod commands.  */
>  static cmd_list_element *set_debuginfod_prefix_list;
> @@ -87,12 +89,12 @@ debuginfod_exec_query (const unsigned char *build_id,
>  struct user_data
>  {
>    user_data (const char *desc, const char *fname)
> -    : desc (desc), fname (fname), has_printed (false)
> +    : desc (desc), fname (fname)
>    { }
>
>    const char * const desc;
>    const char * const fname;
> -  bool has_printed;
> +  ui_out::progress_update progress;
>  };
>
>  /* Deleter for a debuginfod_client.  */
> @@ -108,47 +110,85 @@ struct debuginfod_client_deleter
>  using debuginfod_client_up
>    = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
>
> +
> +/* Convert SIZE into a unit suitable for use with progress updates.
> +   SIZE should in given in bytes and will be converted into KB, MB, GB
> +   or remain unchanged. UNIT will be set to "B", "KB", "MB" or "GB"
> +   accordingly.  */
> +
> +static void
> +get_size_and_unit (double &size, std::string &unit)
> +{
> +  if (size < 1024)
> +    {
> +      /* If size is less than 1 KB then set unit to B.  */
> +      unit = "B";
> +      return;
> +    }
> +
> +  size /= 1024;
> +  if (size < 1024)
> +    {
> +      /* If size is less than 1 MB then set unit to KB.  */
> +      unit = "KB";
> +      return;
> +    }
> +
> +  size /= 1024;
> +  if (size < 1024)
> +    {
> +      /* If size is less than 1 GB then set unit to MB.  */
> +      unit = "MB";
> +      return;
> +    }
> +
> +  size /= 1024;
> +  unit = "GB";
> +}
> +
>  static int
>  progressfn (debuginfod_client *c, long cur, long total)
>  {
>    user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c));
>    gdb_assert (data != nullptr);
>
> +  string_file styled_fname (current_uiout->can_emit_style_escape ());
> +  fprintf_styled (&styled_fname, file_name_style.style (), "%s",
> +                 data->fname);
> +
>    if (check_quit_flag ())
>      {
> -      gdb_printf ("Cancelling download of %s %ps...\n",
> -                 data->desc,
> -                 styled_string (file_name_style.style (), data->fname));
> +      gdb_printf ("Cancelling download of %s %s...\n",
> +                 data->desc, styled_fname.c_str ());
>        return 1;
>      }
>
> -  if (!data->has_printed)
> +  if (debuginfod_verbose == 0)
> +    return 0;
> +
> +  /* Print progress update.  Include the transfer size if available.  */
> +  if (total > 0)
>      {
> -      /* Include the transfer size, if available.  */
> -      if (total > 0)
> +      /* Transfer size is known.  */
> +      double howmuch = (double) cur / (double) total;
> +
> +      if (howmuch >= 0.0 && howmuch <= 1.0)
>         {
> -         float size = 1.0f * total / 1024;
> -         const char *unit = "KB";
> -
> -         /* If size is greater than 0.01 MB, set unit to MB.  */
> -         if (size > 10.24)
> -           {
> -             size /= 1024;
> -             unit = "MB";
> -           }
> -
> -         gdb_printf ("Downloading %.2f %s %s %ps...\n",
> -                     size, unit, data->desc,
> -                     styled_string (file_name_style.style (),
> -                                    data->fname));
> +         double d_total = (double) total;
> +         std::string unit = "";
> +
> +         get_size_and_unit (d_total, unit);
> +         std::string msg = string_printf ("Downloading %0.2f %s %s %s",
> +                                          d_total, unit.c_str (), data->desc,
> +                                          styled_fname.c_str ());
> +         data->progress.update_progress (msg, unit, howmuch, d_total);
> +         return 0;
>         }
> -      else
> -       gdb_printf ("Downloading %s %ps...\n", data->desc,
> -                   styled_string (file_name_style.style (), data->fname));
> -
> -      data->has_printed = true;
>      }
>
> +  std::string msg = string_printf ("Downloading %s %s",
> +                                  data->desc, styled_fname.c_str ());
> +  data->progress.update_progress (msg);
>    return 0;
>  }
>
> @@ -230,6 +270,23 @@ debuginfod_is_enabled ()
>    return true;
>  }
>
> +/* Print the result of the most recent attempted download.  */
> +
> +static void
> +print_outcome (user_data &data, int fd)
> +{
> +  /* Clears the current line of progress output.  */
> +  current_uiout->do_progress_end ();
> +
> +  string_file styled_fname (current_uiout->can_emit_style_escape ());
> +  fprintf_styled (&styled_fname, file_name_style.style (), "%s",
> +                 data.fname);
> +
> +  if (fd < 0 && fd != -ENOENT)
> +    gdb_printf (_("Download failed: %s. Continuing without %s %s.\n"),
> +               safe_strerror (-fd), data.desc, styled_fname.c_str ());
> +}
> +
>  /* See debuginfod-support.h  */
>
>  scoped_fd
> @@ -263,11 +320,7 @@ debuginfod_source_query (const unsigned char *build_id,
>                                         srcpath,
>                                         &dname));
>    debuginfod_set_user_data (c, nullptr);
> -
> -  if (fd.get () < 0 && fd.get () != -ENOENT)
> -    gdb_printf (_("Download failed: %s.  Continuing without source file %ps.\n"),
> -               safe_strerror (-fd.get ()),
> -               styled_string (file_name_style.style (),  srcpath));
> +  print_outcome (data, fd.get ());
>
>    if (fd.get () >= 0)
>      destname->reset (dname);
> @@ -305,11 +358,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
>    scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
>                                            &dname));
>    debuginfod_set_user_data (c, nullptr);
> -
> -  if (fd.get () < 0 && fd.get () != -ENOENT)
> -    gdb_printf (_("Download failed: %s.  Continuing without debug info for %ps.\n"),
> -               safe_strerror (-fd.get ()),
> -               styled_string (file_name_style.style (),  filename));
> +  print_outcome (data, fd.get ());
>
>    if (fd.get () >= 0)
>      destname->reset (dname);
> @@ -346,12 +395,7 @@ debuginfod_exec_query (const unsigned char *build_id,
>
>    scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
>    debuginfod_set_user_data (c, nullptr);
> -
> -  if (fd.get () < 0 && fd.get () != -ENOENT)
> -    gdb_printf (_("Download failed: %s. " \
> -                 "Continuing without executable for %ps.\n"),
> -               safe_strerror (-fd.get ()),
> -               styled_string (file_name_style.style (),  filename));
> +  print_outcome (data, fd.get ());
>
>    if (fd.get () >= 0)
>      destname->reset (dname);
> diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
> index 96a847eb6b6..5c60b139775 100644
> --- a/gdb/mi/mi-out.c
> +++ b/gdb/mi/mi-out.c
> @@ -259,6 +259,34 @@ mi_ui_out::main_stream ()
>    return (string_file *) m_streams.back ();
>  }
>
> +/* Initialize a progress update to be displayed with
> +   mi_ui_out::do_progress_notify.  */
> +
> +void
> +mi_ui_out::do_progress_start ()
> +{
> +  mi_progress_info info;
> +
> +  info.state = progress_update::START;
> +  m_progress_info.push_back (std::move (info));
> +}
> +
> +/* Indicate that a task described by MSG is in progress.  */
> +
> +void
> +mi_ui_out::do_progress_notify (const std::string &msg, const std::string &unit,
> +                              double cur, double total)
> +{
> +  mi_progress_info &info (m_progress_info.back ());
> +
> +  if (info.state == progress_update::START)
> +    {
> +      struct ui_file *stream = gdb_stdout;
> +      gdb_printf (stream, "%s...\n", msg.c_str ());
> +      info.state = progress_update::WORKING;
> +    }
> +}
> +
>  /* Clear the buffer.  */
>
>  void
> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
> index 36d7e42345f..d80a7fce259 100644
> --- a/gdb/mi/mi-out.h
> +++ b/gdb/mi/mi-out.h
> @@ -25,7 +25,6 @@
>  struct ui_out;
>  struct ui_file;
>
> -
>  class mi_ui_out : public ui_out
>  {
>  public:
> @@ -83,13 +82,9 @@ class mi_ui_out : public ui_out
>    virtual bool do_is_mi_like_p () const override
>    { return true; }
>
> -  virtual void do_progress_start (const std::string &, bool) override
> -  {
> -  }
> -
> -  virtual void do_progress_notify (double) override
> -  {
> -  }
> +  virtual void do_progress_start () override;
> +  virtual void do_progress_notify (const std::string &, const std::string &,
> +                                  double, double) override;
>
>    virtual void do_progress_end () override
>    {
> @@ -101,6 +96,16 @@ class mi_ui_out : public ui_out
>    void open (const char *name, ui_out_type type);
>    void close (ui_out_type type);
>
> +  /* The state of a recent progress_update.  */
> +  struct mi_progress_info
> +  {
> +    /* The current state.  */
> +    progress_update::state state;
> +  };
> +
> +  /* Stack of progress info.  */
> +  std::vector<mi_progress_info> m_progress_info;
> +
>    /* Convenience method that returns the MI out's string stream cast
>       to its appropriate type.  Assumes/asserts that output was not
>       redirected.  */
> diff --git a/gdb/ui-out.h b/gdb/ui-out.h
> index 65d40875926..63334dbb8d8 100644
> --- a/gdb/ui-out.h
> +++ b/gdb/ui-out.h
> @@ -278,39 +278,56 @@ class ui_out
>       escapes.  */
>    virtual bool can_emit_style_escape () const = 0;
>
> -  /* An object that starts and finishes a progress meter.  */
> -  class progress_meter
> +  /* An object that starts and finishes displaying progress updates.  */
> +  class progress_update
>    {
>    public:
> +    /* Represents the printing state of a progress update.  */
> +    enum state
> +    {
> +      /* Printing will start with the next update.  */
> +      START,
> +      /* Printing has already started.  */
> +      WORKING,
> +      /* Progress bar printing has already started.  */
> +      BAR
> +    };
> +
>      /* SHOULD_PRINT indicates whether something should be printed for a tty.  */
> -    progress_meter (struct ui_out *uiout, const std::string &name,
> -                   bool should_print)
> -      : m_uiout (uiout)
> +    progress_update ()
>      {
> -      m_uiout->do_progress_start (name, should_print);
> +      m_uiout = current_uiout;
> +      m_uiout->do_progress_start ();
>      }
>
> -    ~progress_meter ()
> +    ~progress_update ()
>      {
> -      m_uiout->do_progress_notify (1.0);
> -      m_uiout->do_progress_end ();
> +
>      }
>
> -    progress_meter (const progress_meter &) = delete;
> -    progress_meter &operator= (const progress_meter &) = delete;
> +    progress_update (const progress_update &) = delete;
> +    progress_update &operator= (const progress_update &) = delete;
>
> -    /* Emit some progress for this progress meter.  HOWMUCH may range
> -       from 0.0 to 1.0.  */
> -    void progress (double howmuch)
> +    /* Emit some progress for this progress meter.  Includes current
> +       amount of progress made and total amount in the display.  */
> +    void update_progress (const std::string& msg, std::string& unit,
> +                         double cur, double total)
>      {
> -      m_uiout->do_progress_notify (howmuch);
> +      m_uiout->do_progress_notify (msg, unit, cur, total);
>      }
>
> +    /* Emit some progress for this progress meter.  */
> +    void update_progress (const std::string& msg)
> +    {
> +      m_uiout->do_progress_notify (msg, "", -1, -1);
> +    }
>    private:
>
>      struct ui_out *m_uiout;
>    };
>
> +  virtual void do_progress_end () = 0;
> +
>   protected:
>
>    virtual void do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
> @@ -345,9 +362,9 @@ class ui_out
>    virtual void do_flush () = 0;
>    virtual void do_redirect (struct ui_file *outstream) = 0;
>
> -  virtual void do_progress_start (const std::string &, bool) = 0;
> -  virtual void do_progress_notify (double) = 0;
> -  virtual void do_progress_end () = 0;
> +  virtual void do_progress_start () = 0;
> +  virtual void do_progress_notify (const std::string &, const std::string &,
> +                                  double, double) = 0;
>
>    /* Set as not MI-like by default.  It is overridden in subclasses if
>       necessary.  */
> --
> 2.37.3
>
  

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> Ping
Aaron> Thanks,
Aaron> Aaron

Thanks for the patch.

>> +  cli_progress_info info;
>> +
>> +  info.pos = 0;
>> +  info.state = progress_update::START;

I think it would be better to have cli_progress_info initialize itself
instead of doing it here.

>> +  m_progress_info.push_back (std::move (info));

This could be just emplace_back then.

>> +  {
>> +    /* Position of the progress indicator.  */
>> +    int pos;

like this could be '= 0'

>> +    /* The current state.  */
>> +    progress_update::state state;

'= progress_update::START'

>> diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
>> index 5f04a2b38ca..fc62412fff2 100644
>> --- a/gdb/debuginfod-support.c
>> +++ b/gdb/debuginfod-support.c
>> @@ -24,7 +24,9 @@
>> #include "gdbsupport/gdb_optional.h"
>> #include "cli/cli-cmds.h"
>> #include "cli/cli-style.h"
>> +#include "cli-out.h"
>> #include "target.h"
>> +#include <sstream>

Is this include used?

>> +/* Convert SIZE into a unit suitable for use with progress updates.
>> +   SIZE should in given in bytes and will be converted into KB, MB, GB
>> +   or remain unchanged. UNIT will be set to "B", "KB", "MB" or "GB"
>> +   accordingly.  */

There's a bunch of pedantic thought about the proper suffixes, but I
think we should just follow GNU coreutils and use the shorter B/K/M/G.

df --help says:

    The SIZE argument is an integer and optional unit (example: 10K is 10*1024).
    Units are K,M,G,T,P,E,Z,Y (powers of 1024) or KB,MB,... (powers of 1000).
    Binary prefixes can be used, too: KiB=K, MiB=M, and so on.

Also since all the responses are just a constant string, it's better to
just return a 'const char *' from the function and avoid allocation.

>> +/* Print the result of the most recent attempted download.  */
>> +
>> +static void
>> +print_outcome (user_data &data, int fd)
>> +{
>> +  /* Clears the current line of progress output.  */
>> +  current_uiout->do_progress_end ();
>> +
>> +  string_file styled_fname (current_uiout->can_emit_style_escape ());
>> +  fprintf_styled (&styled_fname, file_name_style.style (), "%s",
>> +                 data.fname);
>> +
>> +  if (fd < 0 && fd != -ENOENT)
>> +    gdb_printf (_("Download failed: %s. Continuing without %s %s.\n"),
>> +               safe_strerror (-fd), data.desc, styled_fname.c_str ());

I think you can use %ps and styled_string here instead of a separate
string_file.  Not sure this applied elsewhere but it's worth looking.

Hmm the old code did it:

>> -  if (fd.get () < 0 && fd.get () != -ENOENT)
>> -    gdb_printf (_("Download failed: %s.  Continuing without source file %ps.\n"),
>> -               safe_strerror (-fd.get ()),
>> -               styled_string (file_name_style.style (),  srcpath));

Seems like that can just be reused.

>> +void
>> +mi_ui_out::do_progress_start ()
>> +{
>> +  mi_progress_info info;
>> +
>> +  info.state = progress_update::START;

I don't understand why this creates an object and pushes it on a
vector.  Wouldn't printing the message from do_progress_start be the
same, but avoid all the hair?

>> +      struct ui_file *stream = gdb_stdout;
>> +      gdb_printf (stream, "%s...\n", msg.c_str ());

I don't think an explicit stream is needed here.

Nothing ever pops the mi_progress_info from the stack.

>> void
>> diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
>> index 36d7e42345f..d80a7fce259 100644
>> --- a/gdb/mi/mi-out.h
>> +++ b/gdb/mi/mi-out.h
>> @@ -25,7 +25,6 @@
>> struct ui_out;
>> struct ui_file;
>> 
>> -
>> class mi_ui_out : public ui_out

Spurious change.

Tom
  

Hi Tom,

Thanks for the review. I fixed the issues you pointed out.

On Fri, Oct 21, 2022 at 4:03 PM Tom Tromey <tom@tromey.com> wrote:
> >> +void
> >> +mi_ui_out::do_progress_start ()
> >> +{
> >> +  mi_progress_info info;
> >> +
> >> +  info.state = progress_update::START;
>
> I don't understand why this creates an object and pushes it on a
> vector.  Wouldn't printing the message from do_progress_start be the
> same, but avoid all the hair?

If we do this then we can't include the download size in the message.
The download size is not available when do_progress_start is called.
It's made available to do_progress_notify once the download has started.

Aaron

---

If the download size is known, a progress bar is displayed along with
the percentage of completion and the total download size.

  Downloading separate debug info for /lib/libxyz.so
  [############                                      ]  25% (10.01 M)

If the download size is not known, a progress indicator is displayed
with a ticker ("###") that moves across the screen at a rate of 1 tick
every 0.5 seconds.

  Downloading separate debug info for /lib/libxyz.so
  [         ###                                                      ]

If the output stream is not a tty, batch mode is enabled or the screen
is too narrow then only a static description of the download is printed.
No bar or ticker is displayed.

  Downloading separate debug info for /lib/libxyz.so...

In any case, if the size of the download is known at the time the
description is printed then it will be included in the description.

  Downloading 10.01 MB separate debug info for /lib/libxyz.so...
---
 gdb/cli-out.c            | 190 +++++++++++++++++++++++++--------------
 gdb/cli-out.h            |  40 ++++-----
 gdb/debuginfod-support.c | 116 +++++++++++++++---------
 gdb/mi/mi-out.c          |  32 +++++++
 gdb/mi/mi-out.h          |  29 +++---
 gdb/ui-out.h             |  53 +++++++----
 6 files changed, 298 insertions(+), 162 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index fdbed6f5e91..fc601660411 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -26,6 +26,7 @@
 #include "completer.h"
 #include "readline/readline.h"
 #include "cli/cli-style.h"
+#include "top.h"
 
 /* These are the CLI output functions */
 
@@ -262,105 +263,160 @@ cli_ui_out::do_redirect (ui_file *outstream)
     m_streams.pop_back ();
 }
 
-/* The cli_ui_out::do_progress_* functions result in the following:
-   - printed for tty, SHOULD_PRINT == true:
-     <NAME
-      [#####                      ]\r>
-   - printed for tty, SHOULD_PRINT == false:
-     <>
+/* Initialize a progress update to be displayed with
+   cli_ui_out::do_progress_notify.  */
+
+void
+cli_ui_out::do_progress_start ()
+{
+  m_progress_info.emplace_back ();
+}
+
+/* Print a progress update.  MSG is a string to be printed on the line above
+   the progress bar.  TOTAL is the size of the download whose progress is
+   being displayed.  UNIT should be the unit of TOTAL (ex. "K"). If HOWMUCH
+   is between 0.0 and 1.0, a progress bar is displayed indicating the percentage
+   of completion and the download size.  If HOWMUCH is negative, a progress
+   indicator will tick across the screen.  If the output stream is not a tty
+   then only MSG is printed.
+
+   - printed for tty, HOWMUCH between 0.0 and 1.0:
+	<MSG
+	[#########                  ]  HOWMUCH*100% (TOTAL UNIT)\r>
+   - printed for tty, HOWMUCH < 0.0:
+	<MSG
+	[    ###                    ]\r>
    - printed for not-a-tty:
-     <NAME...
-     >
+	<MSG...\n>
 */
 
 void
-cli_ui_out::do_progress_start (const std::string &name, bool should_print)
+cli_ui_out::do_progress_notify (const std::string &msg,
+				const char *unit,
+				double howmuch, double total)
 {
+  int chars_per_line = get_chars_per_line ();
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info meter;
+  cli_progress_info &info (m_progress_info.back ());
+
+#define MIN_CHARS_PER_LINE 50
+#define MAX_CHARS_PER_LINE 4096
+
+  if (chars_per_line > MAX_CHARS_PER_LINE
+      || chars_per_line == -1)
+    chars_per_line = MAX_CHARS_PER_LINE;
 
-  meter.last_value = 0;
-  meter.name = name;
-  if (!stream->isatty ())
+  if (info.state == progress_update::START)
     {
-      gdb_printf (stream, "%s...", meter.name.c_str ());
+      if (stream->isatty ()
+	  && current_ui->input_interactive_p ()
+	  && chars_per_line >= MIN_CHARS_PER_LINE)
+	{
+	  gdb_printf (stream, "%s\n", msg.c_str ());
+	  info.state = progress_update::BAR;
+	}
+      else
+	{
+	  gdb_printf (stream, "%s...\n", msg.c_str ());
+	  info.state = progress_update::WORKING;
+	}
+    }
+
+  if (info.state != progress_update::BAR
+      || chars_per_line < MIN_CHARS_PER_LINE)
+    return;
+
+  if (total > 0 && howmuch >= 0 && howmuch <= 1.0)
+    {
+      std::string progress = string_printf (" %3.f%% (%.2f %s)",
+					    howmuch * 100, total,
+					    unit);
+      int width = chars_per_line - progress.size () - 4;
+      int max = width * howmuch;
+
+      std::string display = "\r[";
+
+      for (int i = 0; i < width; ++i)
+	if (i < max)
+	  display += "#";
+	else
+	  display += " ";
+
+      display += "]" + progress;
+      gdb_printf (stream, "%s", display.c_str ());
       gdb_flush (stream);
-      meter.printing = WORKING;
     }
   else
     {
-      /* Don't actually emit anything until the first call notifies us
-	 of progress.  This makes it so a second progress message can
-	 be started before the first one has been notified, without
-	 messy output.  */
-      meter.printing = should_print ? START : NO_PRINT;
+      using namespace std::chrono;
+      milliseconds diff = duration_cast<milliseconds>
+	(steady_clock::now () - info.last_update);
+
+      /* Advance the progress indicator at a rate of 1 tick every
+	 every 0.5 seconds.  */
+      if (diff.count () >= 500)
+	{
+	  int width = chars_per_line - 4;
+
+	  gdb_printf (stream, "\r[");
+	  for (int i = 0; i < width; ++i)
+	    {
+	      if (i == info.pos % width
+		  || i == (info.pos + 1) % width
+		  || i == (info.pos + 2) % width)
+		gdb_printf (stream, "#");
+	      else
+		gdb_printf (stream, " ");
+	    }
+
+	  gdb_printf (stream, "]");
+	  gdb_flush (stream);
+	  info.last_update = steady_clock::now ();
+	  info.pos++;
+	}
     }
 
-  m_meters.push_back (std::move (meter));
+  return;
 }
 
+/* Clear the current line of the most recent progress update.  Overwrites
+   the current line with whitespace.  */
+
 void
-cli_ui_out::do_progress_notify (double howmuch)
+cli_ui_out::clear_current_line ()
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info &meter (m_meters.back ());
+  int chars_per_line = get_chars_per_line ();
 
-  if (meter.printing == NO_PRINT)
+  if (!stream->isatty ()
+      || !current_ui->input_interactive_p ())
     return;
 
-  if (meter.printing == START)
-    {
-      gdb_printf (stream, "%s\n", meter.name.c_str ());
-      gdb_flush (stream);
-      meter.printing = WORKING;
-    }
+  if (chars_per_line > MAX_CHARS_PER_LINE
+      || chars_per_line == -1)
+    chars_per_line = MAX_CHARS_PER_LINE;
 
-  if (meter.printing == WORKING && howmuch >= 1.0)
-    return;
+  int width = chars_per_line;
 
-  if (!stream->isatty ())
-    return;
+  gdb_printf (stream, "\r");
+  for (int i = 0; i < width; ++i)
+    gdb_printf (stream, " ");
+  gdb_printf (stream, "\r");
 
-  int chars_per_line = get_chars_per_line ();
-  if (chars_per_line > 0)
-    {
-      int i, max;
-      int width = chars_per_line - 3;
-
-      max = width * howmuch;
-      gdb_printf (stream, "\r[");
-      for (i = 0; i < width; ++i)
-	gdb_printf (stream, i < max ? "#" : " ");
-      gdb_printf (stream, "]");
-      gdb_flush (stream);
-      meter.printing = PROGRESS;
-    }
+  gdb_flush (stream);
 }
 
+/* Remove the most recent progress update from the progress_info stack
+   and overwrite the current line with whitespace.  */
+
 void
 cli_ui_out::do_progress_end ()
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info &meter = m_meters.back ();
-
-  if (!stream->isatty ())
-    {
-      gdb_printf (stream, "\n");
-      gdb_flush (stream);
-    }
-  else if (meter.printing == PROGRESS)
-    {
-      int i;
-      int width = get_chars_per_line () - 3;
-
-      gdb_printf (stream, "\r");
-      for (i = 0; i < width + 2; ++i)
-	gdb_printf (stream, " ");
-      gdb_printf (stream, "\r");
-      gdb_flush (stream);
-    }
+  m_progress_info.pop_back ();
 
-  m_meters.pop_back ();
+  if (stream->isatty ())
+    clear_current_line ();
 }
 
 /* local functions */
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 3f01fe0db6d..06d155f7779 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -21,6 +21,7 @@
 #define CLI_OUT_H
 
 #include "ui-out.h"
+#include <chrono>
 #include <vector>
 
 class cli_ui_out : public ui_out
@@ -71,8 +72,9 @@ class cli_ui_out : public ui_out
   virtual void do_flush () override;
   virtual void do_redirect (struct ui_file *outstream) override;
 
-  virtual void do_progress_start (const std::string &, bool) override;
-  virtual void do_progress_notify (double) override;
+  virtual void do_progress_start () override;
+  virtual void do_progress_notify (const std::string &, const char *,
+				   double, double) override;
   virtual void do_progress_end () override;
 
   bool suppress_output ()
@@ -85,32 +87,24 @@ class cli_ui_out : public ui_out
   std::vector<ui_file *> m_streams;
   bool m_suppress_output;
 
-  /* Represents the printing state of a progress meter.  */
-  enum meter_state
-  {
-    /* Printing will start with the next output.  */
-    START,
-    /* Printing has already started.  */
-    WORKING,
-    /* Progress printing has already started.  */
-    PROGRESS,
-    /* Printing should not be done.  */
-    NO_PRINT
-  };
-
-  /* The state of a recent progress meter.  */
+  /* The state of a recent progress update.  */
   struct cli_progress_info
   {
+    /* Position of the progress indicator.  */
+    int pos;
     /* The current state.  */
-    enum meter_state printing;
-    /* The name to print.  */
-    std::string name;
-    /* The last notification value.  */
-    double last_value;
+    progress_update::state state;
+    /* Progress indicator's time of last update.  */
+    std::chrono::steady_clock::time_point last_update;
+
+    cli_progress_info ()
+      : pos (0), state (progress_update::START)
+    {}
   };
 
-  /* Stack of progress meters.  */
-  std::vector<cli_progress_info> m_meters;
+  /* Stack of progress info.  */
+  std::vector<cli_progress_info> m_progress_info;
+  void clear_current_line ();
 };
 
 extern void cli_display_match_list (char **matches, int len, int max);
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 5f04a2b38ca..f1249203da0 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -24,6 +24,7 @@
 #include "gdbsupport/gdb_optional.h"
 #include "cli/cli-cmds.h"
 #include "cli/cli-style.h"
+#include "cli-out.h"
 #include "target.h"
 
 /* Set/show debuginfod commands.  */
@@ -87,12 +88,12 @@ debuginfod_exec_query (const unsigned char *build_id,
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname), has_printed (false)
+    : desc (desc), fname (fname)
   { }
 
   const char * const desc;
   const char * const fname;
-  bool has_printed;
+  ui_out::progress_update progress;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -108,47 +109,74 @@ struct debuginfod_client_deleter
 using debuginfod_client_up
   = std::unique_ptr<debuginfod_client, debuginfod_client_deleter>;
 
+
+/* Convert SIZE into a unit suitable for use with progress updates.
+   SIZE should in given in bytes and will be converted into KB, MB, GB
+   or remain unchanged. UNIT will be set to "B", "KB", "MB" or "GB"
+   accordingly.  */
+
+static const char *
+get_size_and_unit (double &size)
+{
+  if (size < 1024)
+    /* If size is less than 1 KB then set unit to B.  */
+    return "B";
+
+  size /= 1024;
+  if (size < 1024)
+    /* If size is less than 1 MB then set unit to KB.  */
+    return "K";
+
+  size /= 1024;
+  if (size < 1024)
+    /* If size is less than 1 GB then set unit to MB.  */
+    return "M";
+
+  size /= 1024;
+  return "G";
+}
+
 static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
   user_data *data = static_cast<user_data *> (debuginfod_get_user_data (c));
   gdb_assert (data != nullptr);
 
+  string_file styled_fname (current_uiout->can_emit_style_escape ());
+  fprintf_styled (&styled_fname, file_name_style.style (), "%s",
+		  data->fname);
+
   if (check_quit_flag ())
     {
-      gdb_printf ("Cancelling download of %s %ps...\n",
-		  data->desc,
-		  styled_string (file_name_style.style (), data->fname));
+      gdb_printf ("Cancelling download of %s %s...\n",
+		  data->desc, styled_fname.c_str ());
       return 1;
     }
 
-  if (!data->has_printed)
+  if (debuginfod_verbose == 0)
+    return 0;
+
+  /* Print progress update.  Include the transfer size if available.  */
+  if (total > 0)
     {
-      /* Include the transfer size, if available.  */
-      if (total > 0)
+      /* Transfer size is known.  */
+      double howmuch = (double) cur / (double) total;
+
+      if (howmuch >= 0.0 && howmuch <= 1.0)
 	{
-	  float size = 1.0f * total / 1024;
-	  const char *unit = "KB";
-
-	  /* If size is greater than 0.01 MB, set unit to MB.  */
-	  if (size > 10.24)
-	    {
-	      size /= 1024;
-	      unit = "MB";
-	    }
-
-	  gdb_printf ("Downloading %.2f %s %s %ps...\n",
-		      size, unit, data->desc,
-		      styled_string (file_name_style.style (),
-				     data->fname));
+	  double d_total = (double) total;
+	  const char *unit =  get_size_and_unit (d_total);
+	  std::string msg = string_printf ("Downloading %0.2f %s %s %s",
+					   d_total, unit, data->desc,
+					   styled_fname.c_str ());
+	  data->progress.update_progress (msg, unit, howmuch, d_total);
+	  return 0;
 	}
-      else
-	gdb_printf ("Downloading %s %ps...\n", data->desc,
-		    styled_string (file_name_style.style (), data->fname));
-
-      data->has_printed = true;
     }
 
+  std::string msg = string_printf ("Downloading %s %s",
+				   data->desc, styled_fname.c_str ());
+  data->progress.update_progress (msg);
   return 0;
 }
 
@@ -230,6 +258,21 @@ debuginfod_is_enabled ()
   return true;
 }
 
+/* Print the result of the most recent attempted download.  */
+
+static void
+print_outcome (user_data &data, int fd)
+{
+  /* Clears the current line of progress output.  */
+  current_uiout->do_progress_end ();
+
+  if (fd < 0 && fd != -ENOENT)
+    gdb_printf (_("Download failed: %s.  Continuing without %s %ps.\n"),
+		safe_strerror (-fd),
+		data.desc,
+		styled_string (file_name_style.style (), data.fname));
+}
+
 /* See debuginfod-support.h  */
 
 scoped_fd
@@ -263,11 +306,7 @@ debuginfod_source_query (const unsigned char *build_id,
 					srcpath,
 					&dname));
   debuginfod_set_user_data (c, nullptr);
-
-  if (fd.get () < 0 && fd.get () != -ENOENT)
-    gdb_printf (_("Download failed: %s.  Continuing without source file %ps.\n"),
-		safe_strerror (-fd.get ()),
-		styled_string (file_name_style.style (),  srcpath));
+  print_outcome (data, fd.get ());
 
   if (fd.get () >= 0)
     destname->reset (dname);
@@ -305,11 +344,7 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   scoped_fd fd (debuginfod_find_debuginfo (c, build_id, build_id_len,
 					   &dname));
   debuginfod_set_user_data (c, nullptr);
-
-  if (fd.get () < 0 && fd.get () != -ENOENT)
-    gdb_printf (_("Download failed: %s.  Continuing without debug info for %ps.\n"),
-		safe_strerror (-fd.get ()),
-		styled_string (file_name_style.style (),  filename));
+  print_outcome (data, fd.get ());
 
   if (fd.get () >= 0)
     destname->reset (dname);
@@ -346,12 +381,7 @@ debuginfod_exec_query (const unsigned char *build_id,
 
   scoped_fd fd (debuginfod_find_executable (c, build_id, build_id_len, &dname));
   debuginfod_set_user_data (c, nullptr);
-
-  if (fd.get () < 0 && fd.get () != -ENOENT)
-    gdb_printf (_("Download failed: %s. " \
-		  "Continuing without executable for %ps.\n"),
-		safe_strerror (-fd.get ()),
-		styled_string (file_name_style.style (),  filename));
+  print_outcome (data, fd.get ());
 
   if (fd.get () >= 0)
     destname->reset (dname);
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index 028c0058e58..725c1c68546 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -259,6 +259,38 @@ mi_ui_out::main_stream ()
   return (string_file *) m_streams.back ();
 }
 
+/* Initialize a progress update to be displayed with
+   mi_ui_out::do_progress_notify.  */
+
+void
+mi_ui_out::do_progress_start ()
+{
+  m_progress_info.emplace_back ();
+}
+
+/* Indicate that a task described by MSG is in progress.  */
+
+void
+mi_ui_out::do_progress_notify (const std::string &msg, const char *unit,
+			       double cur, double total)
+{
+  mi_progress_info &info (m_progress_info.back ());
+
+  if (info.state == progress_update::START)
+    {
+      gdb_printf ("%s...\n", msg.c_str ());
+      info.state = progress_update::WORKING;
+    }
+}
+
+/* Remove the most recent progress update from the progress_info stack.  */
+
+void
+mi_ui_out::do_progress_end ()
+{
+  m_progress_info.pop_back ();
+}
+
 /* Clear the buffer.  */
 
 void
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 36d7e42345f..2280afc6bbc 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -83,17 +82,11 @@ class mi_ui_out : public ui_out
   virtual bool do_is_mi_like_p () const override
   { return true; }
 
-  virtual void do_progress_start (const std::string &, bool) override
-  {
-  }
-
-  virtual void do_progress_notify (double) override
-  {
-  }
+  virtual void do_progress_start () override;
+  virtual void do_progress_notify (const std::string &, const char *,
+				   double, double) override;
 
-  virtual void do_progress_end () override
-  {
-  }
+  virtual void do_progress_end () override;
 
 private:
 
@@ -101,6 +94,20 @@ class mi_ui_out : public ui_out
   void open (const char *name, ui_out_type type);
   void close (ui_out_type type);
 
+  /* The state of a recent progress_update.  */
+  struct mi_progress_info
+  {
+    /* The current state.  */
+    progress_update::state state;
+
+    mi_progress_info ()
+      : state (progress_update::START)
+    {}
+  };
+
+  /* Stack of progress info.  */
+  std::vector<mi_progress_info> m_progress_info;
+
   /* Convenience method that returns the MI out's string stream cast
      to its appropriate type.  Assumes/asserts that output was not
      redirected.  */
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 65d40875926..5390c23fe79 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -278,39 +278,56 @@ class ui_out
      escapes.  */
   virtual bool can_emit_style_escape () const = 0;
 
-  /* An object that starts and finishes a progress meter.  */
-  class progress_meter
+  /* An object that starts and finishes displaying progress updates.  */
+  class progress_update
   {
   public:
+    /* Represents the printing state of a progress update.  */
+    enum state
+    {
+      /* Printing will start with the next update.  */
+      START,
+      /* Printing has already started.  */
+      WORKING,
+      /* Progress bar printing has already started.  */
+      BAR
+    };
+
     /* SHOULD_PRINT indicates whether something should be printed for a tty.  */
-    progress_meter (struct ui_out *uiout, const std::string &name,
-		    bool should_print)
-      : m_uiout (uiout)
+    progress_update ()
     {
-      m_uiout->do_progress_start (name, should_print);
+      m_uiout = current_uiout;
+      m_uiout->do_progress_start ();
     }
 
-    ~progress_meter ()
+    ~progress_update ()
     {
-      m_uiout->do_progress_notify (1.0);
-      m_uiout->do_progress_end ();
+
     }
 
-    progress_meter (const progress_meter &) = delete;
-    progress_meter &operator= (const progress_meter &) = delete;
+    progress_update (const progress_update &) = delete;
+    progress_update &operator= (const progress_update &) = delete;
 
-    /* Emit some progress for this progress meter.  HOWMUCH may range
-       from 0.0 to 1.0.  */
-    void progress (double howmuch)
+    /* Emit some progress for this progress meter.  Includes current
+       amount of progress made and total amount in the display.  */
+    void update_progress (const std::string& msg, const char *unit,
+			  double cur, double total)
     {
-      m_uiout->do_progress_notify (howmuch);
+      m_uiout->do_progress_notify (msg, unit, cur, total);
     }
 
+    /* Emit some progress for this progress meter.  */
+    void update_progress (const std::string& msg)
+    {
+      m_uiout->do_progress_notify (msg, "", -1, -1);
+    }
   private:
 
     struct ui_out *m_uiout;
   };
 
+  virtual void do_progress_end () = 0;
+
  protected:
 
   virtual void do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
@@ -345,9 +362,9 @@ class ui_out
   virtual void do_flush () = 0;
   virtual void do_redirect (struct ui_file *outstream) = 0;
 
-  virtual void do_progress_start (const std::string &, bool) = 0;
-  virtual void do_progress_notify (double) = 0;
-  virtual void do_progress_end () = 0;
+  virtual void do_progress_start () = 0;
+  virtual void do_progress_notify (const std::string &, const char *,
+				   double, double) = 0;
 
   /* Set as not MI-like by default.  It is overridden in subclasses if
      necessary.  */
  

>>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:

Aaron> If we do this then we can't include the download size in the message.
Aaron> The download size is not available when do_progress_start is called.
Aaron> It's made available to do_progress_notify once the download has started.

Thanks, I get it now.

This patch is ok.

Tom
  

On Wed, Nov 9, 2022 at 3:02 PM Tom Tromey <tom@tromey.com> wrote:
>
> This patch is ok.

Thanks Tom, pushed as commit 27859c6b9d73.

I made a small change to this patch before pushing. If gdb's width
is set to unlimited, we now just print a static description of the
download. No progress bar will be printed, same as if the screen
width was too narrow for the progress bar or batch mode was
enabled. I updated the commit message to reflect this change.

Previously if gdb's width was unlimited we'd set the progress bar
width to maximum (4k characters), however this caused some
visual noise in the gdb.debuginfod test log. To avoid any
unintentionally huge progress bars I think it's better to only display
them when a definite screen width is known.

Aaron