[pushed] [gdb/tui] Fix TUI resizing for TERM=ansi

[pushed] [gdb/tui] Fix TUI resizing for TERM=ansi

[Tom de Vries]

With TERM=ansi, when resizing a TUI window from LINES/COLUMNS 31/118
(maximized) to 20/78 (de-maximized), I get a garbled screen (that ^L doesn't
fix) and a message:
...
@@ resize done 0, size = 77x20
...
with the resulting width being 77 instead of the expected 78.

[ The discrepancy also manifests in CLI, filed as PR30346. ]

The discrepancy comes from tui_resize_all, where we ask readline for the
screen size:
...
   rl_get_screen_size (&screenheight, &screenwidth);
...

As it happens, when TERM is set to ansi, readline decides that the terminal
cannot auto-wrap lines, and reserves one column to deal with that, and as a
result reports back one less than the actual screen width:
...
$ echo $COLUMNS
78
$ TERM=xterm gdb -ex "show width" -ex q
Number of characters gdb thinks are in a line is 78.
$ TERM=ansi  gdb -ex "show width" -ex q
Number of characters gdb thinks are in a line is 77.
...

In tui_resize_all, we need the actual screen width, and using a screenwidth of
one less than the actual value garbles the screen.

This is currently not causing trouble in testing because we have a workaround
in place in proc Term::resize.  If we disable the workaround:
...
-       stty columns [expr {$_cols + 1}] < $::gdb_tty_name
+       stty columns $_cols < $::gdb_tty_name
...
and dump the screen we get the same type of screen garbling:
...
    0 +---------------------------------------+|
    1                                         ||
    2                                         ||
    3                                         ||
...

Another way to reproduce the problem is using command "maint info screen".
After starting gdb with TERM=ansi, entering TUI, and issuing the command, we
get:
...
Number of characters curses thinks are in a line is 78.
...
and after maximizing and demaximizing the window we get:
...
Number of characters curses thinks are in a line is 77.
...
If we use TERM=xterm, we do get the expected 78.

Fix this by:
- detecting when readline will report back less than the actual screen width,
- accordingly setting a new variable readline_hidden_cols,
- using readline_hidden_cols in tui_resize_all to fix the resize problem, and
- removing the workaround in Term::resize.

The test-case gdb.tui/empty.exp serves as regression test.

I've applied the same fix in tui_async_resize_screen, the new test-case
gdb.tui/resize-2.exp serves as a regression test for that change.  Without
that fix, we have:
...
FAIL: gdb.tui/resize-2.exp: again: gdb width 80
...

Tested on x86_64-linux.

PR tui/30337
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30337
---
 gdb/testsuite/gdb.tui/resize-2.exp | 89 ++++++++++++++++++++++++++++++
 gdb/testsuite/lib/tuiterm.exp      | 18 +++---
 gdb/tui/tui-win.c                  |  4 ++
 gdb/utils.c                        | 21 +++++++
 gdb/utils.h                        |  7 +++
 5 files changed, 129 insertions(+), 10 deletions(-)
 create mode 100644 gdb/testsuite/gdb.tui/resize-2.exp

diff --git a/gdb/testsuite/gdb.tui/resize-2.exp b/gdb/testsuite/gdb.tui/resize-2.exp
new file mode 100644
index 00000000000..ebe8216c05d
--- /dev/null
+++ b/gdb/testsuite/gdb.tui/resize-2.exp
@@ -0,0 +1,89 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test TUI resizing using maint screen info command.
+
+require allow_tui_tests
+
+tuiterm_env
+
+Term::clean_restart 24 80
+set screen_dim { 0 0 80 24 }
+
+# Use a layout with just a command window.
+gdb_test "tui new-layout command-layout cmd 1"
+
+if {![Term::prepare_for_tui]} {
+    unsupported "TUI not supported"
+    return 0
+}
+
+# Enter TUI.
+Term::command_no_prompt_prefix "layout command-layout"
+
+proc check_width { what n } {
+    set re "Number of characters $what thinks are in a line is $n"
+    Term::check_region_contents "$what width $n" {*}$::screen_dim $re
+}
+
+# Check that curses has the correct notion of screen width.
+Term::command "maint info screen"
+check_width curses 80
+check_width gdb 80
+
+# Resize with TUI enabled, wait for the resize message.
+Term::resize 40 90
+set screen_dim { 0 0 90 40 }
+
+# Check that curses has the correct notion of screen width after resize.
+Term::command "maint info screen"
+check_width curses 90
+check_width gdb 90
+
+# Temporarily disable TUI.
+gdb_test_multiple "tui disable" "" {
+    -re "$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+# Resize with TUI disabled, so don't wait for the resize message.
+Term::resize 24 80 0
+set screen_dim { 0 0 80 24 }
+gdb_test_multiple "" "two prompt redisplays after resize" {
+    -re "\r.*$gdb_prompt \r.*$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+
+# At this point, curses still thinks the width is 90.  This doesn't look
+# harmful because TUI is disabled.
+gdb_test "maint info screen" \
+    "\r\nNumber of characters curses thinks are in a line is 90.\\r\n.*" \
+    "curses width after resize with TUI disabled"
+
+# Re-enable TUI.
+send_gdb "tui enable\n"
+# The "tui enable" command is issued on the CLI screen, on the TUI we have the
+# last command issued there: "tui disable".
+Term::wait_for "tui disable"
+
+# Check that curses has the correct notion of screen width after screen resize
+# with TUI disabled.
+Term::command "maint info screen"
+with_test_prefix again {
+    check_width curses 80
+    check_width gdb 80
+}
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index d8a99d2798a..8f78c320b66 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -1111,7 +1111,7 @@ namespace eval Term {
 	}
     }
 
-    proc resize {rows cols} {
+    proc resize {rows cols {wait_for_msg 1}} {
 	variable _rows
 	variable _cols
 	variable _resize_count
@@ -1122,17 +1122,15 @@ namespace eval Term {
 	# explicit here.  This also simplifies waiting for the redraw.
 	_do_resize $rows $_cols
 	stty rows $_rows < $::gdb_tty_name
-	# Due to the strange column resizing behavior, and because we
-	# don't care about this intermediate resize, we don't check
-	# the size and the "@@ " prefix here.
-	wait_for "resize done $_resize_count"
+	if { $wait_for_msg } {
+	    wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
+	}
 	incr _resize_count
-	# Somehow the number of columns transmitted to gdb is one less
-	# than what we request from expect.  We hide this weird
-	# details from the caller.
 	_do_resize $_rows $cols
-	stty columns [expr {$_cols + 1}] < $::gdb_tty_name
-	wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
+	stty columns $_cols < $::gdb_tty_name
+	if { $wait_for_msg } {
+	    wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
+	}
 	incr _resize_count
     }
 }
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 3b17cb8dd29..7eac03f47a1 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -36,6 +36,7 @@
 #include "gdbsupport/event-loop.h"
 #include "gdbcmd.h"
 #include "async-event.h"
+#include "utils.h"
 
 #include "tui/tui.h"
 #include "tui/tui-io.h"
@@ -528,6 +529,8 @@ tui_resize_all (void)
   int screenheight, screenwidth;
 
   rl_get_screen_size (&screenheight, &screenwidth);
+  screenwidth += readline_hidden_cols;
+
   width_diff = screenwidth - tui_term_width ();
   height_diff = screenheight - tui_term_height ();
   if (height_diff || width_diff)
@@ -576,6 +579,7 @@ tui_async_resize_screen (gdb_client_data arg)
       int screen_height, screen_width;
 
       rl_get_screen_size (&screen_height, &screen_width);
+      screen_width += readline_hidden_cols;
       set_screen_width_and_height (screen_width, screen_height);
 
       /* win_resized is left set so that the next call to tui_enable()
diff --git a/gdb/utils.c b/gdb/utils.c
index 002a5885aff..e10198accd0 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1116,6 +1116,14 @@ static bool filter_initialized = false;
 
 \f
 
+/* See readline's rlprivate.h.  */
+
+EXTERN_C int _rl_term_autowrap;
+
+/* See utils.h.  */
+
+int readline_hidden_cols = 0;
+
 /* Initialize the number of lines per page and chars per line.  */
 
 void
@@ -1144,6 +1152,19 @@ init_page_info (void)
 
       /* Get the screen size from Readline.  */
       rl_get_screen_size (&rows, &cols);
+
+      /* Readline:
+	 - ignores the COLUMNS variable when detecting screen width
+	   (because rl_prefer_env_winsize defaults to 0)
+	 - puts the detected screen width in the COLUMNS variable
+	   (because rl_change_environment defaults to 1)
+	 - may report one less than the detected screen width in
+	   rl_get_screen_size (when _rl_term_autowrap == 0).
+	 We could set readline_hidden_cols by comparing COLUMNS to cols as
+	 returned by rl_get_screen_size, but instead simply use
+	 _rl_term_autowrap.  */
+      readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
+
       lines_per_page = rows;
       chars_per_line = cols;
 
diff --git a/gdb/utils.h b/gdb/utils.h
index a383036bcfe..29ff376bb52 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -335,4 +335,11 @@ extern void copy_bitwise (gdb_byte *dest, ULONGEST dest_offset,
 			  const gdb_byte *source, ULONGEST source_offset,
 			  ULONGEST nbits, int bits_big_endian);
 
+/* When readline decides that the terminal cannot auto-wrap lines, it reduces
+   the width of the reported screen width by 1.  This variable indicates
+   whether that's the case or not, allowing us to add it back where
+   necessary.  See _rl_term_autowrap in readline/terminal.c.  */
+
+extern int readline_hidden_cols;
+
 #endif /* UTILS_H */

base-commit: bec5d8fc8c78e8a3da2c168366f33a856d55124b
-- 
2.35.3

Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> As it happens, when TERM is set to ansi, readline decides that the terminal
Tom> cannot auto-wrap lines, and reserves one column to deal with that, and as a
Tom> result reports back one less than the actual screen width:
...
Tom> This is currently not causing trouble in testing because we have a workaround
Tom> in place in proc Term::resize.  If we disable the workaround:

Thank you for tracking this down.
I never even considered this might be a readline issue.

Tom> +      readline_hidden_cols = _rl_term_autowrap ? 0 : 1;

I hate to have a new dependency on a readline internal variable.
Don't some distros mark these as hidden in libreadline.so?
I feel like there was another bug along these lines.

However, I don't see another way to do it.
Maybe some official API could be added by the upstream readline.
Would you want to bring it up there?

Anyway it seems ok to me.

thanks,
Tom

[PATCH] [gdb/build] Remove dependency on _rl_term_autowrap

[Tom de Vries]

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

[ was: Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi ]

On 4/30/23 21:15, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> As it happens, when TERM is set to ansi, readline decides that the terminal
> Tom> cannot auto-wrap lines, and reserves one column to deal with that, and as a
> Tom> result reports back one less than the actual screen width:
> ...
> Tom> This is currently not causing trouble in testing because we have a workaround
> Tom> in place in proc Term::resize.  If we disable the workaround:
> 
> Thank you for tracking this down.
> I never even considered this might be a readline issue.
> 
> Tom> +      readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
> 
> I hate to have a new dependency on a readline internal variable.

Agreed, it's ugly.

> Don't some distros mark these as hidden in libreadline.so?
> I feel like there was another bug along these lines.
> 

That's probably PR10723 - "Dependency on readline internal variable" ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=10723 ).

> However, I don't see another way to do it.

I had an earlier version of the patch that used the COLUMNS env variable 
instead.  It felt a bit hacky, so I went for _rl_term_autowrap, but 
patch attached below goes back to the COLUMNS approach.  WDYT?

> Maybe some official API could be added by the upstream readline.
> Would you want to bring it up there?
> 

If we commit the patch below, then this is solved for _rl_term_autowrap, 
but not for the other instances.

I expect bringing it up will will need to be done per instance, with a 
detailed explanation of why we need it, so that sounds like project I 
don't have time for atm.

> Anyway it seems ok to me.

Thanks for the review.

- Tom

[-- Attachment #2: 0001-gdb-build-Remove-dependency-on-_rl_term_autowrap.patch --]
[-- Type: text/x-patch, Size: 2219 bytes --]

From dbb4a6b592df08d6263af38c4857c55acb540352 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Mon, 1 May 2023 07:19:33 +0200
Subject: [PATCH] [gdb/build] Remove dependency on _rl_term_autowrap

Commit deb1ba4e38b ("[gdb/tui] Fix TUI resizing for TERM=ansi") introduced a
dependency on readline private variable _rl_term_autowrap.

There is precedent for this, but it's something we want to get rid of
(PR build/10723).

Remove the dependency on _rl_term_autowrap, and instead calculate
readline_hidden_cols by comparing the environment variable COLS with cols as
returned by rl_get_screen_size.

Tested on x86_64-linux.

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

diff --git a/gdb/utils.c b/gdb/utils.c
index e10198accd0..8738d3fc7e4 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1116,10 +1116,6 @@ static bool filter_initialized = false;
 
 \f
 
-/* See readline's rlprivate.h.  */
-
-EXTERN_C int _rl_term_autowrap;
-
 /* See utils.h.  */
 
 int readline_hidden_cols = 0;
@@ -1160,10 +1156,17 @@ init_page_info (void)
 	   (because rl_change_environment defaults to 1)
 	 - may report one less than the detected screen width in
 	   rl_get_screen_size (when _rl_term_autowrap == 0).
-	 We could set readline_hidden_cols by comparing COLUMNS to cols as
-	 returned by rl_get_screen_size, but instead simply use
-	 _rl_term_autowrap.  */
-      readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
+	 We could use _rl_term_autowrap, but we want to avoid introducing
+	 another dependency on readline private variables, so set
+	 readline_hidden_cols by comparing COLUMNS to cols as returned by
+	 rl_get_screen_size.  */
+      char *columns_env_str = getenv ("COLUMNS");
+      gdb_assert (columns_env_str != nullptr);
+      int columns_env_val = atoi (columns_env_str);
+      gdb_assert (columns_env_val != 0);
+      readline_hidden_cols = columns_env_val - cols;
+      gdb_assert (readline_hidden_cols >= 0);
+      gdb_assert (readline_hidden_cols <= 1);
 
       lines_per_page = rows;
       chars_per_line = cols;

base-commit: 077a1f08485e88f3b234af1dbb8b907b16045e6a
-- 
2.35.3

[PING][PATCH] [gdb/build] Remove dependency on _rl_term_autowrap

[Tom de Vries]

On 5/1/23 09:45, Tom de Vries wrote:
> [ was: Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi ]
> 
> On 4/30/23 21:15, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries via Gdb-patches 
>>>>>>> <gdb-patches@sourceware.org> writes:
>>
>> Tom> As it happens, when TERM is set to ansi, readline decides that 
>> the terminal
>> Tom> cannot auto-wrap lines, and reserves one column to deal with 
>> that, and as a
>> Tom> result reports back one less than the actual screen width:
>> ...
>> Tom> This is currently not causing trouble in testing because we have 
>> a workaround
>> Tom> in place in proc Term::resize.  If we disable the workaround:
>>
>> Thank you for tracking this down.
>> I never even considered this might be a readline issue.
>>
>> Tom> +      readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
>>
>> I hate to have a new dependency on a readline internal variable.
> 
> Agreed, it's ugly.
> 
>> Don't some distros mark these as hidden in libreadline.so?
>> I feel like there was another bug along these lines.
>>
> 
> That's probably PR10723 - "Dependency on readline internal variable" ( 
> https://sourceware.org/bugzilla/show_bug.cgi?id=10723 ).
> 
>> However, I don't see another way to do it.
> 
> I had an earlier version of the patch that used the COLUMNS env variable 
> instead.  It felt a bit hacky, so I went for _rl_term_autowrap, but 
> patch attached below goes back to the COLUMNS approach.  WDYT?
> 

Ping.

If this is not acceptable, I'll bring up making the variable public on 
the readline mailing list.

Thanks,
- Tom

>> Maybe some official API could be added by the upstream readline.
>> Would you want to bring it up there?
>>
> 
> If we commit the patch below, then this is solved for _rl_term_autowrap, 
> but not for the other instances.
> 
> I expect bringing it up will will need to be done per instance, with a 
> detailed explanation of why we need it, so that sounds like project I 
> don't have time for atm.
> 
>> Anyway it seems ok to me.
> 
> Thanks for the review.
> 
> - Tom

[PING^2][PATCH] [gdb/build] Remove dependency on _rl_term_autowrap

[Tom de Vries]

On 5/16/23 10:53, Tom de Vries via Gdb-patches wrote:
> On 5/1/23 09:45, Tom de Vries wrote:
>> [ was: Re: [pushed] [gdb/tui] Fix TUI resizing for TERM=ansi ]
>>
>> On 4/30/23 21:15, Tom Tromey wrote:
>>>>>>>> "Tom" == Tom de Vries via Gdb-patches 
>>>>>>>> <gdb-patches@sourceware.org> writes:
>>>
>>> Tom> As it happens, when TERM is set to ansi, readline decides that 
>>> the terminal
>>> Tom> cannot auto-wrap lines, and reserves one column to deal with 
>>> that, and as a
>>> Tom> result reports back one less than the actual screen width:
>>> ...
>>> Tom> This is currently not causing trouble in testing because we have 
>>> a workaround
>>> Tom> in place in proc Term::resize.  If we disable the workaround:
>>>
>>> Thank you for tracking this down.
>>> I never even considered this might be a readline issue.
>>>
>>> Tom> +      readline_hidden_cols = _rl_term_autowrap ? 0 : 1;
>>>
>>> I hate to have a new dependency on a readline internal variable.
>>
>> Agreed, it's ugly.
>>
>>> Don't some distros mark these as hidden in libreadline.so?
>>> I feel like there was another bug along these lines.
>>>
>>
>> That's probably PR10723 - "Dependency on readline internal variable" ( 
>> https://sourceware.org/bugzilla/show_bug.cgi?id=10723 ).
>>
>>> However, I don't see another way to do it.
>>
>> I had an earlier version of the patch that used the COLUMNS env 
>> variable instead.  It felt a bit hacky, so I went for 
>> _rl_term_autowrap, but patch attached below goes back to the COLUMNS 
>> approach.  WDYT?
>>
> 

Ping^2.

Thanks,
- Tom

> 
> If this is not acceptable, I'll bring up making the variable public on 
> the readline mailing list.
> 
> Thanks,
> - Tom
> 
>>> Maybe some official API could be added by the upstream readline.
>>> Would you want to bring it up there?
>>>
>>
>> If we commit the patch below, then this is solved for 
>> _rl_term_autowrap, but not for the other instances.
>>
>> I expect bringing it up will will need to be done per instance, with a 
>> detailed explanation of why we need it, so that sounds like project I 
>> don't have time for atm.
>>
>>> Anyway it seems ok to me.
>>
>> Thanks for the review.
>>
>> - Tom
> 

Re: [PATCH] [gdb/build] Remove dependency on _rl_term_autowrap

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

>> However, I don't see another way to do it.

Tom> I had an earlier version of the patch that used the COLUMNS env
Tom> variable instead.  It felt a bit hacky, so I went for
Tom> _rl_term_autowrap, but patch attached below goes back to the COLUMNS
Tom> approach.  WDYT?

Looks good to me.  Maybe one nit:

Tom> +      char *columns_env_str = getenv ("COLUMNS");

... mildly better to write "const char *" here.

Tom