[PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Aaron Merey]

clear_current_line overwrites the current line with chars_per_line
blank spaces.  Printing the final space triggers a condition in
pager_file::puts that causes lines_printed to be incremented.  If
lines_printed becomes greater than or equal to lines_allowed, the
pagination prompt will appear if enabled.

In this case the prompt is unnecessary since after printing the final
space clear_current_line immediately moves the cursor to the beginning
of the line with '\r'.  A new line isn't actually started, so the prompt
ends up being spurious.

Additionally it's possible for gdb to crash during this pagination prompt.
Answering the prompt with 'q' throws an exception intended to bring gdb
back to the main event loop.  But since commit 0fea10f32746,
clear_current_line may be called under the progress_update destructor.
The exception will try to propagate through a destructor, causing an abort.

This patch removes the possibility for clear_current_line to trigger the
prompt by only printing chars_per_line - 1 blank spaces.  clear_current_line
is only ever called to clear download progress bars, which do not print
anything on the final character of a line.

An additional step we could take is to revert commit 0fea10f32746 to
prevent any future possibility that an exception could be thrown during
the progress_update destructor.
---
 gdb/cli-out.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 4c598883d4b..e93de1d0aa5 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -396,7 +396,7 @@ cli_ui_out::clear_current_line ()
     chars_per_line = MAX_CHARS_PER_LINE;
 
   gdb_printf (stream, "\r");
-  for (int i = 0; i < chars_per_line; ++i)
+  for (int i = 0; i < chars_per_line - 1; ++i)
     gdb_printf (stream, " ");
   gdb_printf (stream, "\r");
 
-- 
2.39.2

Re: [PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Eli Zaretskii]

> Cc: Aaron Merey <amerey@redhat.com>
> Date: Thu, 11 May 2023 19:22:03 -0400
> From: Aaron Merey via Gdb-patches <gdb-patches@sourceware.org>
> 
> This patch removes the possibility for clear_current_line to trigger the
> prompt by only printing chars_per_line - 1 blank spaces.  clear_current_line
> is only ever called to clear download progress bars, which do not print
> anything on the final character of a line.

I'm not sure we should rely on the fact that the final character of
the line is never there.  clear_current_line is a general-purpose
method, so it should do its job regardless.

Can't we do something to handle the last character as well?  E.g.,
could it be possible first to delete one character, so there are
really only N-1 characters to fill with blank spaces?

Thanks.

Re: [PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Aaron Merey]

Hi Eli,

On Fri, May 12, 2023 at 2:50 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Cc: Aaron Merey <amerey@redhat.com>
> > Date: Thu, 11 May 2023 19:22:03 -0400
> > From: Aaron Merey via Gdb-patches <gdb-patches@sourceware.org>
> >
> > This patch removes the possibility for clear_current_line to trigger the
> > prompt by only printing chars_per_line - 1 blank spaces.  clear_current_line
> > is only ever called to clear download progress bars, which do not print
> > anything on the final character of a line.
>
> I'm not sure we should rely on the fact that the final character of
> the line is never there.  clear_current_line is a general-purpose
> method, so it should do its job regardless.
>
> Can't we do something to handle the last character as well?  E.g.,
> could it be possible first to delete one character, so there are
> really only N-1 characters to fill with blank spaces?

We could rename clear_current_line to something like clear_progress_notify
to help indicate that this is a special purpose function.  We could also
just disable pagination for the duration of clear_current_line.  I'm not
sure we can delete a character without incrementing chars_per_line.

Aaron

Re: [PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Eli Zaretskii]

> From: Aaron Merey <amerey@redhat.com>
> Date: Mon, 15 May 2023 17:32:48 -0400
> Cc: gdb-patches@sourceware.org
> 
> > I'm not sure we should rely on the fact that the final character of
> > the line is never there.  clear_current_line is a general-purpose
> > method, so it should do its job regardless.
> >
> > Can't we do something to handle the last character as well?  E.g.,
> > could it be possible first to delete one character, so there are
> > really only N-1 characters to fill with blank spaces?
> 
> We could rename clear_current_line to something like clear_progress_notify
> to help indicate that this is a special purpose function.  We could also
> just disable pagination for the duration of clear_current_line.

I think we should do both, thanks.

[PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Aaron Merey]

On Mon, May 15, 2023 at 10:27 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Merey <amerey@redhat.com>
> > Date: Mon, 15 May 2023 17:32:48 -0400
> > Cc: gdb-patches@sourceware.org
> >
> > > I'm not sure we should rely on the fact that the final character of
> > > the line is never there.  clear_current_line is a general-purpose
> > > method, so it should do its job regardless.
> > >
> > > Can't we do something to handle the last character as well?  E.g.,
> > > could it be possible first to delete one character, so there are
> > > really only N-1 characters to fill with blank spaces?
> >
> > We could rename clear_current_line to something like clear_progress_notify
> > to help indicate that this is a special purpose function.  We could also
> > just disable pagination for the duration of clear_current_line.
>
> I think we should do both, thanks.

I revised the patch with clear_current_line renamed to clear_progress_notify.
This function has been changed back to overwriting an entire line with whitespace
and pagination is disabled for its duration.

Aaron

---
 gdb/cli-out.c | 11 +++++++----
 gdb/cli-out.h |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/gdb/cli-out.c b/gdb/cli-out.c
index 4c598883d4b..20d3d93f1ad 100644
--- a/gdb/cli-out.c
+++ b/gdb/cli-out.c
@@ -378,15 +378,18 @@ cli_ui_out::do_progress_notify (const std::string &msg,
   return;
 }
 
-/* Clear the current line of the most recent progress update.  Overwrites
-   the current line with whitespace.  */
+/* Clear do_progress_notify output from the current line.  Overwrites the
+   notification with whitespace.  */
 
 void
-cli_ui_out::clear_current_line ()
+cli_ui_out::clear_progress_notify ()
 {
   struct ui_file *stream = m_streams.back ();
   int chars_per_line = get_chars_per_line ();
 
+  scoped_restore save_pagination
+    = make_scoped_restore (&pagination_enabled, false);
+
   if (!stream->isatty ()
       || !current_ui->input_interactive_p ()
       || chars_per_line < MIN_CHARS_PER_LINE)
@@ -413,7 +416,7 @@ cli_ui_out::do_progress_end ()
   m_progress_info.pop_back ();
 
   if (stream->isatty ())
-    clear_current_line ();
+    clear_progress_notify ();
 }
 
 /* local functions */
diff --git a/gdb/cli-out.h b/gdb/cli-out.h
index 0729834cbc6..34016182269 100644
--- a/gdb/cli-out.h
+++ b/gdb/cli-out.h
@@ -104,7 +104,7 @@ class cli_ui_out : public ui_out
 
   /* Stack of progress info.  */
   std::vector<cli_progress_info> m_progress_info;
-  void clear_current_line ();
+  void clear_progress_notify ();
 };
 
 extern void cli_display_match_list (char **matches, int len, int max);
-- 
2.39.2

Re: [PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Eli Zaretskii]

> From: Aaron Merey <amerey@redhat.com>
> Cc: gdb-patches@sourceware.org,
> 	Aaron Merey <amerey@redhat.com>
> Date: Fri, 19 May 2023 17:38:24 -0400
> 
> On Mon, May 15, 2023 at 10:27 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > > From: Aaron Merey <amerey@redhat.com>
> > > Date: Mon, 15 May 2023 17:32:48 -0400
> > > Cc: gdb-patches@sourceware.org
> > >
> > > > I'm not sure we should rely on the fact that the final character of
> > > > the line is never there.  clear_current_line is a general-purpose
> > > > method, so it should do its job regardless.
> > > >
> > > > Can't we do something to handle the last character as well?  E.g.,
> > > > could it be possible first to delete one character, so there are
> > > > really only N-1 characters to fill with blank spaces?
> > >
> > > We could rename clear_current_line to something like clear_progress_notify
> > > to help indicate that this is a special purpose function.  We could also
> > > just disable pagination for the duration of clear_current_line.
> >
> > I think we should do both, thanks.
> 
> I revised the patch with clear_current_line renamed to clear_progress_notify.
> This function has been changed back to overwriting an entire line with whitespace
> and pagination is disabled for its duration.

Thanks.

Acked-by: Eli Zaretskii <eliz@gnu.org>

Re: [PATCH] gdb/cli-out.c: clear_current_line shouldn't trigger pagination prompt

[Aaron Merey]

On Sat, May 20, 2023 at 1:47 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Aaron Merey <amerey@redhat.com>
> > Cc: gdb-patches@sourceware.org,
> >       Aaron Merey <amerey@redhat.com>
> > Date: Fri, 19 May 2023 17:38:24 -0400
> >
> > On Mon, May 15, 2023 at 10:27 PM Eli Zaretskii <eliz@gnu.org> wrote:
> > >
> > > > From: Aaron Merey <amerey@redhat.com>
> > > > Date: Mon, 15 May 2023 17:32:48 -0400
> > > > Cc: gdb-patches@sourceware.org
> > > >
> > > > > I'm not sure we should rely on the fact that the final character of
> > > > > the line is never there.  clear_current_line is a general-purpose
> > > > > method, so it should do its job regardless.
> > > > >
> > > > > Can't we do something to handle the last character as well?  E.g.,
> > > > > could it be possible first to delete one character, so there are
> > > > > really only N-1 characters to fill with blank spaces?
> > > >
> > > > We could rename clear_current_line to something like clear_progress_notify
> > > > to help indicate that this is a special purpose function.  We could also
> > > > just disable pagination for the duration of clear_current_line.
> > >
> > > I think we should do both, thanks.
> >
> > I revised the patch with clear_current_line renamed to clear_progress_notify.
> > This function has been changed back to overwriting an entire line with whitespace
> > and pagination is disabled for its duration.
>
> Thanks.
>
> Acked-by: Eli Zaretskii <eliz@gnu.org>

Thanks Eli, merged as commit 6aebb6e100fb3c

Aaron