[PATCH] Remove download size from debuginfod progress messages

[PATCH] Remove download size from debuginfod progress messages

[Aaron Merey]

Currently debuginfod progress update messages include the size of
each download:

  Downloading 7.5 MB separate debug info /lib/libxyz.so.0

This value originates from the Content-Length HTTP header of the
transfer.  However this header is not guaranteed to be present for
each download.  This can happen when debuginfod servers compress files
on-the-fly at the time of transfer.  In this case gdb wrongly prints
"-0.00 MB" as the size.

This patch removes download sizes from progress messages for now until
a more thorough reworking of progress updating is implemented [1].

[1] https://sourceware.org/pipermail/gdb-patches/2022-February/185798.html
---
 gdb/debuginfod-support.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 3614ff23882..ab40d117c9e 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -86,12 +86,12 @@ debuginfod_exec_query (const unsigned char *build_id,
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname)
+    : desc (desc), fname (fname), has_printed (false)
   { }
 
   const char * const desc;
   const char * const fname;
-  gdb::optional<ui_out::progress_meter> meter;
+  bool has_printed;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -121,25 +121,13 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (total == 0)
-    return 0;
-
-  if (!data->meter.has_value ())
+  if (!data->has_printed)
     {
-      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);
+      data->has_printed = true;
+      printf_filtered ("Downloading %s %ps...\n", data->desc,
+		       styled_string (file_name_style.style (), data->fname));
     }
 
-  data->meter->progress ((double)cur / (double)total);
-
   return 0;
 }
 
-- 
2.35.1

Re: [PATCH] Remove download size from debuginfod progress messages

[Keith Seitz]

On 3/22/22 17:34, Aaron Merey wrote:
> Currently debuginfod progress update messages include the size of
> each download:
> 
>    Downloading 7.5 MB separate debug info /lib/libxyz.so.0
> 
> This value originates from the Content-Length HTTP header of the
> transfer.  However this header is not guaranteed to be present for
> each download.  This can happen when debuginfod servers compress files
> on-the-fly at the time of transfer.  In this case gdb wrongly prints
> "-0.00 MB" as the size.

I realize this is an interim patch to address an issue currently
under development, and I'm okay with that. There are a lot of
people that use devel versions of gdb to do work. [At least I
hope I'm not alone!]

However, I would really rather not simply remove all download
sizes. If we can detect the condition that would print the unhelpful
"-0.00 MB", then *just* removing the download size for that case (or
printing "unknown" or something) would be my preferred path.

YMMV, and I am not a maintainer, etc.

Keith

Re: [PATCH] Remove download size from debuginfod progress messages

[Aaron Merey]

Hi Keith,

On Wed, Mar 23, 2022 at 12:02 PM Keith Seitz <keiths@redhat.com> wrote:
> On 3/22/22 17:34, Aaron Merey wrote:
> > Currently debuginfod progress update messages include the size of
> > each download:
> >
> >    Downloading 7.5 MB separate debug info /lib/libxyz.so.0
> >
> > This value originates from the Content-Length HTTP header of the 
> > transfer.  However this header is not guaranteed to be present for 
> > each download.  This can happen when debuginfod servers compress files
> > on-the-fly at the time of transfer.  In this case gdb wrongly prints
> > "-0.00 MB" as the size.
>
> I realize this is an interim patch to address an issue currently
> under development, and I'm okay with that. There are a lot of
> people that use devel versions of gdb to do work. [At least I
> hope I'm not alone!]
>
> However, I would really rather not simply remove all download
> sizes. If we can detect the condition that would print the unhelpful
> "-0.00 MB", then *just* removing the download size for that case (or 
> printing "unknown" or something) would be my preferred path.

I wanted to keep this patch as simple as possible in order to avoid
getting bogged down by details that might be more appropriate for
the patch in-progress that reworks these messages.  But I agree that
we should continue to print sizes if they are available.  I've added
this to the patch below.

Aaron

---
Remove download size from debuginfod progress messages if unavailable

Currently debuginfod progress update messages include the size of
each download:

  Downloading 7.5 MB separate debug info /lib/libxyz.so.0

This value originates from the Content-Length HTTP header of the
transfer.  However this header is not guaranteed to be present for
each download.  This can happen when debuginfod servers compress files
on-the-fly at the time of transfer.  In this case gdb wrongly prints
"-0.00 MB" as the size.

This patch removes download sizes from progress messages when they are
not available.  It also removes usage of the progress bar until
a more thorough reworking of progress updating is implemented. [1]

[1] https://sourceware.org/pipermail/gdb-patches/2022-February/185798.html

---
 gdb/debuginfod-support.c | 44 ++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 3614ff23882..d5d023ff7bb 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -86,12 +86,12 @@ debuginfod_exec_query (const unsigned char *build_id,
 struct user_data
 {
   user_data (const char *desc, const char *fname)
-    : desc (desc), fname (fname)
+    : desc (desc), fname (fname), has_printed (false)
   { }
 
   const char * const desc;
   const char * const fname;
-  gdb::optional<ui_out::progress_meter> meter;
+  bool has_printed;
 };
 
 /* Deleter for a debuginfod_client.  */
@@ -121,24 +121,32 @@ progressfn (debuginfod_client *c, long cur, long total)
       return 1;
     }
 
-  if (total == 0)
-    return 0;
-
-  if (!data->meter.has_value ())
+  if (!data->has_printed)
     {
-      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);
-    }
+      /* Include the transfer size, if available.  */
+      if (total > 0)
+	{
+	  float size = 1.0f * total / 1024;
+	  const char *unit = "KB";
+
+	  /* If size is greater than 0.01 MB, set unit to MB.  */
+	  if (size > 10.24)
+	    {
+	      size /= 1024;
+	      unit = "MB";
+	    }
+
+	  printf_filtered ("Downloading %.2f %s %s %ps...\n",
+			   size, unit, data->desc,
+			   styled_string (file_name_style.style (),
+					  data->fname));
+	}
+      else
+	printf_filtered ("Downloading %s %ps...\n", data->desc,
+			 styled_string (file_name_style.style (), data->fname));
 
-  data->meter->progress ((double)cur / (double)total);
+      data->has_printed = true;
+    }
 
   return 0;
 }
-- 
2.35.1

Re: [PATCH] Remove download size from debuginfod progress messages

[Keith Seitz]

On 3/23/22 11:06, Aaron Merey wrote:

> On Wed, Mar 23, 2022 at 12:02 PM Keith Seitz <keiths@redhat.com> wrote:
>> On 3/22/22 17:34, Aaron Merey wrote:
>>
>> However, I would really rather not simply remove all download
>> sizes. If we can detect the condition that would print the unhelpful
>> "-0.00 MB", then *just* removing the download size for that case (or
>> printing "unknown" or something) would be my preferred path.
> 
> I wanted to keep this patch as simple as possible in order to avoid
> getting bogged down by details that might be more appropriate for
> the patch in-progress that reworks these messages.  But I agree that
> we should continue to print sizes if they are available.  I've added
> this to the patch below.

I appreciate your patience. I think this is fine, but you will need
to wait for a global maintainer to approve.

Thanks!

Keith

Re: [PATCH] Remove download size from debuginfod progress messages

[Simon Marchi]

On 2022-03-23 14:06, Aaron Merey via Gdb-patches wrote:
> Hi Keith,
>
> On Wed, Mar 23, 2022 at 12:02 PM Keith Seitz <keiths@redhat.com> wrote:
>> On 3/22/22 17:34, Aaron Merey wrote:
>>> Currently debuginfod progress update messages include the size of
>>> each download:
>>>
>>>    Downloading 7.5 MB separate debug info /lib/libxyz.so.0
>>>
>>> This value originates from the Content-Length HTTP header of the
>>> transfer.  However this header is not guaranteed to be present for
>>> each download.  This can happen when debuginfod servers compress files
>>> on-the-fly at the time of transfer.  In this case gdb wrongly prints
>>> "-0.00 MB" as the size.
>>
>> I realize this is an interim patch to address an issue currently
>> under development, and I'm okay with that. There are a lot of
>> people that use devel versions of gdb to do work. [At least I
>> hope I'm not alone!]
>>
>> However, I would really rather not simply remove all download
>> sizes. If we can detect the condition that would print the unhelpful
>> "-0.00 MB", then *just* removing the download size for that case (or
>> printing "unknown" or something) would be my preferred path.
>
> I wanted to keep this patch as simple as possible in order to avoid
> getting bogged down by details that might be more appropriate for
> the patch in-progress that reworks these messages.  But I agree that
> we should continue to print sizes if they are available.  I've added
> this to the patch below.
>
> Aaron
>
> ---
> Remove download size from debuginfod progress messages if unavailable
>
> Currently debuginfod progress update messages include the size of
> each download:
>
>   Downloading 7.5 MB separate debug info /lib/libxyz.so.0
>
> This value originates from the Content-Length HTTP header of the
> transfer.  However this header is not guaranteed to be present for
> each download.  This can happen when debuginfod servers compress files
> on-the-fly at the time of transfer.  In this case gdb wrongly prints
> "-0.00 MB" as the size.
>
> This patch removes download sizes from progress messages when they are
> not available.  It also removes usage of the progress bar until
> a more thorough reworking of progress updating is implemented. [1]
>
> [1] https://sourceware.org/pipermail/gdb-patches/2022-February/185798.html

I'm fine with an interim patch, but I'd really like to see the progress
bar back, I enjoy it :)!

I haven't been involved in those dicussions so I don't know what the
plan is.  But typically progress bars have an "undefined total" mode
where you can still tell that the operation is progressing (bytes are
being received), you just don't what percentage of the total.

But really, is it a problem for GDB master to print "~0.00 MB", until
the "real" fix is merged?  It's not like it's causing a crash or
anything like that?  Or is it something you want to push to the GDB 12
branch?

The patch LGTM in any case, if that's the route you want to take.

Simon

Re: [PATCH] Remove download size from debuginfod progress messages

[Aaron Merey]

Hi Simon,

On Thu, Mar 24, 2022 at 11:26 AM Simon Marchi <simark@simark.ca> wrote:
> I'm fine with an interim patch, but I'd really like to see the progress
> bar back, I enjoy it :)!
>
> I haven't been involved in those dicussions so I don't know what the
> plan is.  But typically progress bars have an "undefined total" mode
> where you can still tell that the operation is progressing (bytes are
> being received), you just don't what percentage of the total.

That is probably what we'll end up doing. The progress bar will return!

> But really, is it a problem for GDB master to print "~0.00 MB", until
> the "real" fix is merged?  It's not like it's causing a crash or
> anything like that?  Or is it something you want to push to the GDB 12
> branch?

We received a request to fix this issue and thought it would be best
to patch it here first before addressing it downstream.

> The patch LGTM in any case, if that's the route you want to take.

Thanks. Pushed as commit b0e0d830d9.

Aaron