[PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Aaron Merey]

In some cases the prompt "--Type <RET> for more, q to quit, c to
continue without paging--" can appear during a large series of
debuginfod downloads when lines_printed exceeds lines_allowed.

This is inconvenient plus ctrl-c during this prompt could leave some
of gdb's internal structures in a broken state.

Fix this by adding a bool count_lines_printed to control whether
lines_printed is incremented when a newline is printed.  Set this
value to false when performing a download.
---
 gdb/debuginfod-support.c |  6 ++++++
 gdb/utils.c              | 10 ++++++++--
 gdb/utils.h              |  2 ++
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/gdb/debuginfod-support.c b/gdb/debuginfod-support.c
index 04d254a1601..d5fb6153ee4 100644
--- a/gdb/debuginfod-support.c
+++ b/gdb/debuginfod-support.c
@@ -293,6 +293,8 @@ debuginfod_source_query (const unsigned char *build_id,
   user_data data ("source file", srcpath);
 
   debuginfod_set_user_data (c, &data);
+  scoped_restore save_count_lines_printed
+    = make_scoped_restore (&count_lines_printed, false);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   if (target_supports_terminal_ours ())
     {
@@ -334,6 +336,8 @@ debuginfod_debuginfo_query (const unsigned char *build_id,
   user_data data ("separate debug info for", filename);
 
   debuginfod_set_user_data (c, &data);
+  scoped_restore save_count_lines_printed
+    = make_scoped_restore (&count_lines_printed, false);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   if (target_supports_terminal_ours ())
     {
@@ -372,6 +376,8 @@ debuginfod_exec_query (const unsigned char *build_id,
   user_data data ("executable for", filename);
 
   debuginfod_set_user_data (c, &data);
+  scoped_restore save_count_lines_printed
+    = make_scoped_restore (&count_lines_printed, false);
   gdb::optional<target_terminal::scoped_restore_terminal_state> term_state;
   if (target_supports_terminal_ours ())
     {
diff --git a/gdb/utils.c b/gdb/utils.c
index 95adbe58e4a..cb6f7276ab3 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1166,6 +1166,10 @@ show_chars_per_line (struct ui_file *file, int from_tty,
 /* Current count of lines printed on this page, chars on this line.  */
 static unsigned int lines_printed, chars_printed;
 
+/* Controls whether to increment lines_printed.  */
+
+bool count_lines_printed = true;
+
 /* True if pagination is disabled for just one command.  */
 
 static bool pagination_disabled_for_command;
@@ -1675,7 +1679,8 @@ pager_file::puts (const char *linebuffer)
 	      bool did_paginate = false;
 
 	      chars_printed = 0;
-	      lines_printed++;
+	      if (count_lines_printed)
+		lines_printed++;
 	      if (m_wrap_column)
 		{
 		  /* We are about to insert a newline at an historic
@@ -1736,7 +1741,8 @@ pager_file::puts (const char *linebuffer)
 	{
 	  chars_printed = 0;
 	  wrap_here (0); /* Spit out chars, cancel further wraps.  */
-	  lines_printed++;
+	  if (count_lines_printed)
+	    lines_printed++;
 	  m_stream->puts ("\n");
 	  lineptr++;
 	}
diff --git a/gdb/utils.h b/gdb/utils.h
index 7865812998e..fdac3e03211 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -167,6 +167,8 @@ extern int get_chars_per_line ();
 
 extern bool pagination_enabled;
 
+extern bool count_lines_printed;
+
 /* A flag indicating whether to timestamp debugging messages.  */
 extern bool debug_timestamp;
 
-- 
2.39.0

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Tom Tromey]

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

Aaron> In some cases the prompt "--Type <RET> for more, q to quit, c to
Aaron> continue without paging--" can appear during a large series of
Aaron> debuginfod downloads when lines_printed exceeds lines_allowed.

Aaron> This is inconvenient plus ctrl-c during this prompt could leave some
Aaron> of gdb's internal structures in a broken state.

Aaron> Fix this by adding a bool count_lines_printed to control whether
Aaron> lines_printed is incremented when a newline is printed.  Set this
Aaron> value to false when performing a download.

There's already pagination_disabled for this.  Or you can print to a
stream other than gdb_stdout.

However, my question is why pagination is even enabled at the likely
spots where debuginfod might be called.  I thought infrun generally
disabled it.

Tom

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Aaron Merey]

On Sat, Jan 28, 2023 at 10:01 AM Tom Tromey <tom@tromey.com> wrote:
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> In some cases the prompt "--Type <RET> for more, q to quit, c to
> Aaron> continue without paging--" can appear during a large series of
> Aaron> debuginfod downloads when lines_printed exceeds lines_allowed.
>
> Aaron> This is inconvenient plus ctrl-c during this prompt could leave some
> Aaron> of gdb's internal structures in a broken state.
>
> Aaron> Fix this by adding a bool count_lines_printed to control whether
> Aaron> lines_printed is incremented when a newline is printed.  Set this
> Aaron> value to false when performing a download.
>
> There's already pagination_disabled for this.  Or you can print to a
> stream other than gdb_stdout.
>
> However, my question is why pagination is even enabled at the likely
> spots where debuginfod might be called.  I thought infrun generally
> disabled it.

I was able to get a prompt to show up with the command

  gdb  -ex 'set height 25' -ex 'start' qemu-kvm

Setting pagination_enabled to false does stop the prompt in this case,
however during testing of the on-demand downloading feature that I'm
working on [1] I saw cases where this still didn't prevent the prompt.

For example 'list main.c:50' might trigger a number of downloads where
debuginfod-related output increases lines_printed past the lines_allowed
limit.  When the source lines actually print, the prompt immediately
shows up since pagination has been re-enabled and lines_allowed was
exceeded.

Adding a count_lines_printed toggle takes care of this case too.
However I didn't realise that printing to a stream besides gdb_stdout
should avoid this.  I'll try to implement this fix using another stream.

Aaron

[1] https://sourceware.org/pipermail/gdb-patches/2022-November/193416.html

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Tom Tromey]

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

Aaron> I was able to get a prompt to show up with the command

Aaron>   gdb  -ex 'set height 25' -ex 'start' qemu-kvm

Yeah, but my question is why.  Like, I suppose I would expect that
somewhere in the stack between the debuginfod code and the main loop is
some bit of infrun that suppresses pagination.  But why isn't there, and
should there be?

It's fine if the answer is no, it just surprises me a bit that this is
happening.  After all the other symbol readers print things to stdout,
so presumably this could affect those as well.

Aaron> Setting pagination_enabled to false does stop the prompt in this case,
Aaron> however during testing of the on-demand downloading feature that I'm
Aaron> working on [1] I saw cases where this still didn't prevent the prompt.

If clearing pagination_enabled results in paging, then that sounds like
a serious bug.  Looking at the code I don't see how it can happen.
Could you track it down?

thanks,
Tom

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Aaron Merey]

On Tue, Jan 31, 2023 at 9:23 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
> Aaron> Setting pagination_enabled to false does stop the prompt in this case,
> Aaron> however during testing of the on-demand downloading feature that I'm
> Aaron> working on [1] I saw cases where this still didn't prevent the prompt.
>
> If clearing pagination_enabled results in paging, then that sounds like
> a serious bug.  Looking at the code I don't see how it can happen.
> Could you track it down?

Sure I'll take a look at it.

Aaron

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Tom Tromey]

>>>>> "Aaron" == Aaron Merey <amerey@redhat.com> writes:

Aaron> Setting pagination_enabled to false does stop the prompt in this case,
Aaron> however during testing of the on-demand downloading feature that I'm
Aaron> working on [1] I saw cases where this still didn't prevent the prompt.

>> If clearing pagination_enabled results in paging, then that sounds like
>> a serious bug.  Looking at the code I don't see how it can happen.
>> Could you track it down?

Aaron> Sure I'll take a look at it.

It occurred to me last night that this could be a subtle change
introduced by the pager rewrite.

Before, disabling pagination also disabled line splitting, and took an
earlier exit.

But now, if pagination is disabled for some output, those lines still
"count" towards the total.  So then when pagination is no longer
disabled, a print might cause the pager to intervene.

Is this what's happening in your case?  If so, I'm not totally sure it
is a bug.  Normally disabling paging should be done because using the
pager isn't safe or appropriate at some point in time.  But having those
lines still "count" seems alright, maybe even useful.

Tom

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Aaron Merey]

On Wed, Feb 8, 2023 at 9:44 AM Tom Tromey <tom@tromey.com> wrote:
> It occurred to me last night that this could be a subtle change
> introduced by the pager rewrite.
>
> Before, disabling pagination also disabled line splitting, and took an
> earlier exit.
>
> But now, if pagination is disabled for some output, those lines still
> "count" towards the total.  So then when pagination is no longer
> disabled, a print might cause the pager to intervene.
>
> Is this what's happening in your case?  If so, I'm not totally sure it
> is a bug.  Normally disabling paging should be done because using the
> pager isn't safe or appropriate at some point in time.  But having those
> lines still "count" seems alright, maybe even useful.

Yes pagination_enabled=false prevents prompt_for_continue but lines_printed
still increments for each newline printed.

Like you said, this prompt shouldn't show up around debuginfod progress
output since most downloads happen under fetch_inferior_event which
disables pagination until it returns.

The reproducer I previously mentioned

On Mon, Jan 30, 2023 at 9:09 PM Aaron Merey <amerey@redhat.com> wrote:
>   gdb  -ex 'set height 25' -ex 'start' qemu-kvm

triggers the prompt during a download occurring right before 'start'
begins running the inferior and pagination isn't disabled by
fetch_inferior_event.

I don't think this particular case is too much of a problem because
there are only a few downloads that can happen before the rest occur
under fetch_inferior_event.  So the prompt will only show up at the
start if gdb's height is unusually small, in which case you can expect
regular prompts anyways.

However the on-demand downloading work creates new situations where
a large series of downloads might happen outside fetch_inferior_event.
For example, "break client.c:50" could download the .debug_line and
.debug_line_str of each shared library to search for the filename
"client.c". Then it downloads debuginfo for each match, all of which
happens outside of fetch_inferior_event.

Setting pagination_enabled=false in debuginfod_*_query prevents the
prompt durings the downloads but lines_printed keeps increasing.
If lines_allowed is exceeded then after the download you get an (IMO)
annoying prompt:

    (gdb) break client.c:50
    [...]
    Downloading separate debug info for /lib64/libabc.so
    Downloading separate debug info for /lib64/libxyz.so
    --Type <RET> for more, q to quit, c to continue without paging--
    Breakpoint 1 at 0xcf525d: client.c:50. (2 locations)
    (gdb)

Adding a count_lines_printed flag gives us another way to fine-tune
when the prompt shows up.  It keeps prompt behavior around debuginfod
output consistent whether or not the inferior is running.

Aaron

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Tom Tromey]

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

Aaron> However the on-demand downloading work creates new situations where
Aaron> a large series of downloads might happen outside fetch_inferior_event.
Aaron> For example, "break client.c:50" could download the .debug_line and
Aaron> .debug_line_str of each shared library to search for the filename
Aaron> "client.c". Then it downloads debuginfo for each match, all of which
Aaron> happens outside of fetch_inferior_event.

Aaron> Setting pagination_enabled=false in debuginfod_*_query prevents the
Aaron> prompt durings the downloads but lines_printed keeps increasing.
Aaron> If lines_allowed is exceeded then after the download you get an (IMO)
Aaron> annoying prompt:

I think this is what the user has requested, though.
The pager can easily be disabled, but if we let code work around it,
there's not really any way to ask for it to be enabled again.

Aaron> Adding a count_lines_printed flag gives us another way to fine-tune
Aaron> when the prompt shows up.  It keeps prompt behavior around debuginfod
Aaron> output consistent whether or not the inferior is running.

This is an internal distinction but it seems to me that to the user, gdb
output is just gdb output and can't really be differentiated by its
origin.

Tom

Re: [PATCH] gdb/debuginfod: Prevent prompt for continue during downloading.

[Aaron Merey]

On Mon, Feb 13, 2023 at 9:18 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Aaron" == Aaron Merey via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Aaron> However the on-demand downloading work creates new situations where
> Aaron> a large series of downloads might happen outside fetch_inferior_event.
> Aaron> For example, "break client.c:50" could download the .debug_line and
> Aaron> .debug_line_str of each shared library to search for the filename
> Aaron> "client.c". Then it downloads debuginfo for each match, all of which
> Aaron> happens outside of fetch_inferior_event.
>
> Aaron> Setting pagination_enabled=false in debuginfod_*_query prevents the
> Aaron> prompt durings the downloads but lines_printed keeps increasing.
> Aaron> If lines_allowed is exceeded then after the download you get an (IMO)
> Aaron> annoying prompt:
>
> I think this is what the user has requested, though.
> The pager can easily be disabled, but if we let code work around it,
> there's not really any way to ask for it to be enabled again.

Fair point, although some users (myself included) may want promptless
debuginfod output without having to completely disable the pager. Maybe
avoiding line counting in these cases should be done through
'set debuginfod verbose'?  In any case I think the debuginfod_*_query
functions should at least disable pagination.

> Aaron> Adding a count_lines_printed flag gives us another way to fine-tune
> Aaron> when the prompt shows up.  It keeps prompt behavior around debuginfod
> Aaron> output consistent whether or not the inferior is running.
>
> This is an internal distinction but it seems to me that to the user, gdb
> output is just gdb output and can't really be differentiated by its
> origin.

Inconsistent behavior due to an internal distinction could make the
behavior even more perplexing to some users.

Aaron