[PATCH v2] gdb/debuginfod: Rework progress updates

[PATCH v2] gdb/debuginfod: Rework progress updates

[Aaron Merey]

For more information regarding v1 see
https://sourceware.org/pipermail/gdb-patches/2022-January/185034.html

Previously debuginfod would use a progress bar to indicate the
progress of each download:

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

This format assumed that an accurate transfer size would always become
available.  Progress updates would not start printing until the size
could be displayed.

However there are cases where the total size of the transfer is not
available.  In this case gdb would appear to hang while the transfer
occured.  Additionally there are cases where the transfer size was
reported to gdb as -1. This resulted in progress updates indicating
that a file of size -0.00 MB is downloading.

This patch reworks progress updates to account for cases where the
transfer size is not known.  It also ensures that progress is promptly
reported and it provides ways to update progress output when new
information becomes available.

When the transfer size is not known, a progress throbber now spins
in front of the description of the download in order to indicate
that the transfer is ongoing. The throbber consists of the characters
-\|/ cycling at a rate of one tick per second.

    <THROBBER> Downloading separate debug info for /lib/libxyz.so.1

When the transfer size is known, the percentage of the transfer that
has completed will be displayed in front of the description of the
download.

    (34%) Downloading 1.23 MB separate debug info for /lib/libxyz.so.1

Progress output is written on a single line that is overwritten with
whitespace and rewritten to for each update or tick of the throbber.
This facilitates seamlessly switching from a throbber to a percentage
indicator part way through the transfer.

To reduce visual noise, when a transfer completes without an error
the line of progress output is again overwritten with whitespace.
This prevents many lines of progress output from accumulating when
many debuginfo files are downloaded.

In case a user would like to see a persistant record of the files
gdb aquired through debuginfod, an additional 'set debuginfod verbose N'
setting has been added. If N is at least 2, then the following will
be printed for each file that is downloaded or aquired from a local
debuginfod cache:

    Retrieved 1.23 MB separate debug info for /lib/libxyz.so.1

When --interpreter=mi or the output stream is not a tty, progress
updates consist of the following printed at the start of each
download:

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

No further progress updating will occur in this case.
---
 gdb/cli-out.c            | 149 +++++++++++++++++++++++++++++----------
 gdb/cli-out.h            |  30 +++-----
 gdb/debuginfod-support.c | 140 +++++++++++++++++++++++++++---------
 gdb/mi/mi-out.c          |  33 +++++++++
 gdb/mi/mi-out.h          |  20 ++++--
 gdb/ui-out.h             |  62 ++++++++++++----
 6 files changed, 328 insertions(+), 106 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 5ff645b4a83..9cc7e456583 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -265,10 +265,17 @@ 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>
+/* The cli_ui_out::do_progress_{start, notify} functions result in
+   the following:
+
+   - printed for tty, SHOULD_PRINT == true
+      - next state == PERCENT:
+         <(XX%) NAME\r>
+      - next state == SPIN:
+         <-\|/ NAME\r>
+      - next state == BAR:
+         <NAME
+         [#####                      ]\r>
    - printed for tty, SHOULD_PRINT == false:
      <>
    - printed for not-a-tty:
@@ -280,15 +287,14 @@ void
 cli_ui_out::do_progress_start (const std::string &name, bool should_print)
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info meter;
+  cli_progress_info info;
 
-  meter.last_value = 0;
-  meter.name = name;
+  info.name = name;
   if (!stream->isatty ())
     {
-      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
+      fprintf_unfiltered (stream, "%s...\n", info.name.c_str ());
       gdb_flush (stream);
-      meter.printing = WORKING;
+      info.state = progress_update::WORKING;
     }
   else
     {
@@ -296,74 +302,141 @@ cli_ui_out::do_progress_start (const std::string &name, bool should_print)
 	 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;
+      info.state = should_print ? progress_update::START
+				: progress_update::NO_PRINT;
     }
 
-  m_meters.push_back (std::move (meter));
+  m_progress_info.push_back (std::move (info));
 }
 
+/* Pick a reasonable limit for the progress update length.  */
+#define MAX_CHARS_PER_LINE 4096
+
 void
-cli_ui_out::do_progress_notify (double howmuch)
+cli_ui_out::do_progress_notify (double howmuch,
+			        progress_update::state next_state)
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info &meter (m_meters.back ());
+  cli_progress_info &info (m_progress_info.back ());
 
-  if (meter.printing == NO_PRINT)
+  if (info.state == progress_update::NO_PRINT)
     return;
 
-  if (meter.printing == START)
+  int chars_per_line = get_chars_per_line ();
+  if (chars_per_line > MAX_CHARS_PER_LINE)
+    chars_per_line = MAX_CHARS_PER_LINE;
+
+  if (info.state == progress_update::START)
     {
-      fprintf_unfiltered (stream, "%s\n", meter.name.c_str ());
+      fprintf_unfiltered (stream, "%s", info.name.c_str ());
+      if (chars_per_line <= 0)
+	fprintf_unfiltered (stream, "...\n");
       gdb_flush (stream);
-      meter.printing = WORKING;
+      info.state = progress_update::WORKING;
     }
 
-  if (meter.printing == WORKING && howmuch >= 1.0)
+  if (chars_per_line <= 0)
+    return;
+
+  if (info.state == progress_update::WORKING && howmuch >= 1.0)
     return;
 
   if (!stream->isatty ())
     return;
 
-  int chars_per_line = get_chars_per_line ();
-  if (chars_per_line > 0)
+  if (next_state == progress_update::PERCENT)
+    {
+      fprintf_unfiltered (stream, "\r(%2.0f%%) %s",
+			  howmuch * 100, info.name.c_str ());
+      gdb_flush (stream);
+      info.state = progress_update::PERCENT;
+    }
+  else if (next_state == progress_update::SPIN)
+    {
+      using namespace std::chrono;
+      seconds diff = duration_cast<seconds>
+	(steady_clock::now () - info.last_update);
+
+      /* Advance the spinner no faster than 1 tick per second.  */
+      if (diff.count () >= 1.0)
+	{
+	  static int spin = 0;
+
+	  fprintf_unfiltered (stream, "\r%c %s", "-\\|/"[spin++ % 4],
+			      info.name.c_str ());
+	  gdb_flush (stream);
+	  info.last_update = steady_clock::now ();
+	}
+      info.state = progress_update::SPIN;
+    }
+  else if (next_state == progress_update::BAR)
     {
       int i, max;
       int width = chars_per_line - 3;
-
       max = width * howmuch;
+
+      if (info.state == progress_update::SPIN
+	  || info.state == progress_update::PERCENT)
+	{
+	  /* Ensure the progress bar prints on its own line so that
+	     progress updates don't overwrite name.  */
+	  fprintf_unfiltered (stream, "\r%s\n", info.name.c_str ());
+	  gdb_flush (stream);
+	}
+
       fprintf_unfiltered (stream, "\r[");
+
       for (i = 0; i < width; ++i)
 	fprintf_unfiltered (stream, i < max ? "#" : " ");
       fprintf_unfiltered (stream, "]");
       gdb_flush (stream);
-      meter.printing = PROGRESS;
+      info.state = progress_update::BAR;
     }
+
+  return;
+}
+
+/* Set NAME as the new description of the most recent progress update.  */
+
+void
+cli_ui_out::update_progress_name (const std::string &name)
+{
+  cli_progress_info &info = m_progress_info.back ();
+  info.name = name;
 }
 
+cli_ui_out::progress_update::state
+cli_ui_out::get_progress_state ()
+{
+  cli_progress_info &info = m_progress_info.back ();
+  return info.state;
+}
+
+/* Clear the current line of the most recent progress update.  */
+
 void
 cli_ui_out::do_progress_end ()
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info &meter = m_meters.back ();
+  m_progress_info.pop_back ();
 
   if (!stream->isatty ())
-    {
-      fprintf_unfiltered (stream, "\n");
-      gdb_flush (stream);
-    }
-  else if (meter.printing == PROGRESS)
-    {
-      int i;
-      int width = get_chars_per_line () - 3;
+    return;
 
-      fprintf_unfiltered (stream, "\r");
-      for (i = 0; i < width + 2; ++i)
-	fprintf_unfiltered (stream, " ");
-      fprintf_unfiltered (stream, "\r");
-      gdb_flush (stream);
-    }
+  int chars_per_line = get_chars_per_line ();
+  if (chars_per_line <= 0
+      || chars_per_line > MAX_CHARS_PER_LINE)
+    chars_per_line = MAX_CHARS_PER_LINE;
+
+  int i;
+  int width = get_chars_per_line () - 3;
+
+  fprintf_unfiltered (stream, "\r");
+  for (i = 0; i < width + 2; ++i)
+    fprintf_unfiltered (stream, " ");
+  fprintf_unfiltered (stream, "\r");
 
-  m_meters.pop_back ();
+  gdb_flush (stream);
 }
 
 /* local functions */
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 4af5524495a..ee287161cf0 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
@@ -72,8 +73,10 @@ class cli_ui_out : public ui_out
   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_notify (double, progress_update::state) override;
   virtual void do_progress_end () override;
+  virtual void update_progress_name (const std::string &) override;
+  virtual progress_update::state get_progress_state () override;
 
   bool suppress_output ()
   { return m_suppress_output; }
@@ -85,32 +88,19 @@ 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;
+    progress_update::state state;
     /* The name to print.  */
     std::string name;
-    /* The last notification value.  */
-    double last_value;
+    /* Time of last spinner 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;
 };
 
 extern cli_ui_out *cli_out_new (struct ui_file *stream);
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 56d8e7781c5..20cf6688ccc 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -23,6 +23,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.  */
@@ -76,13 +77,13 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
 struct user_data
 {
-  user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname)
+  user_data (const char *desc, string_file &styled_fname)
+    : desc (desc), styled_fname (styled_fname)
   { }
 
   const char * const desc;
-  const char * const fname;
-  gdb::optional<ui_out::progress_meter> meter;
+  string_file & styled_fname;
+  gdb::optional<ui_out::progress_update> progress;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -98,6 +99,25 @@ 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 or MB.
+   UNIT will be set to "KB" or "MB" accordingly.  */
+
+static void
+get_size_and_unit (double *size, const char **unit)
+{
+  *size /= 1024;
+
+  /* If size is greater than 0.01 MB then set unit to MB.  */
+  if (*size > 10.24)
+    {
+      *size /= 1024;
+      *unit = "MB";
+    }
+  else
+    *unit = "KB";
+}
+
 static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
@@ -106,31 +126,57 @@ progressfn (debuginfod_client *c, long cur, long total)
 
   if (check_quit_flag ())
     {
-      printf_filtered ("Cancelling download of %s %ps...\n",
-		       data->desc,
-		       styled_string (file_name_style.style (), data->fname));
+      if (data->progress.has_value ())
+	data->progress.reset ();
+
+      printf_filtered ("Cancelled download of %s %s\n",
+		       data->desc, data->styled_fname.c_str ());
       return 1;
     }
 
-  if (total == 0)
+  if (debuginfod_verbose == 0
+      || (data->progress.has_value ()
+	  && data->progress->get_state () == ui_out::progress_update::WORKING))
     return 0;
 
-  if (!data->meter.has_value ())
+  /* Print progress update.  Include the transfer size if available.  */
+  if (total > 0)
     {
-      float size_in_mb = 1.0f * total / (1024 * 1024);
-      string_file styled_filename (current_uiout->can_emit_style_escape ());
-      fprintf_styled (&styled_filename,
-		      file_name_style.style (),
-		      "%s",
-		      data->fname);
-      std::string message
-	= string_printf ("Downloading %.2f MB %s %s", size_in_mb, data->desc,
-			 styled_filename.c_str());
-      data->meter.emplace (current_uiout, message, 1);
+      /* Transfer size is known.  */
+      if (!data->progress.has_value ()
+	  || data->progress->get_state () != ui_out::progress_update::PERCENT)
+	{
+	  double size = (double)total;
+	  const char *unit = "";
+
+	  get_size_and_unit (&size, &unit);
+	  std::string message = string_printf ("Downloading %.2f %s %s %s",
+					       size, unit, data->desc,
+					       data->styled_fname.c_str ());
+
+	  if (!data->progress.has_value ())
+	    data->progress.emplace (current_uiout, message, 1);
+	  else
+	    data->progress->update_name (message);
+	}
+
+	double percent = (double)cur / (double)total;
+	if (percent >= 0.0 || percent <= 100.0)
+	  {
+	    current_uiout->update_progress_percent (percent);
+	    return 0;
+	  }
     }
 
-  current_uiout->progress ((double)cur / (double)total);
+  if (!data->progress.has_value ()
+      || data->progress->get_state () != ui_out::progress_update::SPIN)
+    {
+      std::string message = string_printf ("Downloading %s %s", data->desc,
+					   data->styled_fname.c_str ());
+      data->progress.emplace (current_uiout, message, 1);
+    }
 
+  current_uiout->update_progress_spin ();
   return 0;
 }
 
@@ -186,6 +232,40 @@ 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.  */
+  if (data.progress.has_value ())
+    data.progress.reset ();
+
+  if (debuginfod_verbose > 1 && fd >= 0)
+    {
+      struct stat s;
+
+      if (fstat (fd, &s) == 0)
+	{
+	  double size = (double)s.st_size;
+	  const char *unit = "";
+
+	  get_size_and_unit (&size, &unit);
+	  printf_filtered (_("Retrieved %.02f %s %s %s\n"), size, unit,
+			   data.desc, data.styled_fname.c_str ());
+	}
+      else
+	warning (_("Retrieved %s %s but size cannot be read: %s\n"),
+		 data.desc, data.styled_fname.c_str (),
+		 safe_strerror (errno));
+    }
+  else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT)
+    printf_filtered (_("Download failed: %s. " \
+		       "Continuing without %s %s.\n"),
+		     safe_strerror (-fd), data.desc,
+		     data.styled_fname.c_str ());
+}
+
 /* See debuginfod-support.h  */
 
 scoped_fd
@@ -202,7 +282,9 @@ debuginfod_source_query (const unsigned char *build_id,
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
 
-  user_data data ("source file", srcpath);
+  string_file styled_srcpath (current_uiout->can_emit_style_escape ());
+  fprintf_styled (&styled_srcpath, file_name_style.style (), "%s", srcpath);
+  user_data data ("source file", styled_srcpath);
 
   debuginfod_set_user_data (c, &data);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
@@ -218,11 +300,7 @@ debuginfod_source_query (const unsigned char *build_id,
 					srcpath,
 					nullptr));
   debuginfod_set_user_data (c, nullptr);
-
-  if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
-    printf_filtered (_("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 = make_unique_xstrdup (srcpath);
@@ -247,7 +325,9 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
     return scoped_fd (-ENOMEM);
 
   char *dname = nullptr;
-  user_data data ("separate debug info for", filename);
+  string_file styled_filename (current_uiout->can_emit_style_escape ());
+  fprintf_styled (&styled_filename, file_name_style.style (), "%s", filename);
+  user_data data ("separate debug info for", styled_filename);
 
   debuginfod_set_user_data (c, &data);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
@@ -260,11 +340,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 (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
-    printf_filtered (_("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);
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index 20c6f0f9194..ad89d7400f8 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -258,6 +258,39 @@ mi_ui_out::main_stream ()
   return (string_file *) m_streams.back ();
 }
 
+/* Indicate that a task described by NAME is in progress:
+
+   - SHOULD_PRINT == true:
+       <NAME...
+       >
+   - SHOULD_PRINT == false:
+       <>
+*/
+
+void
+mi_ui_out::do_progress_start (const std::string &name, bool should_print)
+{
+  struct ui_file *stream = gdb_stdout;
+  mi_progress_info info;
+
+  if (should_print)
+    {
+      fprintf_unfiltered (stream, "%s...\n", name.c_str ());
+      gdb_flush (stream);
+    }
+  info.state = progress_update::WORKING;
+  m_progress_info.push_back (std::move (info));
+}
+
+/* Get the state of the most recent progress update.  */
+
+mi_ui_out::progress_update::state
+mi_ui_out::get_progress_state ()
+{
+  mi_progress_info &info = m_progress_info.back ();
+  return info.state;
+}
+
 /* Clear the buffer.  */
 
 void
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 1b7fa96a182..d915b35f633 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,15 +82,18 @@ 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_start (const std::string &, bool) override;
+  virtual progress_update::state get_progress_state () override;
+
+  virtual void do_progress_notify (double, progress_update::state) override
   {
   }
 
-  virtual void do_progress_notify (double) override
+  virtual void do_progress_end () override
   {
   }
 
-  virtual void do_progress_end () override
+  virtual void update_progress_name (const std::string &) override
   {
   }
 
@@ -101,6 +103,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 05312150c21..4c490c11b82 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -280,26 +280,52 @@ 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,
+      /* Spinner printing has already started.  */
+      SPIN,
+      /* Percent printing has already started.  */
+      PERCENT,
+      /* Printing should not be done.  */
+      NO_PRINT
+    };
+
     /* SHOULD_PRINT indicates whether something should be printed for a tty.  */
-    progress_meter (struct ui_out *uiout, const std::string &name,
-		    bool should_print)
+    progress_update (struct ui_out *uiout, const std::string &name,
+		     bool should_print)
       : m_uiout (uiout)
     {
       m_uiout->do_progress_start (name, should_print);
     }
 
-    ~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;
+    void update_name (std::string &name)
+    {
+      m_uiout->update_progress_name (name);
+    }
+
+    state get_state ()
+    {
+      return m_uiout->get_progress_state ();
+    }
+
+    progress_update (const progress_update &) = delete;
+    progress_update &operator= (const progress_update &) = delete;
 
   private:
 
@@ -307,10 +333,20 @@ class ui_out
   };
 
   /* Emit some progress corresponding to the most recently created
-     progress meter.  HOWMUCH may range from 0.0 to 1.0.  */
-  void progress (double howmuch)
+     progress_update object.  */
+  void update_progress_bar (double howmuch)
+  {
+    do_progress_notify (howmuch, progress_update::BAR);
+  }
+
+  void update_progress_percent (double howmuch)
+  {
+    do_progress_notify (howmuch, progress_update::PERCENT);
+  }
+
+  void update_progress_spin ()
   {
-    do_progress_notify (howmuch);
+    do_progress_notify (0, progress_update::SPIN);
   }
 
  protected:
@@ -348,8 +384,10 @@ class ui_out
   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_notify (double, progress_update::state) = 0;
   virtual void do_progress_end () = 0;
+  virtual void update_progress_name (const std::string &) = 0;
+  virtual progress_update::state get_progress_state () = 0;
 
   /* Set as not MI-like by default.  It is overridden in subclasses if
      necessary.  */
-- 
2.34.1

[PATCH v3] gdb: Improve debuginfod progress updates

[Aaron Merey]

v2 can be found here:
https://sourceware.org/pipermail/gdb-patches/2022-January/185432.html

v3 adds truncation of progress update messages when printed to a
terminal that is not wide enough to fit the message on one line.

The new function build_message will try to fit as much of the
untruncated message as possible.  For example if the original
message is:

  Downloading XX MB separate debug info for /aa/bb/cc/dd/ee

Then possible truncated messages are:

  Downloading XX MB separate debug info for /aa/bb/.../ee
  Downloading XX MB separate debug info for ee
  Downloading XX MB separate debug info
  Downloading XX MB
  Downloading

---
 gdb/cli-out.c            | 170 ++++++++++++++++++------
 gdb/cli-out.h            |  31 ++---
 gdb/debuginfod-support.c | 276 ++++++++++++++++++++++++++++++++++-----
 gdb/mi/mi-out.c          |  34 +++++
 gdb/mi/mi-out.h          |  20 ++-
 gdb/ui-out.h             |  62 +++++++--
 6 files changed, 486 insertions(+), 107 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 5ff645b4a83..3523b074e75 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -265,10 +265,17 @@ 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>
+/* The cli_ui_out::do_progress_{start, notify} functions result in
+   the following:
+
+   - printed for tty, SHOULD_PRINT == true
+      - next state == PERCENT:
+	<(XX%) NAME\r>
+      - next state == SPIN:
+	<-\|/ NAME\r>
+      - next state == BAR:
+	<NAME
+	[#####                      ]\r>
    - printed for tty, SHOULD_PRINT == false:
      <>
    - printed for not-a-tty:
@@ -280,15 +287,14 @@ void
 cli_ui_out::do_progress_start (const std::string &name, bool should_print)
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info meter;
+  cli_progress_info info;
 
-  meter.last_value = 0;
-  meter.name = name;
+  info.name = name;
   if (!stream->isatty ())
     {
-      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
+      fprintf_unfiltered (stream, "%s\n", info.name.c_str ());
       gdb_flush (stream);
-      meter.printing = WORKING;
+      info.state = progress_update::WORKING;
     }
   else
     {
@@ -296,74 +302,158 @@ cli_ui_out::do_progress_start (const std::string &name, bool should_print)
 	 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;
+      info.state = should_print ? progress_update::START
+				: progress_update::NO_PRINT;
     }
 
-  m_meters.push_back (std::move (meter));
+  m_progress_info.push_back (std::move (info));
 }
 
+/* Pick a reasonable limit for the progress update length.  */
+#define MAX_CHARS_PER_LINE 4096
+
 void
-cli_ui_out::do_progress_notify (double howmuch)
+cli_ui_out::do_progress_notify (double howmuch,
+			        progress_update::state next_state)
 {
   struct ui_file *stream = m_streams.back ();
-  cli_progress_info &meter (m_meters.back ());
+  cli_progress_info &info (m_progress_info.back ());
 
-  if (meter.printing == NO_PRINT)
+  if (info.state == progress_update::NO_PRINT)
     return;
 
-  if (meter.printing == START)
+  int chars_per_line = get_chars_per_line ();
+  if (chars_per_line > MAX_CHARS_PER_LINE)
+    chars_per_line = MAX_CHARS_PER_LINE;
+
+  if (info.state == progress_update::START)
     {
-      fprintf_unfiltered (stream, "%s\n", meter.name.c_str ());
+      fprintf_unfiltered (stream, "%s", info.name.c_str ());
+      if (chars_per_line <= 0)
+	fprintf_unfiltered (stream, "\n");
       gdb_flush (stream);
-      meter.printing = WORKING;
+      info.state = progress_update::WORKING;
     }
 
-  if (meter.printing == WORKING && howmuch >= 1.0)
+  if (chars_per_line <= 0)
+    return;
+
+  if (info.state == progress_update::WORKING && howmuch >= 1.0)
     return;
 
   if (!stream->isatty ())
     return;
 
-  int chars_per_line = get_chars_per_line ();
-  if (chars_per_line > 0)
+  if (next_state == progress_update::PERCENT)
+    {
+      fprintf_unfiltered (stream, "\r(%2.0f%%) %s",
+			  howmuch * 100, info.name.c_str ());
+      gdb_flush (stream);
+      info.state = progress_update::PERCENT;
+    }
+  else if (next_state == progress_update::SPIN)
+    {
+      using namespace std::chrono;
+      seconds diff = duration_cast<seconds>
+	(steady_clock::now () - info.last_update);
+
+      /* Advance the spinner no faster than 1 tick per second.  */
+      if (diff.count () >= 1.0)
+	{
+	  static int spin = 0;
+
+	  fprintf_unfiltered (stream, "\r%c %s", "-\\|/"[spin++ % 4],
+			      info.name.c_str ());
+	  gdb_flush (stream);
+	  info.last_update = steady_clock::now ();
+	}
+      info.state = progress_update::SPIN;
+    }
+  else if (next_state == progress_update::BAR)
     {
       int i, max;
       int width = chars_per_line - 3;
-
       max = width * howmuch;
+
+      if (info.state == progress_update::SPIN
+	  || info.state == progress_update::PERCENT)
+	{
+	  /* Ensure the progress bar prints on its own line so that
+	     progress updates don't overwrite NAME.  */
+	  fprintf_unfiltered (stream, "\r%s\n", info.name.c_str ());
+	  gdb_flush (stream);
+	}
+
       fprintf_unfiltered (stream, "\r[");
+
       for (i = 0; i < width; ++i)
 	fprintf_unfiltered (stream, i < max ? "#" : " ");
       fprintf_unfiltered (stream, "]");
       gdb_flush (stream);
-      meter.printing = PROGRESS;
+      info.state = progress_update::BAR;
     }
+
+  return;
 }
 
+/* Clear the current line of the most recent progress update.  Overwrites
+   the current line with whitespace.  */
+
 void
-cli_ui_out::do_progress_end ()
+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 (!stream->isatty ())
-    {
-      fprintf_unfiltered (stream, "\n");
-      gdb_flush (stream);
-    }
-  else if (meter.printing == PROGRESS)
-    {
-      int i;
-      int width = get_chars_per_line () - 3;
+  if (chars_per_line <= 0
+      || chars_per_line > MAX_CHARS_PER_LINE)
+    chars_per_line = MAX_CHARS_PER_LINE;
 
-      fprintf_unfiltered (stream, "\r");
-      for (i = 0; i < width + 2; ++i)
-	fprintf_unfiltered (stream, " ");
-      fprintf_unfiltered (stream, "\r");
-      gdb_flush (stream);
-    }
+  int i;
+  int width = chars_per_line;
+
+  fprintf_unfiltered (stream, "\r");
+  for (i = 0; i < width; ++i)
+    fprintf_unfiltered (stream, " ");
+  fprintf_unfiltered (stream, "\r");
+
+  gdb_flush (stream);
+}
+
+/* Set NAME as the new description of the most recent progress update.  */
+
+void
+cli_ui_out::update_progress_name (const std::string &name)
+{
+  struct ui_file *stream = m_streams.back ();
+  cli_progress_info &info = m_progress_info.back ();
+  info.name = name;
+
+  if (stream->isatty ())
+    clear_current_line ();
+}
+
+/* Get the current state of the most recent progress update.  */
+
+cli_ui_out::progress_update::state
+cli_ui_out::get_progress_state ()
+{
+  cli_progress_info &info = m_progress_info.back ();
+  return info.state;
+}
+
+
+/* Remove the most recent progress update from the stack and
+   overwrite the current line with whitespace.  */
+
+void
+cli_ui_out::do_progress_end ()
+{
+  struct ui_file *stream = m_streams.back ();
+  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 4af5524495a..2afd5b32706 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
@@ -72,8 +73,10 @@ class cli_ui_out : public ui_out
   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_notify (double, progress_update::state) override;
   virtual void do_progress_end () override;
+  virtual void update_progress_name (const std::string &) override;
+  virtual progress_update::state get_progress_state () override;
 
   bool suppress_output ()
   { return m_suppress_output; }
@@ -85,32 +88,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;
+    progress_update::state state;
     /* The name to print.  */
     std::string name;
-    /* The last notification value.  */
-    double last_value;
+    /* Time of last spinner 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 cli_ui_out *cli_out_new (struct ui_file *stream);
diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 56d8e7781c5..34a72618663 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -23,7 +23,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;
@@ -76,13 +78,13 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 
 struct user_data
 {
-  user_data (const char *desc, const char *fname)
+  user_data (const char *desc, std::string &fname)
     : desc (desc), fname (fname)
   { }
 
   const char * const desc;
-  const char * const fname;
-  gdb::optional<ui_out::progress_meter> meter;
+  std::string & fname;
+  gdb::optional<ui_out::progress_update> progress;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -98,6 +100,149 @@ 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 or MB.
+   UNIT will be set to "KB" or "MB" accordingly.  */
+
+static void
+get_size_and_unit (double *size, const char **unit)
+{
+  *size /= 1024;
+
+  /* If size is greater than 0.01 MB then set unit to MB.  */
+  if (*size > 10.24)
+    {
+      *size /= 1024;
+      *unit = "MB";
+    }
+  else
+    *unit = "KB";
+}
+
+/* Ensure the progress message can fit on a single line. Otherwise
+   garbled output is possible with \r.
+
+   An example of possible truncations, starting with the original message:
+   "Downloading XX MB separate debug info for /aa/bb/cc/dd/ee"
+   "Downloading XX MB separate debug info for /aa/bb/.../ee"
+   "Downloading XX MB separate debug info for ee"
+   "Downloading XX MB separate debug info"
+   "Downloading XX MB"
+   "Downloading"
+  */
+
+static std::string
+build_message (std::string size, std::string unit, std::string desc, std::string fname)
+{
+  int width = get_chars_per_line ();
+  std::stringstream message;
+
+  message << "Downloading";
+  /* Leave room for spinner and percent indicator.  */
+  int message_size = message.str ().length () + 6;
+
+  if (!size.empty () && !unit.empty ())
+    {
+      message_size += size.size () + unit.size () + 2;
+      if (message_size > width)
+	return message.str ();
+
+      /* "Downloading XX MB" */
+      message << " " << size << " " << unit;
+    }
+
+  /* If FNAME does not fit then message will end with DESC_END.
+     In case DESC_END is "separate debug info for", remove " for".  */
+  std::string desc_end = desc;
+  if (desc.substr (desc.size () - 4) == " for")
+    desc_end = desc.substr (0, desc.size () - 4);
+
+  if (message_size + desc_end.size () + 1 > width)
+    return message.str ();
+
+  string_file styled_fname (current_uiout->can_emit_style_escape ());
+  if (message_size + desc.size () + fname.size () + 2 <= width)
+    {
+      /* Truncation is not necessary. Return untruncated message.
+	 "Downloading XX MB separate debug info for /usr/libxyz.so" */
+      fprintf_styled (&styled_fname, file_name_style.style (), "%s",
+		      fname.c_str ());
+
+      message << " " << desc << " " << styled_fname.c_str ();
+      return message.str ();
+    }
+
+   while (fname.back () == '/')
+    fname.pop_back ();
+
+  /* Find path separators for the first, second and final components.
+     If FNAME does not have path separators and it does not fit in the
+     available space, do not include it in message.  */
+  size_t sep1 = fname.find ('/');
+  if (sep1 == std::string::npos)
+    {
+      message << " " << desc_end;
+      return message.str ();
+    }
+
+  size_t sep2 = fname.find ('/', sep1 + 1);
+  size_t sep3;
+  if (sep2 == std::string::npos)
+    sep3 = std::string::npos;
+  else
+    sep3 = fname.find ('/', sep2 + 1);
+  size_t seplast = fname.find_last_of ('/');
+
+  /* If the first, second, and final path components are distinct, try to
+     truncate FNAME so that it fits in the available space.  Preserve the
+     first, second and final path components.  For example,
+     "/aa/bb/cc/dd/ee" becomes "/aa/bb/.../ee" and
+     "../aa/bb/cc/dd/" becomes "../aa/.../ee"  */
+  std::stringstream trunc;
+  if (sep2 != sep3 && sep2 != seplast && sep2 != std::string::npos)
+    {
+      std::stringstream fnametmp;
+
+      if (sep1 == 0 && sep3 != seplast && sep3 != std::string::npos)
+	fnametmp << fname.substr (0, sep3 + 1)
+		 << "..." << fname.substr (seplast);
+      else if (sep1 != 0)
+	fnametmp << fname.substr (0, sep2 + 1)
+		 << "..." << fname.substr (seplast);
+
+      if (!fnametmp.str ().empty ())
+	{
+	  trunc << " " << desc << " ";
+	  if (message_size + trunc.str ().size () + fnametmp.str ().size () <= width)
+	    {
+	      fprintf_styled (&styled_fname, file_name_style.style (), "%s",
+			      fnametmp.str ().c_str ());
+	      message << trunc.str () << styled_fname.c_str ();
+	      return message.str ();
+	    }
+	}
+    }
+
+  /* The first, second and final components are not distinct or
+     "/aa/bb/.../ee" does not fit. Try "ee" instead.  */
+  trunc.str ("");
+  trunc << " " << desc << " ";
+  fname = fname.substr (seplast + 1);
+  if (message_size + trunc.str ().size () + fname.size () <= width)
+    {
+      fprintf_styled (&styled_fname, file_name_style.style (), "%s",
+		      fname.c_str ());
+      message << trunc.str () << styled_fname.c_str ();
+      return message.str ();
+    }
+
+  /* We aren't able to fit anything from FNAME. End message with DESC_END
+     since we already confirmed it will fit.  */
+  message << " " << desc_end;
+  return message.str ();
+}
+
+
 static int
 progressfn (debuginfod_client *c, long cur, long total)
 {
@@ -106,31 +251,68 @@ progressfn (debuginfod_client *c, long cur, long total)
 
   if (check_quit_flag ())
     {
-      printf_filtered ("Cancelling download of %s %ps...\n",
-		       data->desc,
-		       styled_string (file_name_style.style (), data->fname));
+      if (data->progress.has_value ())
+	data->progress.reset ();
+
+      string_file styled_fname (current_uiout->can_emit_style_escape ());
+      fprintf_styled (&styled_fname, file_name_style.style (), "%s",
+		      data->fname.c_str ());
+
+      printf_filtered ("Cancelled download of %s %s\n",
+		       data->desc, styled_fname.c_str ());
       return 1;
     }
 
-  if (total == 0)
+  if (debuginfod_verbose == 0
+      || (data->progress.has_value ()
+	  && data->progress->get_state () == ui_out::progress_update::WORKING))
     return 0;
 
-  if (!data->meter.has_value ())
+  /* Print progress update.  Include the transfer size if available.  */
+  if (total > 0)
     {
-      float size_in_mb = 1.0f * total / (1024 * 1024);
-      string_file styled_filename (current_uiout->can_emit_style_escape ());
-      fprintf_styled (&styled_filename,
-		      file_name_style.style (),
-		      "%s",
-		      data->fname);
-      std::string message
-	= string_printf ("Downloading %.2f MB %s %s", size_in_mb, data->desc,
-			 styled_filename.c_str());
-      data->meter.emplace (current_uiout, message, 1);
+      /* Transfer size is known.  */
+      double percent = (double)cur / (double)total;
+
+      if (percent >= 0.0 && percent <= 1.0)
+	{
+	  if (!data->progress.has_value ()
+	      || data->progress->get_state ()
+		 != ui_out::progress_update::PERCENT)
+	    {
+	      double size = (double)total;
+	      const char *unit = "";
+
+	      get_size_and_unit (&size, &unit);
+	      std::string fsize = string_printf ("%.2f", size);
+	      std::string message = build_message (fsize, unit, data->desc,
+						   data->fname);
+	      if (!data->progress.has_value ())
+		data->progress.emplace (current_uiout, message, 1);
+	      else
+		data->progress->update_name (message);
+	    }
+
+	    /* Ensure PERCENT doesn't require three digits to display.  */
+	    if (percent > 0.99 && percent <= 1.0)
+	      percent = .99;
+	    current_uiout->update_progress_percent (percent);
+	    return 0;
+	}
     }
 
-  current_uiout->progress ((double)cur / (double)total);
+  if (!data->progress.has_value ()
+      || data->progress->get_state () != ui_out::progress_update::SPIN)
+    {
+      std::string message = build_message ("", "", data->desc, data->fname);
+
+      if (!data->progress.has_value ())
+	data->progress.emplace (current_uiout, message, 1);
+      else
+	data->progress->update_name (message);
+    }
 
+  current_uiout->update_progress_spin ();
   return 0;
 }
 
@@ -186,6 +368,44 @@ 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.  */
+  if (data.progress.has_value ())
+    data.progress.reset ();
+
+  string_file styled_fname (current_uiout->can_emit_style_escape ());
+  fprintf_styled (&styled_fname, file_name_style.style (), "%s",
+		  data.fname.c_str ());
+
+  if (debuginfod_verbose > 1 && fd >= 0)
+    {
+      struct stat s;
+
+      if (fstat (fd, &s) == 0)
+	{
+	  double size = (double)s.st_size;
+	  const char *unit = "";
+
+	  get_size_and_unit (&size, &unit);
+	  printf_filtered (_("Retrieved %.02f %s %s %s\n"), size, unit,
+			   data.desc, styled_fname.c_str ());
+	}
+      else
+	warning (_("Retrieved %s %s but size cannot be read: %s\n"),
+		 data.desc, styled_fname.c_str (),
+		 safe_strerror (errno));
+    }
+  else if (debuginfod_verbose > 0 && fd < 0 && fd != -ENOENT)
+    printf_filtered (_("Download failed: %s. " \
+		       "Continuing without %s %s.\n"),
+		     safe_strerror (-fd), data.desc,
+		     styled_fname.c_str ());
+}
+
 /* See debuginfod-support.h  */
 
 scoped_fd
@@ -202,7 +422,8 @@ debuginfod_source_query (const unsigned char *build_id,
   if (c == nullptr)
     return scoped_fd (-ENOMEM);
 
-  user_data data ("source file", srcpath);
+  std::string fname = srcpath;
+  user_data data ("source file", fname);
 
   debuginfod_set_user_data (c, &data);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
@@ -218,11 +439,7 @@ debuginfod_source_query (const unsigned char *build_id,
 					srcpath,
 					nullptr));
   debuginfod_set_user_data (c, nullptr);
-
-  if (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
-    printf_filtered (_("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 = make_unique_xstrdup (srcpath);
@@ -247,7 +464,8 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
     return scoped_fd (-ENOMEM);
 
   char *dname = nullptr;
-  user_data data ("separate debug info for", filename);
+  std::string fname = filename;
+  user_data data ("separate debug info for", fname);
 
   debuginfod_set_user_data (c, &data);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
@@ -260,11 +478,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 (debuginfod_verbose > 0 && fd.get () < 0 && fd.get () != -ENOENT)
-    printf_filtered (_("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);
diff --git a/gdb/mi/mi-out.c b/gdb/mi/mi-out.c
index 20c6f0f9194..a7f697b4f28 100644
--- a/gdb/mi/mi-out.c
+++ b/gdb/mi/mi-out.c
@@ -258,6 +258,40 @@ mi_ui_out::main_stream ()
   return (string_file *) m_streams.back ();
 }
 
+/* Indicate that a task described by NAME is in progress:
+
+   - SHOULD_PRINT == true:
+     <NAME
+     >
+   - SHOULD_PRINT == false:
+     <>
+*/
+
+void
+mi_ui_out::do_progress_start (const std::string &name, bool should_print)
+{
+  struct ui_file *stream = gdb_stdout;
+  mi_progress_info info;
+
+  if (should_print)
+    {
+      fprintf_unfiltered (stream, "%s\n", name.c_str ());
+      gdb_flush (stream);
+    }
+
+  info.state = progress_update::WORKING;
+  m_progress_info.push_back (std::move (info));
+}
+
+/* Get the state of the most recent progress update.  */
+
+mi_ui_out::progress_update::state
+mi_ui_out::get_progress_state ()
+{
+  mi_progress_info &info = m_progress_info.back ();
+  return info.state;
+}
+
 /* Clear the buffer.  */
 
 void
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index 1b7fa96a182..d915b35f633 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,15 +82,18 @@ 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_start (const std::string &, bool) override;
+  virtual progress_update::state get_progress_state () override;
+
+  virtual void do_progress_notify (double, progress_update::state) override
   {
   }
 
-  virtual void do_progress_notify (double) override
+  virtual void do_progress_end () override
   {
   }
 
-  virtual void do_progress_end () override
+  virtual void update_progress_name (const std::string &) override
   {
   }
 
@@ -101,6 +103,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 05312150c21..4c490c11b82 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -280,26 +280,52 @@ 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,
+      /* Spinner printing has already started.  */
+      SPIN,
+      /* Percent printing has already started.  */
+      PERCENT,
+      /* Printing should not be done.  */
+      NO_PRINT
+    };
+
     /* SHOULD_PRINT indicates whether something should be printed for a tty.  */
-    progress_meter (struct ui_out *uiout, const std::string &name,
-		    bool should_print)
+    progress_update (struct ui_out *uiout, const std::string &name,
+		     bool should_print)
       : m_uiout (uiout)
     {
       m_uiout->do_progress_start (name, should_print);
     }
 
-    ~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;
+    void update_name (std::string &name)
+    {
+      m_uiout->update_progress_name (name);
+    }
+
+    state get_state ()
+    {
+      return m_uiout->get_progress_state ();
+    }
+
+    progress_update (const progress_update &) = delete;
+    progress_update &operator= (const progress_update &) = delete;
 
   private:
 
@@ -307,10 +333,20 @@ class ui_out
   };
 
   /* Emit some progress corresponding to the most recently created
-     progress meter.  HOWMUCH may range from 0.0 to 1.0.  */
-  void progress (double howmuch)
+     progress_update object.  */
+  void update_progress_bar (double howmuch)
+  {
+    do_progress_notify (howmuch, progress_update::BAR);
+  }
+
+  void update_progress_percent (double howmuch)
+  {
+    do_progress_notify (howmuch, progress_update::PERCENT);
+  }
+
+  void update_progress_spin ()
   {
-    do_progress_notify (howmuch);
+    do_progress_notify (0, progress_update::SPIN);
   }
 
  protected:
@@ -348,8 +384,10 @@ class ui_out
   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_notify (double, progress_update::state) = 0;
   virtual void do_progress_end () = 0;
+  virtual void update_progress_name (const std::string &) = 0;
+  virtual progress_update::state get_progress_state () = 0;
 
   /* Set as not MI-like by default.  It is overridden in subclasses if
      necessary.  */
-- 
2.34.1

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

[Patrick Monnerat]

Hi Aaron,

I finally found time to check your patch with Insight and it seems to work.

Please find some questions/remarks embedded in your code.


> +      /* Transfer size is known.  */
> +      double percent = (double)cur / (double)total;
The variable name is confusing as it is <= 1.0.
> +
> +      if (percent >= 0.0 && percent <= 1.0)
I don't think this test is needed: cur and total are obtained 
(indirectly) from curl and IMHO you can trust it.
> +     progress_update object.  */
> +  void update_progress_bar (double howmuch)

This is never called! why do you provide both PERCENT and (unused) BAR? 
This is a bit confusing.

Thanks for your work on it.

Patrick

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

[Aaron Merey]

Hi Patrick,

Thanks for taking another look.

On Sun, Feb 13, 2022 at 7:56 PM Patrick Monnerat <patrick@monnerat.net> wrote:
> > +      /* Transfer size is known.  */
> > +      double percent = (double)cur / (double)total;
> The variable name is confusing as it is <= 1.0.

update_progress_percent uses the name "howmuch" for this.  It would be
better if "howmuch" was used here too.

> > +
> > +      if (percent >= 0.0 && percent <= 1.0)
> I don't think this test is needed: cur and total are obtained
> (indirectly) from curl and IMHO you can trust it.

I've experienced at least one case where percent was > 1.0.  I haven't
been able to reproduce it because it seemed to coincide with a network
hiccup.  If percent isn't between 0 and 1 then we default to the spinner
to avoid printing a message with a nonsensical completion percentage.

> > +     progress_update object.  */
> > +  void update_progress_bar (double howmuch)
>
> This is never called! why do you provide both PERCENT and (unused) BAR?
> This is a bit confusing.

The progress update message originally included the bar but I now want to
only print messages that fit entirely on one line.  This makes it possible
to rewrite an entire message with transfer size information once it becomes
available.  Because the progress bar was already implemented I figured I'd
leave it in case it ends up serving a purpose in the future.

Aaron

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

[Patrick Monnerat]

Hi Aaron,

On 2/16/22 03:09, Aaron Merey wrote:
> Thanks for taking another look.
You're welcome.
>>> +      /* Transfer size is known.  */
>>> +      double percent = (double)cur / (double)total;
>> The variable name is confusing as it is <= 1.0.
> update_progress_percent uses the name "howmuch" for this.  It would be
> better if "howmuch" was used here too.
Good choice.
>>> +      if (percent >= 0.0 && percent <= 1.0)
>> I don't think this test is needed: cur and total are obtained
>> (indirectly) from curl and IMHO you can trust it.
> I've experienced at least one case where percent was > 1.0.  I haven't
> been able to reproduce it because it seemed to coincide with a network
> hiccup.
Very strange! TCP is supposed to guarantee no duplicate data reception. 
If this really occurs, debuginfo data are probably corrupted too!
> +     progress_update object.  */
> +  void update_progress_bar (double howmuch)
>> This is never called! why do you provide both PERCENT and (unused) BAR?
>> This is a bit confusing.
> The progress update message originally included the bar but I now want to
> only print messages that fit entirely on one line.  This makes it possible
> to rewrite an entire message with transfer size information once it becomes
> available.  Because the progress bar was already implemented I figured I'd
> leave it in case it ends up serving a purpose in the future.
Thanks for this precision. Maybe put it in a comment?


Cheers,

Patrick

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

[Aaron Merey]

On Wed, Feb 16, 2022 at 5:38 AM Patrick Monnerat <patrick@monnerat.net> wrote:
> > I've experienced at least one case where percent was > 1.0.  I haven't
> > been able to reproduce it because it seemed to coincide with a network
> > hiccup.
> Very strange! TCP is supposed to guarantee no duplicate data reception.
> If this really occurs, debuginfo data are probably corrupted too!

I have not seen any corrupt debuginfo acquired from debuginfod.
Each time debuginfo is downloaded there is a check to make sure it
has the expected build-id. The integrity of the debuginfo is also verified
using the CRC in the parent file's .gnu_debuglink section.

Aaron

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

[Tom Tromey]

>>>>> "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 [].

See the "indeterminate" example here:

    https://github.com/p-ranav/indicators

(That page made me LOL with the emoji rocket, maybe gdb should be more
emojified.)

Anyway it seems like this sort of change would avoid some hair in the
implementation.

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.

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);

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.

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.

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.

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.

Tom

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

[Aaron Merey]

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

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

[Tom Tromey]

>> 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 [].

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

Could you show what your proposed output looks like?
I mean, after everything is downloaded?

To me it seems like having a "Downloading..." message per .so is no big deal.
gdb is often chatty.

>> 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.

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

I am not sure we're talking about the same thing... MI defines a
progress indication message, but your patch isn't using that.  So I
wonder if any MI notification is needed at all.

Tom

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

[Aaron Merey]

On Fri, Mar 18, 2022 at 3:23 PM Tom Tromey <tom@tromey.com> wrote:
> Aaron> I'd like to restrict the update messages to a single line so that an
> Aaron> entire message can be overwritten with \r.  Then when many downloads
> Aaron> occur we avoid filling the terminal with "Downloading XY MB separate
> Aaron> debuginfo for libxyz" messages.
>
> Could you show what your proposed output looks like?
> I mean, after everything is downloaded?

After each download, the progress update message is overwritten with
whitespace. So by default there is no lasting indication that a download
occured (this can be changed with 'set debuginfod verbose 2' though).

For example while downloads are in progress a user might see

    (gdb) start
    Temporary breakpoint 1 at 0x40f406: file gdb.c, line 25.
    Starting program: /usr/local/bin/gdb
    \ Downloading separate debug info for /lib64/libc.so.6

where the last line is overwritten with each download's progress message.
When the final download finishes, the message is overwritten with whitespace
and gdb's output continues as if debuginfod wasn't used:

    (gdb) start
    Temporary breakpoint 1 at 0x40f406: file gdb.c, line 25.
    Starting program: /usr/local/bin/gdb
    [Thread debugging using libthread_db enabled]
    Using host libthread_db library "/lib64/libthread_db.so.1".

    Temporary breakpoint 1, main (argc=1, argv=0x7fffffffe658) at gdb.c:25
    25  {
    (gdb)

> To me it seems like having a "Downloading..." message per .so is no big deal.
> gdb is often chatty.

I'd like to avoid printing a large number of "Downloading..." messages but
my solution in this patch comes with the added complexity of message
truncation when the terminal isn't wide enough.  Unless there is a desire
among users to limit these messages this way then maybe it's better to
just stick to the cylon-style display you suggested.

> >> 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.
>
> Aaron> These MI implementations are the easiest way to see progress updates when
> Aaron> using gdb+debuginfod with IDEs, for instance.  Otherwise I think each IDE
> Aaron> would have to learn how to parse and display the mi_load_progress output,
> Aaron> preferably in a way that's consistent with the CLI progress messages.
>
> I am not sure we're talking about the same thing... MI defines a
> progress indication message, but your patch isn't using that.  So I
> wonder if any MI notification is needed at all.

I think I've conflated mi_ui_out with MI itself.  I don't think we'll need
to modify any MI notifications for these progress updates.

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

[Tom Tromey]

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

>> To me it seems like having a "Downloading..." message per .so is no big deal.
>> gdb is often chatty.

Aaron> I'd like to avoid printing a large number of "Downloading..." messages but
Aaron> my solution in this patch comes with the added complexity of message
Aaron> truncation when the terminal isn't wide enough.  Unless there is a desire
Aaron> among users to limit these messages this way then maybe it's better to
Aaron> just stick to the cylon-style display you suggested.

Yeah, personally I think having the Downloading message is fine, so
unless there are other contrary views, I'd say go with the simpler code
here.

Tom