[PATCH 0/3] Minor pager fixes

[PATCH 0/3] Minor pager fixes

[Tom Tromey]

This fixes a couple of minor bugs that were reported against the
pager.  Also, while I was working in this area again, I changed
'gdb_putc' to return 'void'.

Regression tested on x86-64 Fedora 34.

Tom

[PATCH 1/3] Allow word wrapping even when paging is disabled

[Tom Tromey]

PR cli/20741 points out that when pagination is disabled, this also
disabled word wrapping.  However, the manual documents that these
settings are separate -- if you intend to disable the wrapping, you
must use "set width unlimited".

This patch fixes the bug by letting the pagination-disabled case fall
through to the code that also handles word-wrapping.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=20741
---
 gdb/utils.c | 52 ++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 68bf3a67340..184ddf56849 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1569,10 +1569,8 @@ pager_file::puts (const char *linebuffer)
   if (linebuffer == 0)
     return;
 
-  /* Don't do any filtering if it is disabled.  */
-  if (!pagination_enabled
-      || pagination_disabled_for_command
-      || batch_flag
+  /* Don't do any filtering or wrapping if both are disabled.  */
+  if (batch_flag
       || (lines_per_page == UINT_MAX && chars_per_line == UINT_MAX)
       || top_level_interpreter () == NULL
       || top_level_interpreter ()->interp_ui_out ()->is_mi_like_p ())
@@ -1600,8 +1598,9 @@ pager_file::puts (const char *linebuffer)
       /* Possible new page.  Note that PAGINATION_DISABLED_FOR_COMMAND
 	 might be set during this loop, so we must continue to check
 	 it here.  */
-      if ((lines_printed >= lines_per_page - 1)
-	  && !pagination_disabled_for_command)
+      if (pagination_enabled
+	  && !pagination_disabled_for_command
+	  && lines_printed >= lines_per_page - 1)
 	prompt_for_continue ();
 
       while (*lineptr && *lineptr != '\n')
@@ -1676,8 +1675,9 @@ pager_file::puts (const char *linebuffer)
 	      /* Possible new page.  Note that
 		 PAGINATION_DISABLED_FOR_COMMAND might be set during
 		 this loop, so we must continue to check it here.  */
-	      if (lines_printed >= lines_per_page - 1
-		  && !pagination_disabled_for_command)
+	      if (pagination_enabled
+		  && !pagination_disabled_for_command
+		  && lines_printed >= lines_per_page - 1)
 		{
 		  prompt_for_continue ();
 		  did_paginate = true;
@@ -1731,6 +1731,41 @@ pager_file::write (const char *buf, long length_buf)
   this->puts (str.c_str ());
 }
 
+#if GDB_SELF_TEST
+
+/* Test that disabling the pager does not also disable word
+   wrapping.  */
+
+static void
+test_pager ()
+{
+  string_file *strfile = new string_file ();
+  pager_file pager (strfile);
+
+  /* Make sure the pager is disabled.  */
+  scoped_restore save_enabled
+    = make_scoped_restore (&pagination_enabled, false);
+  scoped_restore save_disabled
+    = make_scoped_restore (&pagination_disabled_for_command, false);
+  scoped_restore save_batch
+    = make_scoped_restore (&batch_flag, false);
+  scoped_restore save_lines
+    = make_scoped_restore (&lines_per_page, 50);
+  /* Make it easy to word wrap.  */
+  scoped_restore save_chars
+    = make_scoped_restore (&chars_per_line, 15);
+  scoped_restore save_printed
+    = make_scoped_restore (&chars_printed, 0);
+
+  pager.puts ("aaaaaaaaaaaa");
+  pager.wrap_here (2);
+  pager.puts ("bbbbbbbbbbbb\n");
+
+  SELF_CHECK (strfile->string () == "aaaaaaaaaaaa\n  bbbbbbbbbbbb\n");
+}
+
+#endif /* GDB_SELF_TEST */
+
 void
 gdb_puts (const char *linebuffer, struct ui_file *stream)
 {
@@ -3683,5 +3718,6 @@ When set, debugging messages will be marked with seconds and microseconds."),
   selftests::register_test ("gdb_argv_array_view", gdb_argv_as_array_view_test);
   selftests::register_test ("strncmp_iw_with_mode",
 			    strncmp_iw_with_mode_tests);
+  selftests::register_test ("pager", test_pager);
 #endif
 }
-- 
2.34.1

[PATCH 2/3] Handle "set height 1"

[Tom Tromey]

PR cli/17151 points out that "set height 1" has pathological behavior
in gdb.  What I see is that gdb will endlessly print the pagination
prompt.  This patch takes a simple and expedient approach to a fix:
pretend that the height is really 2.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=17151
---
 gdb/testsuite/gdb.base/page.exp |  7 ++++---
 gdb/utils.c                     | 10 ++++++++--
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/page.exp b/gdb/testsuite/gdb.base/page.exp
index a38f1dd0c94..268777f8acb 100644
--- a/gdb/testsuite/gdb.base/page.exp
+++ b/gdb/testsuite/gdb.base/page.exp
@@ -121,6 +121,7 @@ foreach_with_prefix size {"0" "0x80000000" "unlimited"} {
 gdb_test "set width -1" "integer -1 out of range"
 gdb_test "set height -1" "integer -1 out of range"
 
-gdb_exit
-return 0
-
+# A height of 1 used to cause pathological behavior -- it would
+# endlessly prompt for paging without printing anything.
+gdb_test_no_output "set height 1"
+gdb_test "print 23" " = 23"
diff --git a/gdb/utils.c b/gdb/utils.c
index 184ddf56849..f455ae1e708 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1588,6 +1588,12 @@ pager_file::puts (const char *linebuffer)
 			 m_wrap_indent = 0;
 		       });
 
+  /* If the user does "set height 1" then the pager will exhibit weird
+     behavior.  This is pathological, though, so don't allow it.  */
+  const unsigned int lines_allowed = (lines_per_page > 1
+				      ? lines_per_page - 1
+				      : 1);
+
   /* Go through and output each character.  Show line extension
      when this is necessary; prompt user for new page when this is
      necessary.  */
@@ -1600,7 +1606,7 @@ pager_file::puts (const char *linebuffer)
 	 it here.  */
       if (pagination_enabled
 	  && !pagination_disabled_for_command
-	  && lines_printed >= lines_per_page - 1)
+	  && lines_printed >= lines_allowed)
 	prompt_for_continue ();
 
       while (*lineptr && *lineptr != '\n')
@@ -1677,7 +1683,7 @@ pager_file::puts (const char *linebuffer)
 		 this loop, so we must continue to check it here.  */
 	      if (pagination_enabled
 		  && !pagination_disabled_for_command
-		  && lines_printed >= lines_per_page - 1)
+		  && lines_printed >= lines_allowed)
 		{
 		  prompt_for_continue ();
 		  did_paginate = true;
-- 
2.34.1

[PATCH 3/3] Return void from gdb_putc

[Tom Tromey]

I don't think it's very useful to return the character from gdb_putc,
so this patch changes it to return void.
---
 gdb/ui-file.c | 3 +--
 gdb/ui-file.h | 2 +-
 gdb/utils.c   | 7 ++-----
 gdb/utils.h   | 4 ++--
 4 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/gdb/ui-file.c b/gdb/ui-file.c
index afb12b408f5..47044e42a67 100644
--- a/gdb/ui-file.c
+++ b/gdb/ui-file.c
@@ -60,12 +60,11 @@ ui_file::putstrn (const char *str, int n, int quoter, bool async_safe)
     printchar (str[i], quoter, async_safe);
 }
 
-int
+void
 ui_file::putc (int c)
 {
   char copy = (char) c;
   write (&copy, 1);
-  return c;
 }
 
 void
diff --git a/gdb/ui-file.h b/gdb/ui-file.h
index f8e1fe8be3c..7e15d98d6da 100644
--- a/gdb/ui-file.h
+++ b/gdb/ui-file.h
@@ -51,7 +51,7 @@ class ui_file
      write_async_safe method.  */
   void putstrn (const char *str, int n, int quoter, bool async_safe = false);
 
-  int putc (int c);
+  void putc (int c);
 
   void vprintf (const char *, va_list) ATTRIBUTE_PRINTF (2, 0);
 
diff --git a/gdb/utils.c b/gdb/utils.c
index f455ae1e708..ae9a488337e 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1825,16 +1825,13 @@ fputs_highlighted (const char *str, const compiled_regex &highlight,
     gdb_puts (str, stream);
 }
 
-/* Write character C to gdb_stdout using GDB's paging mechanism and return C.
-   May return nonlocally.  */
-
-int
+void
 gdb_putc (int c)
 {
   return gdb_stdout->putc (c);
 }
 
-int
+void
 gdb_putc (int c, struct ui_file *stream)
 {
   return stream->putc (c);
diff --git a/gdb/utils.h b/gdb/utils.h
index c1ac85497bf..7e2028580bf 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -224,9 +224,9 @@ extern void set_screen_width_and_height (int width, int height);
 
 extern void gdb_puts (const char *, struct ui_file *);
 
-extern int gdb_putc (int c, struct ui_file *);
+extern void gdb_putc (int c, struct ui_file *);
 
-extern int gdb_putc (int c);
+extern void gdb_putc (int c);
 
 extern void gdb_puts (const char *);
 
-- 
2.34.1

Re: [PATCH 2/3] Handle "set height 1"

[Eli Zaretskii]

> From: Tom Tromey <tom@tromey.com>
> Date: Fri,  1 Apr 2022 22:46:38 -0600
> Cc: Tom Tromey <tom@tromey.com>
> 
> PR cli/17151 points out that "set height 1" has pathological behavior
> in gdb.  What I see is that gdb will endlessly print the pagination
> prompt.  This patch takes a simple and expedient approach to a fix:
> pretend that the height is really 2.

Should this be documented?

Re: [PATCH 2/3] Handle "set height 1"

[Tom Tromey]

>> PR cli/17151 points out that "set height 1" has pathological behavior
>> in gdb.  What I see is that gdb will endlessly print the pagination
>> prompt.  This patch takes a simple and expedient approach to a fix:
>> pretend that the height is really 2.

Eli> Should this be documented?

My view is it's a pathological case that doesn't really come up, so it's
fine for it to behave in any reasonable way, so I don't think we need to
document it.

Tom

Re: [PATCH 0/3] Minor pager fixes

[Tom Tromey]

>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

Tom> This fixes a couple of minor bugs that were reported against the
Tom> pager.  Also, while I was working in this area again, I changed
Tom> 'gdb_putc' to return 'void'.

Tom> Regression tested on x86-64 Fedora 34.

I'm checking these in.

Tom