[PATCH] gdb: print size of downloaded debuginfod binary

[PATCH] gdb: print size of downloaded debuginfod binary

[Martin Liška]

With the patch, one can see now:

Reading symbols from /usr/bin/gcc...
Downloading 2.69 MiB separate debug info for /usr/bin/gcc...
Downloading 1.00 MiB separate debug info for /lib64/ld-linux-x86-64.so.2...
Downloading 10.72 MiB separate debug info for /lib64/libc.so.6...
...

ChangeLog:

	* gdb/debuginfod-support.c (progressfn): Print size of the
	downloaded debunginfod binary.
---
  gdb/debuginfod-support.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index e21b2f40ca..ffc832e936 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -84,8 +84,8 @@ progressfn (debuginfod_client *c, long cur, long total)
      {
        /* Print this message only once.  */
        data->has_printed = true;
-      printf_filtered ("Downloading %s %ps...\n",
-		       data->desc,
+      printf_filtered ("Downloading %.2f MiB %s %ps...\n",
+		       1.0f * total / (1024 * 1024), data->desc,
  		       styled_string (file_name_style.style (), data->fname));
      }
  
-- 
2.29.2

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Simon Marchi via Gdb-patches]

On 2020-12-07 6:43 a.m., Martin Liška wrote:
> With the patch, one can see now:
> 
> Reading symbols from /usr/bin/gcc...
> Downloading 2.69 MiB separate debug info for /usr/bin/gcc...
> Downloading 1.00 MiB separate debug info for /lib64/ld-linux-x86-64.so.2...
> Downloading 10.72 MiB separate debug info for /lib64/libc.so.6...
> ...

I think that's a good idea.  Given that we don't have a progress bar 
(yet), this gives clue to the user about why it might take some time.

Let's see if others have other opinions about this.  If nothing comes 
for by next week, you can go ahead and merge it.

Simon

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On 12/7/20 6:07 PM, Simon Marchi wrote:
> 
> 
> On 2020-12-07 6:43 a.m., Martin Liška wrote:
>> With the patch, one can see now:
>>
>> Reading symbols from /usr/bin/gcc...
>> Downloading 2.69 MiB separate debug info for /usr/bin/gcc...
>> Downloading 1.00 MiB separate debug info for /lib64/ld-linux-x86-64.so.2...
>> Downloading 10.72 MiB separate debug info for /lib64/libc.so.6...
>> ...
> 

Thank you for the review.

> I think that's a good idea.  Given that we don't have a progress bar (yet), this gives clue to the user about why it might take some time.

Btw. I can also offer a WIP patch that implements the progress bar:

...
Temporary breakpoint 1 at 0x406150: file /home/marxin/Programming/gcc/gcc/gcc-main.c, line 44.
Starting program: /home/marxin/bin/gcc/bin/gcc
Downloading 1.00 MB separate debug info for /lib64/ld-linux-x86-64.so.2...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
Downloading 4.20 MB separate debug info for /lib64/libm.so.6...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
Downloading 10.72 MB separate debug info for /lib64/libc.so.6...10%..20%..30%..40%..50%

Thoughts?

> 
> Let's see if others have other opinions about this.  If nothing comes for by next week, you can go ahead and merge it.

All right,
thanks,
Martin

> 
> Simon


[-- Attachment #2: gdb-progress-bar.patch --]
[-- Type: text/x-patch, Size: 1723 bytes --]

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index e21b2f40ca..5a810b897d 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -43,14 +43,17 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
 #else
 #include <elfutils/debuginfod.h>
 
+#define PROGRESS_STEP_PERCENT 10
+
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname), has_printed (false)
+    : desc (desc), fname (fname), next_percent_progress (PROGRESS_STEP_PERCENT), has_printed (false)
   { }
 
   const char * const desc;
   const char * const fname;
+  long next_percent_progress;
   bool has_printed;
 };
 
@@ -74,6 +77,7 @@ progressfn (debuginfod_client *c, long cur, long total)
 
   if (check_quit_flag ())
     {
+      // TODO
       printf_filtered ("Cancelling download of %s %ps...\n",
 		       data->desc,
 		       styled_string (file_name_style.style (), data->fname));
@@ -84,9 +88,23 @@ progressfn (debuginfod_client *c, long cur, long total)
     {
       /* Print this message only once.  */
       data->has_printed = true;
-      printf_filtered ("Downloading %s %ps...\n",
+      printf_filtered ("Downloading %.2f MB %s %ps.",
+		       1.0f * total / (1024 * 1024),
 		       data->desc,
 		       styled_string (file_name_style.style (), data->fname));
+      gdb_flush (gdb_stdout);
+    }
+  else if (total != 0)
+    {
+      long percent = 100 * cur / total;
+      while (percent >= data->next_percent_progress)
+	{
+	  printf ("..%ld%%", data->next_percent_progress);
+	  data->next_percent_progress += PROGRESS_STEP_PERCENT;
+	}
+      if (percent == 100)
+	printf ("\n");
+      gdb_flush (gdb_stdout);
     }
 
   return 0;

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Simon Marchi via Gdb-patches]

On 2020-12-08 5:16 a.m., Martin Liška wrote:
> Temporary breakpoint 1 at 0x406150: file 
> /home/marxin/Programming/gcc/gcc/gcc-main.c, line 44.
> Starting program: /home/marxin/bin/gcc/bin/gcc
> Downloading 1.00 MB separate debug info for 
> /lib64/ld-linux-x86-64.so.2...10%..20%..30%..40%..50%..60%..70%..80%..90%..100% 
> 
> Downloading 4.20 MB separate debug info for 
> /lib64/libm.so.6...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
> Downloading 10.72 MB separate debug info for 
> /lib64/libc.so.6...10%..20%..30%..40%..50%
> 
> Thoughts?

Could you make it so the percentage always gets written at the same 
place, overwriting the  previous percentage written?

Simon

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

On 12/8/20 3:28 PM, Simon Marchi wrote:
> On 2020-12-08 5:16 a.m., Martin Liška wrote:
>> Temporary breakpoint 1 at 0x406150: file /home/marxin/Programming/gcc/gcc/gcc-main.c, line 44.
>> Starting program: /home/marxin/bin/gcc/bin/gcc
>> Downloading 1.00 MB separate debug info for /lib64/ld-linux-x86-64.so.2...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
>> Downloading 4.20 MB separate debug info for /lib64/libm.so.6...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
>> Downloading 10.72 MB separate debug info for /lib64/libc.so.6...10%..20%..30%..40%..50%
>>
>> Thoughts?
> 
> Could you make it so the percentage always gets written at the same place, overwriting the  previous percentage written?

Sure, implemented in the attached patch.

Martin

> 
> Simon


[-- Attachment #2: 0001-gdb-Print-percent-progress-for-debuginfod.patch --]
[-- Type: text/x-patch, Size: 2171 bytes --]

From 51f332475399ce42767fc912291fa879b91d4ddf Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 9 Dec 2020 11:48:15 +0100
Subject: [PATCH] gdb: Print percent progress for debuginfod.

Prints progress like:

Downloading 4.89 MB separate debug info for /usr/lib64/libgcrypt.so.20.. 100%
Downloading 1.10 MB separate debug info for /usr/lib64/liblzma.so.5.. 100%
Downloading 1.31 MB separate debug info for /usr/lib64/liblz4.so.1.. 100%
Downloading 0.96 MB separate debug info for /usr/lib64/libsmime3.so..  36%

ChangeLog:

	* gdb/debuginfod-support.c (struct user_data): Add
	last_percent_printed field.
	(progressfn): Print progress.
---
 gdb/debuginfod-support.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index e21b2f40ca..3d68f08fc0 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -46,12 +46,12 @@ debuginfod_debuginfo_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), last_percent_printed (-1)
   { }
 
   const char * const desc;
   const char * const fname;
-  bool has_printed;
+  long last_percent_printed;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -80,13 +80,20 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (!data->has_printed && total != 0)
+  if (total != 0)
     {
       /* Print this message only once.  */
-      data->has_printed = true;
-      printf_filtered ("Downloading %s %ps...\n",
-		       data->desc,
-		       styled_string (file_name_style.style (), data->fname));
+      long percent = 100 * cur / total;
+      if (percent != data->last_percent_printed)
+	{
+	  data->last_percent_printed = percent;
+	  printf_filtered ("\rDownloading %.2f MB %s %ps.. %3ld%%%s",
+			   1.0f * total / (1024 * 1024),
+			   data->desc,
+			   styled_string (file_name_style.style (), data->fname),
+			   percent, cur == total ? "\n" : "");
+	  gdb_flush (gdb_stdout);
+	}
     }
 
   return 0;
-- 
2.29.2

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom Tromey]

>> With the patch, one can see now:
>> Reading symbols from /usr/bin/gcc...
>> Downloading 2.69 MiB separate debug info for /usr/bin/gcc...
>> Downloading 1.00 MiB separate debug info for /lib64/ld-linux-x86-64.so.2...
>> Downloading 10.72 MiB separate debug info for /lib64/libc.so.6...
>> ...

Simon> I think that's a good idea.  Given that we don't have a progress bar
Simon> (yet), this gives clue to the user about why it might take some time.

Simon> Let's see if others have other opinions about this.  If nothing comes
Simon> for by next week, you can go ahead and merge it.

FWIW I asked the debuginfod folks on irc and they were in favor of this
as well.  So, I suggest checking it in.

Tom

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1502 bytes --]

On 12/9/20 11:51 AM, Martin Liška wrote:
> On 12/8/20 3:28 PM, Simon Marchi wrote:
>> On 2020-12-08 5:16 a.m., Martin Liška wrote:
>> Temporary breakpoint 1 at 0x406150: file
>>> /home/marxin/Programming/gcc/gcc/gcc-main.c, line 44.
>>> Starting program: /home/marxin/bin/gcc/bin/gcc
>>> Downloading 1.00 MB separate debug info for
>>> /lib64/ld-linux-x86-64.so.2...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
>>>
>>> Downloading 4.20 MB separate debug info for
>>> /lib64/libm.so.6...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
>>> Downloading 10.72 MB separate debug info for
>>> /lib64/libc.so.6...10%..20%..30%..40%..50%
>>>
>>> Thoughts?
>>
>> Could you make it so the percentage always gets written at the same
>> place, overwriting the  previous percentage written?
> 
> Sure, implemented in the attached patch.
> 

Hi,

I played around with this patch on openSUSE Tumbleweed and found two
problems:
- '\r' is not handled in fputs_maybe_filtered, so we run into
  the pagination prompt after having printed just one downloading line
- when printing a downloading line longer than chars_per_line,
  it will wrap, so the '\r' does not reset back to the actual start of
  the line.  The effect you see is that the download line is printed
  over and over again, flooding the screen.

This updated patch fixes both problems, the latter by printing the %
progress on its own line (which is cleared by a final '\r' after
reaching 100%, in order not to waste lines on this).

Thanks,
- Tom

[-- Attachment #2: 0010-gdb-Print-percent-progress-for-debuginfod.patch --]
[-- Type: text/x-patch, Size: 3051 bytes --]

gdb: Print percent progress for debuginfod

Prints progress like:

Downloading 4.89 MB separate debug info for /usr/lib64/libgcrypt.so.20.
Downloading 1.10 MB separate debug info for /usr/lib64/liblzma.so.5.
Downloading 1.31 MB separate debug info for /usr/lib64/liblz4.so.1.
Downloading 0.96 MB separate debug info for /usr/lib64/libsmime3.so.
  36%

ChangeLog:

2020-12-10  Martin Liska  <mliska@suse.cz>
	    Tom de Vries  <tdevries@suse.de>

	* gdb/debuginfod-support.c (struct user_data): Add
	last_percent_printed and first_line_printed field.
	(progressfn): Print progress.
	* utils.c (fputs_maybe_filtered): Handle '\r'.

---
 gdb/debuginfod-support.c | 36 ++++++++++++++++++++++++++++++------
 gdb/utils.c              |  6 ++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index e21b2f40ca..3d74b2d9c8 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -46,12 +46,14 @@ debuginfod_debuginfo_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), last_percent_printed (-1),
+      first_line_printed (false)
   { }
 
   const char * const desc;
   const char * const fname;
-  bool has_printed;
+  long last_percent_printed;
+  bool first_line_printed;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -80,14 +82,36 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (!data->has_printed && total != 0)
+  if (total == 0)
+    return 0;
+
+  if (!data->first_line_printed)
     {
-      /* Print this message only once.  */
-      data->has_printed = true;
-      printf_filtered ("Downloading %s %ps...\n",
+      printf_filtered ("Downloading %.2f MB %s %ps.\n",
+		       1.0f * total / (1024 * 1024),
 		       data->desc,
 		       styled_string (file_name_style.style (), data->fname));
+      gdb_flush (gdb_stdout);
+      data->first_line_printed = true;
+    }
+
+  if (cur == total)
+    {
+      printf_filtered ("\r");
+      gdb_flush (gdb_stdout);
     }
+  else if (cur > 0)
+    {
+      /* Print this message only once.  */
+      long percent = 100 * cur / total;
+      if (percent != data->last_percent_printed)
+	{
+	  data->last_percent_printed = percent;
+	  printf_filtered ("\r%3ld%%", percent);
+	  gdb_flush (gdb_stdout);
+	}
+    }
+
 
   return 0;
 }
diff --git a/gdb/utils.c b/gdb/utils.c
index 3226656e2c..26c671a44c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1769,6 +1769,12 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 		 don't increment chars_printed here.  */
 	      lineptr += skip_bytes;
 	    }
+	  else if (*lineptr == '\r')
+	    {
+	      wrap_buffer.push_back ('\r');
+	      chars_printed = 0;
+	      lineptr++;
+	    }
 	  else
 	    {
 	      wrap_buffer.push_back (*lineptr);

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Martin Liška]

On 12/10/20 12:26 AM, Tom de Vries wrote:
> On 12/9/20 11:51 AM, Martin Liška wrote:
>> On 12/8/20 3:28 PM, Simon Marchi wrote:
>>> On 2020-12-08 5:16 a.m., Martin Liška wrote:
>>> Temporary breakpoint 1 at 0x406150: file
>>>> /home/marxin/Programming/gcc/gcc/gcc-main.c, line 44.
>>>> Starting program: /home/marxin/bin/gcc/bin/gcc
>>>> Downloading 1.00 MB separate debug info for
>>>> /lib64/ld-linux-x86-64.so.2...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
>>>>
>>>> Downloading 4.20 MB separate debug info for
>>>> /lib64/libm.so.6...10%..20%..30%..40%..50%..60%..70%..80%..90%..100%
>>>> Downloading 10.72 MB separate debug info for
>>>> /lib64/libc.so.6...10%..20%..30%..40%..50%
>>>>
>>>> Thoughts?
>>>
>>> Could you make it so the percentage always gets written at the same
>>> place, overwriting the  previous percentage written?
>>
>> Sure, implemented in the attached patch.
>>
> 
> Hi,
> 
> I played around with this patch on openSUSE Tumbleweed and found two
> problems:
> - '\r' is not handled in fputs_maybe_filtered, so we run into
>    the pagination prompt after having printed just one downloading line
> - when printing a downloading line longer than chars_per_line,
>    it will wrap, so the '\r' does not reset back to the actual start of
>    the line.  The effect you see is that the download line is printed
>    over and over again, flooding the screen.
> 
> This updated patch fixes both problems, the latter by printing the %
> progress on its own line (which is cleared by a final '\r' after
> reaching 100%, in order not to waste lines on this).

Hi Tom.

Thank you for the improvement, I like it.

Martin

> 
> Thanks,
> - Tom
> 

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom Tromey]

Tom> This updated patch fixes both problems, the latter by printing the %
Tom> progress on its own line (which is cleared by a final '\r' after
Tom> reaching 100%, in order not to waste lines on this).

A long time ago I had patches to show a progress bar when reading DWARF.

https://sourceware.org/pipermail/gdb-patches/2017-April/139882.html

Maybe this could be resurrected.
I've occasionally thought of adding this to the TUI as well, say briefly
overwriting the status bar with a progress meter.

Tom

[gdb/cli] Add a progress meter

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

[ Re: [PATCH] gdb: print size of downloaded debuginfod binary ]

On 12/10/20 8:21 PM, Tom Tromey wrote:
> Tom> This updated patch fixes both problems, the latter by printing the %
> Tom> progress on its own line (which is cleared by a final '\r' after
> Tom> reaching 100%, in order not to waste lines on this).
> 
> A long time ago I had patches to show a progress bar when reading DWARF.
> 
> https://sourceware.org/pipermail/gdb-patches/2017-April/139882.html
> 
> Maybe this could be resurrected.
> I've occasionally thought of adding this to the TUI as well, say briefly
> overwriting the status bar with a progress meter.

I've taken a look, and think that this code is more in line with gdb cli
setup, so I've reimplemented on top of this.

Here is the first patch, adding the progress meter infrastructure
factored out from your patch.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-cli-Add-a-progress-meter.patch --]
[-- Type: text/x-patch, Size: 7779 bytes --]

[gdb/cli] Add a progress meter

This patch adds a progress bar.  It's not used anywhere yet.

gdb/ChangeLog:

2020-12-11  Tom Tromey  <tom@tromey.com>
	    Tom de Vries  <tdevries@suse.de>

	* utils.h (get_chars_per_line): Declare.
	* utils.c (get_chars_per_line): New function.
	(fputs_maybe_filtered): Handle '\r'.
	* ui-out.h (ui_out::progress_meter): New class.
	(ui_out::progress, ui_out::do_progress_start)
	(ui_out::do_progress_notify, ui_out::do_progress_end): New
	methods.
	* ui-out.c (do_progress_end)
	(make_cleanup_ui_out_progress_begin_end, ui_out_progress): New
	functions.
	* mi/mi-out.h (mi_ui_out::do_progress_start)
	(mi_ui_out::do_progress_notify, mi_ui_out::do_progress_end): New
	methods.
	* cli-out.h (struct cli_ui_out) <do_progress_start,
	do_progress_notify, do_progress_end>: New methods.
	<enum meter_stat, struct cli_progress_info>: New.
	<m_meters>: New member.
	* cli-out.c (cli_ui_out::do_progress_start)
	(cli_ui_out::do_progress_notify, cli_ui_out::do_progress_end): New
	methods.

---
 gdb/cli-out.c   | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/cli-out.h   | 29 ++++++++++++++++++++
 gdb/mi/mi-out.h | 12 ++++++++
 gdb/ui-out.h    | 36 ++++++++++++++++++++++++
 gdb/utils.c     |  8 ++++++
 gdb/utils.h     |  4 +++
 6 files changed, 174 insertions(+)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index e47272ad87..5d07426b73 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -265,6 +265,91 @@ cli_ui_out::do_redirect (ui_file *outstream)
     m_streams.pop_back ();
 }
 
+void
+cli_ui_out::do_progress_start (const std::string &name, int should_print)
+{
+  struct ui_file *stream = m_streams.back ();
+  cli_progress_info meter;
+
+  meter.last_value = 0;
+  meter.name = name;
+  if (!stream->isatty ())
+    {
+      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
+      gdb_flush (stream);
+      meter.printing = WORKING;
+    }
+  else
+    {
+      /* Don't actually emit anything until the first call notify 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;
+    }
+
+  m_meters.push_back (meter);
+}
+
+void
+cli_ui_out::do_progress_notify (double howmuch)
+{
+  struct ui_file *stream = m_streams.back ();
+  cli_progress_info &meter (m_meters.back ());
+
+  if (meter.printing == NO_PRINT)
+    return;
+
+  if (meter.printing == START)
+    {
+      fprintf_unfiltered (stream, "%s\n", meter.name.c_str ());
+      gdb_flush (stream);
+      meter.printing = WORKING;
+    }
+
+  if (stream->isatty ())
+    {
+      int i, max;
+      int width = get_chars_per_line () - 3;
+
+      max = width * howmuch;
+      fprintf_unfiltered (stream, "\r[");
+      for (i = 0; i < width; ++i)
+	fprintf_unfiltered (stream, i < max ? "#" : " ");
+      fprintf_unfiltered (stream, "]");
+      gdb_flush (stream);
+    }
+}
+
+void
+cli_ui_out::do_progress_end ()
+{
+  struct ui_file *stream = m_streams.back ();
+  cli_progress_info &meter = m_meters.back ();
+
+  if (meter.printing != NO_PRINT)
+    {
+      if (stream->isatty ())
+	{
+	  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");
+	  gdb_flush (stream);
+	}
+      else
+	{
+	  fprintf_unfiltered (stream, "done.\n");
+	  gdb_flush (stream);
+	}
+    }
+
+  m_meters.pop_back ();
+}
+
 /* local functions */
 
 void
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 84e957ca89..0c45b77d07 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -71,6 +71,10 @@ 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 &, int) override;
+  virtual void do_progress_notify (double) override;
+  virtual void do_progress_end () override;
+
   bool suppress_output ()
   { return m_suppress_output; }
 
@@ -80,6 +84,31 @@ 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,
+    /* Printing should not be done.  */
+    NO_PRINT
+  };
+
+  /* The state of a recent progress meter.  */
+  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;
+  };
+
+  /* Stack of progress meters.  */
+  std::vector<cli_progress_info> m_meters;
 };
 
 extern cli_ui_out *cli_out_new (struct ui_file *stream);
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index de4b3e01a4..4be0decd14 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -83,6 +83,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 &, int) override
+  {
+  }
+
+  virtual void do_progress_notify (double) override
+  {
+  }
+
+  virtual void do_progress_end () override
+  {
+  }
+
 private:
 
   void field_separator ();
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9fc60614d6..9ae4eeb83a 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -275,6 +275,38 @@ class ui_out
      escapes.  */
   virtual bool can_emit_style_escape () const = 0;
 
+  /* An object that starts and finishes a progress meter.  */
+  class progress_meter
+  {
+  public:
+    progress_meter (struct ui_out *uiout, const std::string &name,
+		    int should_print)
+      : m_uiout (uiout)
+    {
+      m_uiout->do_progress_start (name, should_print);
+    }
+
+    ~progress_meter ()
+    {
+      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;
+
+  private:
+
+    struct ui_out *m_uiout;
+  };
+
+  /* Emit some progress corresponding to the most recently created
+     progress meter.  HOWMUCH may range from 0.0 to 1.0.  */
+  void progress (double howmuch)
+  {
+    do_progress_notify (howmuch);
+  }
+
  protected:
 
   virtual void do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
@@ -309,6 +341,10 @@ 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 &, int) = 0;
+  virtual void do_progress_notify (double) = 0;
+  virtual void do_progress_end () = 0;
+
   /* Set as not MI-like by default.  It is overridden in subclasses if
      necessary.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 3226656e2c..b1d1694584 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1579,6 +1579,14 @@ gdb_flush (struct ui_file *stream)
   stream->flush ();
 }
 
+/* See utils.h.  */
+
+int
+get_chars_per_line (void)
+{
+  return chars_per_line;
+}
+
 /* Indicate that if the next sequence of characters overflows the line,
    a newline should be inserted here rather than when it hits the end.
    If INDENT is non-null, it is a string to be printed to indent the
diff --git a/gdb/utils.h b/gdb/utils.h
index a8c65ed817..5ae97b7469 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -364,6 +364,10 @@ extern void wrap_here (const char *);
 
 extern void reinitialize_more_filter (void);
 
+/* Return the number of characters in a line.  */
+
+extern int get_chars_per_line (void);
+
 extern bool pagination_enabled;
 
 extern struct ui_file **current_ui_gdb_stdout_ptr (void);

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On 12/11/20 6:51 PM, Tom de Vries wrote:
> On 12/10/20 8:21 PM, Tom Tromey wrote:
>> Tom> This updated patch fixes both problems, the latter by printing the %
>> Tom> progress on its own line (which is cleared by a final '\r' after
>> Tom> reaching 100%, in order not to waste lines on this).
>>
>> A long time ago I had patches to show a progress bar when reading DWARF.
>>
>> https://sourceware.org/pipermail/gdb-patches/2017-April/139882.html
>>
>> Maybe this could be resurrected.
>> I've occasionally thought of adding this to the TUI as well, say briefly
>> overwriting the status bar with a progress meter.
> 
> I've taken a look, and think that this code is more in line with gdb cli
> setup, so I've reimplemented on top of this.
> 
> Here is the first patch, adding the progress meter infrastructure
> factored out from your patch.

And here's the reimplemented patch.

I've test-driven it a bit on tumbleweed, and behaviour looks ok.

Currently testing on x86_64-linux.

Thanks,
- Tom

[-- Attachment #2: 0002-gdb-Print-progress-for-debuginfod.patch --]
[-- Type: text/x-patch, Size: 2386 bytes --]

[gdb] Print progress for debuginfod

Prints progress like:

Downloading 4.89 MB separate debug info for /usr/lib64/libgcrypt.so.20.
Downloading 1.10 MB separate debug info for /usr/lib64/liblzma.so.5.
Downloading 1.31 MB separate debug info for /usr/lib64/liblz4.so.1.
Downloading 0.96 MB separate debug info for /usr/lib64/libsmime3.so.
[###                                                                    ]

Tested on x86_64-linux.

ChangeLog:

2020-12-10  Martin Liska  <mliska@suse.cz>
	    Tom de Vries  <tdevries@suse.de>

	* gdb/debuginfod-support.c (struct user_data): Remove has_printed
	field.  Add meter field.
	(progressfn): Print progress using meter.

---
 gdb/debuginfod-support.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 86630576e3..a17cb7c5a8 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -21,6 +21,8 @@
 #include "cli/cli-style.h"
 #include "gdbsupport/scoped_fd.h"
 #include "debuginfod-support.h"
+#include "gdbsupport/gdb_optional.h"
+#include <iomanip>
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -48,12 +50,12 @@ debuginfod_debuginfo_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;
+  gdb::optional<ui_out::progress_meter> meter;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -82,15 +84,21 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (!data->has_printed && total != 0)
+  if (total == 0)
+    return 0;
+
+  if (!data->meter.has_value ())
     {
-      /* Print this message only once.  */
-      data->has_printed = true;
-      printf_filtered ("Downloading %s %ps...\n",
-		       data->desc,
-		       styled_string (file_name_style.style (), data->fname));
+      float size_in_mb = 1.0f * total / (1024 * 1024);
+      std::stringstream message;
+      message.precision (2);
+      message << "Downloading " << size_in_mb << " MB " << data->desc
+	      << " " << data->fname;
+      data->meter.emplace (current_uiout, message.str (), 1);
     }
 
+  current_uiout->progress ((double)cur / (double)total);
+
   return 0;
 }
 

Re: [gdb/cli] Add a progress meter

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> I've taken a look, and think that this code is more in line with gdb cli
Tom> setup, so I've reimplemented on top of this.

Tom> Here is the first patch, adding the progress meter infrastructure
Tom> factored out from your patch.

Not sure if I ought to review a patch that I largely wrote, but I did
find some things I think should be done differently nowadays.

Tom> +void
Tom> +cli_ui_out::do_progress_start (const std::string &name, int should_print)

should_print should probably be a bool, throughout this patch.

Tom> +      /* Don't actually emit anything until the first call notify us

I think s/notify/notifies/

Tom> +  if (!stream->isatty ())
Tom> +    {
Tom> +      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());

This is probably fine but I suppose the meaning of "should_print" ought
to be described somewhere, since otherwise this could seem like a bug --
it is printing something even when should_print==false.

Tom> +  m_meters.push_back (meter);

std::move(meter) would be more efficient here, since it would avoid
another string copy.

Tom> +    progress_meter (struct ui_out *uiout, const std::string &name,
Tom> +		    int should_print)
Tom> +      : m_uiout (uiout)

Here's probably where should_print should be documented.

Tom> +get_chars_per_line (void)

Just "()", not "(void)", now.

Tom> +++ b/gdb/utils.h
Tom> @@ -364,6 +364,10 @@ extern void wrap_here (const char *);
 
Tom>  extern void reinitialize_more_filter (void);
 
Tom> +/* Return the number of characters in a line.  */
Tom> +
Tom> +extern int get_chars_per_line (void);

Here too.

Tom

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +  gdb::optional<ui_out::progress_meter> meter;

Seems like a bit of a shame to have an optional here, but then
essentially require optional-style behavior in the CLI ui-out as well
(in deferring output until the first update).

Maybe it could be done in progress_meter itself.  Not sure.

Tom> -      /* Print this message only once.  */
Tom> -      data->has_printed = true;
Tom> -      printf_filtered ("Downloading %s %ps...\n",
Tom> -		       data->desc,
Tom> -		       styled_string (file_name_style.style (), data->fname));
Tom> +      float size_in_mb = 1.0f * total / (1024 * 1024);
Tom> +      std::stringstream message;
Tom> +      message.precision (2);
Tom> +      message << "Downloading " << size_in_mb << " MB " << data->desc
Tom> +	      << " " << data->fname;

Seems like this could use string_printf.
I suspect there's some way to preserve the styling but offhand I don't
recall what it is.  Maybe you have to use string_file to accomplish that.

Tom

Re: [gdb/cli] Add a progress meter

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 2172 bytes --]

On 12/11/20 8:21 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> I've taken a look, and think that this code is more in line with gdb cli
> Tom> setup, so I've reimplemented on top of this.
> 
> Tom> Here is the first patch, adding the progress meter infrastructure
> Tom> factored out from your patch.
> 
> Not sure if I ought to review a patch that I largely wrote, but I did
> find some things I think should be done differently nowadays.
> 
> Tom> +void
> Tom> +cli_ui_out::do_progress_start (const std::string &name, int should_print)
> 
> should_print should probably be a bool, throughout this patch.
> 

Done.

> Tom> +      /* Don't actually emit anything until the first call notify us
> 
> I think s/notify/notifies/
> 

Done.

> Tom> +  if (!stream->isatty ())
> Tom> +    {
> Tom> +      fprintf_unfiltered (stream, "%s...", meter.name.c_str ());
> 
> This is probably fine but I suppose the meaning of "should_print" ought
> to be described somewhere,

Done.

> since otherwise this could seem like a bug --
> it is printing something even when should_print==false.
> 
> Tom> +  m_meters.push_back (meter);
> 
> std::move(meter) would be more efficient here, since it would avoid
> another string copy.
> 

Done.

> Tom> +    progress_meter (struct ui_out *uiout, const std::string &name,
> Tom> +		    int should_print)
> Tom> +      : m_uiout (uiout)
> 
> Here's probably where should_print should be documented.
> 
> Tom> +get_chars_per_line (void)
> 
> Just "()", not "(void)", now.
> 
> Tom> +++ b/gdb/utils.h
> Tom> @@ -364,6 +364,10 @@ extern void wrap_here (const char *);
>  
> Tom>  extern void reinitialize_more_filter (void);
>  
> Tom> +/* Return the number of characters in a line.  */
> Tom> +
> Tom> +extern int get_chars_per_line (void);
> 
> Here too.

Done.

I've also fixed the first-progress-call-has-howmuch=1.0 problem
mentioned in the original review conversation with Pedro, by adding a
new enum meter_state element PROGRESS.

I've also fixed the problem that when f.i. run with -batch,
chars_per_line is zero. The previous version of the patch was printing
"[]" in that case.

Thanks,
- Tom


[-- Attachment #2: 0010-gdb-cli-Add-a-progress-meter.patch --]
[-- Type: text/x-patch, Size: 9100 bytes --]

[gdb/cli] Add a progress meter

Add a progress meter.  It's not used anywhere yet.

gdb/ChangeLog:

2020-12-11  Tom Tromey  <tom@tromey.com>
	    Tom de Vries  <tdevries@suse.de>

	* utils.h (get_chars_per_line): Declare.
	* utils.c (get_chars_per_line): New function.
	(fputs_maybe_filtered): Handle '\r'.
	* ui-out.h (ui_out::progress_meter): New class.
	(ui_out::progress, ui_out::do_progress_start)
	(ui_out::do_progress_notify, ui_out::do_progress_end): New
	methods.
	* ui-out.c (do_progress_end)
	(make_cleanup_ui_out_progress_begin_end, ui_out_progress): New
	functions.
	* mi/mi-out.h (mi_ui_out::do_progress_start)
	(mi_ui_out::do_progress_notify, mi_ui_out::do_progress_end): New
	methods.
	* cli-out.h (struct cli_ui_out) <do_progress_start,
	do_progress_notify, do_progress_end>: New methods.
	<enum meter_stat, struct cli_progress_info>: New.
	<m_meters>: New member.
	* cli-out.c (cli_ui_out::do_progress_start)
	(cli_ui_out::do_progress_notify, cli_ui_out::do_progress_end): New
	methods.

---
 gdb/cli-out.c   | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 gdb/cli-out.h   |  31 +++++++++++++++++
 gdb/mi/mi-out.h |  12 +++++++
 gdb/ui-out.h    |  37 +++++++++++++++++++++
 gdb/utils.c     |  14 ++++++++
 gdb/utils.h     |   4 +++
 6 files changed, 199 insertions(+)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index e47272ad87..7722ecc4fe 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -265,6 +265,107 @@ 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:
+     <>
+   - printed for not-a-tty:
+     <NAME...done.
+     >
+*/
+
+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;
+
+  meter.last_value = 0;
+  meter.name = name;
+  if (!stream->isatty ())
+    {
+      fprintf_unfiltered (stream, "%s...", meter.name.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;
+    }
+
+  m_meters.push_back (std::move (meter));
+}
+
+void
+cli_ui_out::do_progress_notify (double howmuch)
+{
+  struct ui_file *stream = m_streams.back ();
+  cli_progress_info &meter (m_meters.back ());
+
+  if (meter.printing == NO_PRINT)
+    return;
+
+  if (meter.printing == START)
+    {
+      fprintf_unfiltered (stream, "%s\n", meter.name.c_str ());
+      gdb_flush (stream);
+      meter.printing = WORKING;
+    }
+
+  if (meter.printing == WORKING && howmuch >= 1.0)
+    return;
+
+  if (!stream->isatty ())
+    return;
+
+  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;
+      fprintf_unfiltered (stream, "\r[");
+      for (i = 0; i < width; ++i)
+	fprintf_unfiltered (stream, i < max ? "#" : " ");
+      fprintf_unfiltered (stream, "]");
+      gdb_flush (stream);
+      meter.printing = PROGRESS;
+    }
+}
+
+void
+cli_ui_out::do_progress_end ()
+{
+  struct ui_file *stream = m_streams.back ();
+  cli_progress_info &meter = m_meters.back ();
+
+  if (!stream->isatty ())
+    {
+      fprintf_unfiltered (stream, "done.\n");
+      gdb_flush (stream);
+    }
+  else if (meter.printing == PROGRESS)
+    {
+      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");
+      gdb_flush (stream);
+    }
+
+  m_meters.pop_back ();
+}
+
 /* local functions */
 
 void
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 84e957ca89..5f55554fdb 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -71,6 +71,10 @@ 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_end () override;
+
   bool suppress_output ()
   { return m_suppress_output; }
 
@@ -80,6 +84,33 @@ 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.  */
+  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;
+  };
+
+  /* Stack of progress meters.  */
+  std::vector<cli_progress_info> m_meters;
 };
 
 extern cli_ui_out *cli_out_new (struct ui_file *stream);
diff --git a/gdb/mi/mi-out.h b/gdb/mi/mi-out.h
index de4b3e01a4..da8f4e6457 100644
--- a/gdb/mi/mi-out.h
+++ b/gdb/mi/mi-out.h
@@ -83,6 +83,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_notify (double) override
+  {
+  }
+
+  virtual void do_progress_end () override
+  {
+  }
+
 private:
 
   void field_separator ();
diff --git a/gdb/ui-out.h b/gdb/ui-out.h
index 9fc60614d6..dfd9679e4c 100644
--- a/gdb/ui-out.h
+++ b/gdb/ui-out.h
@@ -275,6 +275,39 @@ class ui_out
      escapes.  */
   virtual bool can_emit_style_escape () const = 0;
 
+  /* An object that starts and finishes a progress meter.  */
+  class progress_meter
+  {
+  public:
+    /* 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)
+    {
+      m_uiout->do_progress_start (name, should_print);
+    }
+
+    ~progress_meter ()
+    {
+      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;
+
+  private:
+
+    struct ui_out *m_uiout;
+  };
+
+  /* Emit some progress corresponding to the most recently created
+     progress meter.  HOWMUCH may range from 0.0 to 1.0.  */
+  void progress (double howmuch)
+  {
+    do_progress_notify (howmuch);
+  }
+
  protected:
 
   virtual void do_table_begin (int nbrofcols, int nr_rows, const char *tblid)
@@ -309,6 +342,10 @@ 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;
+
   /* Set as not MI-like by default.  It is overridden in subclasses if
      necessary.  */
 
diff --git a/gdb/utils.c b/gdb/utils.c
index 3226656e2c..abcf6e256b 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1579,6 +1579,14 @@ gdb_flush (struct ui_file *stream)
   stream->flush ();
 }
 
+/* See utils.h.  */
+
+int
+get_chars_per_line ()
+{
+  return chars_per_line;
+}
+
 /* Indicate that if the next sequence of characters overflows the line,
    a newline should be inserted here rather than when it hits the end.
    If INDENT is non-null, it is a string to be printed to indent the
@@ -1769,6 +1777,12 @@ fputs_maybe_filtered (const char *linebuffer, struct ui_file *stream,
 		 don't increment chars_printed here.  */
 	      lineptr += skip_bytes;
 	    }
+	  else if (*lineptr == '\r')
+	    {
+	      wrap_buffer.push_back (*lineptr);
+	      chars_printed = 0;
+	      lineptr++;
+	    }
 	  else
 	    {
 	      wrap_buffer.push_back (*lineptr);
diff --git a/gdb/utils.h b/gdb/utils.h
index a8c65ed817..e87ce11323 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -364,6 +364,10 @@ extern void wrap_here (const char *);
 
 extern void reinitialize_more_filter (void);
 
+/* Return the number of characters in a line.  */
+
+extern int get_chars_per_line ();
+
 extern bool pagination_enabled;
 
 extern struct ui_file **current_ui_gdb_stdout_ptr (void);

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

On 12/11/20 8:23 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> +  gdb::optional<ui_out::progress_meter> meter;
> 
> Seems like a bit of a shame to have an optional here, but then
> essentially require optional-style behavior in the CLI ui-out as well
> (in deferring output until the first update).
> 

Well, the natural place to declare "ui_out::progress_meter meter" given
its lifetime is in debuginfod_source_query and debuginfod_debuginfo_query.

But the only constructor has the name argument with no means of changing
it later, and the name (containing the size) is only known lateron, when
progressfn is called.

So the optional is used as a means to have lifetime as we want it, while
postponing construction until we known all the constructor arguments.

> Maybe it could be done in progress_meter itself.  Not sure.
> 
> Tom> -      /* Print this message only once.  */
> Tom> -      data->has_printed = true;
> Tom> -      printf_filtered ("Downloading %s %ps...\n",
> Tom> -		       data->desc,
> Tom> -		       styled_string (file_name_style.style (), data->fname));
> Tom> +      float size_in_mb = 1.0f * total / (1024 * 1024);
> Tom> +      std::stringstream message;
> Tom> +      message.precision (2);
> Tom> +      message << "Downloading " << size_in_mb << " MB " << data->desc
> Tom> +	      << " " << data->fname;
> 
> Seems like this could use string_printf.

Done.

> I suspect there's some way to preserve the styling but offhand I don't
> recall what it is.  Maybe you have to use string_file to accomplish that.
> 

Done.

Thanks,
- Tom

[-- Attachment #2: 0011-gdb-Print-progress-for-debuginfod.patch --]
[-- Type: text/x-patch, Size: 2602 bytes --]

[gdb] Print progress for debuginfod

Prints progress like:

Downloading 4.89 MB separate debug info for /usr/lib64/libgcrypt.so.20.
Downloading 1.10 MB separate debug info for /usr/lib64/liblzma.so.5.
Downloading 1.31 MB separate debug info for /usr/lib64/liblz4.so.1.
Downloading 0.96 MB separate debug info for /usr/lib64/libsmime3.so.
[###                                                                    ]

Tested on x86_64-linux.

ChangeLog:

2020-12-10  Martin Liska  <mliska@suse.cz>
	    Tom de Vries  <tdevries@suse.de>

	* gdb/debuginfod-support.c (struct user_data): Remove has_printed
	field.  Add meter field.
	(progressfn): Print progress using meter.

---
 gdb/debuginfod-support.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index e21b2f40ca..e9b2dcd5be 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -21,6 +21,7 @@
 #include "cli/cli-style.h"
 #include "gdbsupport/scoped_fd.h"
 #include "debuginfod-support.h"
+#include "gdbsupport/gdb_optional.h"
 
 #ifndef HAVE_LIBDEBUGINFOD
 scoped_fd
@@ -46,12 +47,12 @@ debuginfod_debuginfo_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;
+  gdb::optional<ui_out::progress_meter> meter;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -80,15 +81,25 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (!data->has_printed && total != 0)
+  if (total == 0)
+    return 0;
+
+  if (!data->meter.has_value ())
     {
-      /* Print this message only once.  */
-      data->has_printed = true;
-      printf_filtered ("Downloading %s %ps...\n",
-		       data->desc,
-		       styled_string (file_name_style.style (), data->fname));
+      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);
     }
 
+  current_uiout->progress ((double)cur / (double)total);
+
   return 0;
 }
 

Re: [gdb/cli] Add a progress meter

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> +   - printed for not-a-tty:
Tom> +     <NAME...done.

Tom> +  if (!stream->isatty ())
Tom> +    {
Tom> +      fprintf_unfiltered (stream, "done.\n");
Tom> +      gdb_flush (stream);
Tom> +    }

We got rid of the "done" for symbol reading, so IMO we might as well
drop it here too.

Otherwise this looks good to me.

Tom

Re: [PATCH] gdb: print size of downloaded debuginfod binary

[Tom Tromey]

>> Seems like a bit of a shame to have an optional here, but then
>> essentially require optional-style behavior in the CLI ui-out as well
>> (in deferring output until the first update).

Tom> Well, the natural place to declare "ui_out::progress_meter meter" given
Tom> its lifetime is in debuginfod_source_query and debuginfod_debuginfo_query.

Tom> But the only constructor has the name argument with no means of changing
Tom> it later, and the name (containing the size) is only known lateron, when
Tom> progressfn is called.

Makes sense to me.

This looks good.  Thanks for doing this.

Tom