[8/8,gdb/tui] Fix TUI for TERM=ansi

  With TERM=ansi, when resizing a TUI window from LINES/COLUMNS 31/118
(maximized) to 20/78 (de-maximized), I get a garbled screen 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 that 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.

Fix this by:
- detecting when readline reports 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.

Tested on x86_64-linux.

PR tui/30337
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30337
---
 gdb/testsuite/lib/tuiterm.exp | 10 ++--------
 gdb/tui/tui-win.c             |  3 +++
 gdb/utils.c                   |  9 +++++++++
 gdb/utils.h                   |  7 +++++++
 4 files changed, 21 insertions(+), 8 deletions(-)
  

On 4/13/23 16:08, Tom de Vries via Gdb-patches wrote:
> @@ -1144,6 +1148,11 @@ init_page_info (void)
>   
>         /* Get the screen size from Readline.  */
>         rl_get_screen_size (&rows, &cols);
> +      if (gdb_stdout->isatty ()) {
> +	readline_hidden_cols = COLS - cols;
> +	gdb_assert (readline_hidden_cols >= 0);
> +	gdb_assert (readline_hidden_cols <= 1);
> +      }
>         lines_per_page = rows;
>         chars_per_line = cols;
>   

Reading this over once more, I noticed this is missing a fix for an 
assertion failure we run into when doing:
...
$ TERM=blabla gdb
...

So, this bit is missing:
...
-      if (gdb_stdout->isatty ()) {
+      if (gdb_stdout->isatty () && COLS != 0) {
...

Thanks,
- Tom
  

On 4/13/23 16:17, Tom de Vries via Gdb-patches wrote:
> On 4/13/23 16:08, Tom de Vries via Gdb-patches wrote:
>> @@ -1144,6 +1148,11 @@ init_page_info (void)
>>         /* Get the screen size from Readline.  */
>>         rl_get_screen_size (&rows, &cols);
>> +      if (gdb_stdout->isatty ()) {
>> +    readline_hidden_cols = COLS - cols;
>> +    gdb_assert (readline_hidden_cols >= 0);
>> +    gdb_assert (readline_hidden_cols <= 1);
>> +      }
>>         lines_per_page = rows;
>>         chars_per_line = cols;
> 
> Reading this over once more, I noticed this is missing a fix for an 
> assertion failure we run into when doing:
> ...
> $ TERM=blabla gdb
> ...
> 
> So, this bit is missing:
> ...
> -      if (gdb_stdout->isatty ()) {
> +      if (gdb_stdout->isatty () && COLS != 0) {
> ...
> 

I also ran into trouble with this version.  It's not a good idea to use 
COLS, because it's just a copy of the env variable COLUMNS at the time 
of curses startup.  So, I can trigger one of the asserts by doing:
...
$ COLUMNS=<some-n> gdb
...

Here's an updated version, using the COLUMNS value as written by 
readline instead.

Thanks,
- Tom
  

On 4/14/23 13:02, Tom de Vries via Gdb-patches wrote:
> On 4/13/23 16:17, Tom de Vries via Gdb-patches wrote:
>> On 4/13/23 16:08, Tom de Vries via Gdb-patches wrote:
>>> @@ -1144,6 +1148,11 @@ init_page_info (void)
>>>         /* Get the screen size from Readline.  */
>>>         rl_get_screen_size (&rows, &cols);
>>> +      if (gdb_stdout->isatty ()) {
>>> +    readline_hidden_cols = COLS - cols;
>>> +    gdb_assert (readline_hidden_cols >= 0);
>>> +    gdb_assert (readline_hidden_cols <= 1);
>>> +      }
>>>         lines_per_page = rows;
>>>         chars_per_line = cols;
>>
>> Reading this over once more, I noticed this is missing a fix for an 
>> assertion failure we run into when doing:
>> ...
>> $ TERM=blabla gdb
>> ...
>>
>> So, this bit is missing:
>> ...
>> -      if (gdb_stdout->isatty ()) {
>> +      if (gdb_stdout->isatty () && COLS != 0) {
>> ...
>>
> 
> I also ran into trouble with this version.  It's not a good idea to use 
> COLS, because it's just a copy of the env variable COLUMNS at the time 
> of curses startup.  So, I can trigger one of the asserts by doing:
> ...
> $ COLUMNS=<some-n> gdb
> ...
> 
> Here's an updated version, using the COLUMNS value as written by 
> readline instead.


> @@ -1144,6 +1148,24 @@ 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).
> +	 Set readline_hidden_cols by comparing COLUMNS to cols as returned by
> +	 rl_get_screen_size.  */
> +      char *columns_env = getenv ("COLUMNS");
> +      gdb_assert (columns_env != nullptr);
> +      int columns_env_val = atoi (columns_env);
> +      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;
>  

Hmm, it seems there is precedent for using private functions and 
variables of readline, the ones advertised as "functions and variables 
global to the readline library, but not intended for use by applications":
...
$ find gdb* -type f | grep -v ChangeLog | xargs grep -i 
"extern.*[^a-zA-Z]_rl_"
gdb/tui/tui-io.c:  extern int _rl_echoing_p;
gdb/completer.c:  extern int _rl_complete_mark_directories;
gdb/completer.c:extern int _rl_completion_prefix_display_length;
gdb/completer.c:extern int _rl_print_completions_horizontally;
gdb/completer.c:EXTERN_C int _rl_qsort_string_compare (const void *, 
const void *);
gdb/cli-out.c:EXTERN_C void _rl_erase_entire_line (void);
...

So, this patch could be simplified by just using _rl_term_autowrap.

Thanks,
- Tom
  

On 4/18/23 08:10, Tom de Vries via Gdb-patches wrote:
> So, this patch could be simplified by just using _rl_term_autowrap.

This version uses _rl_term_autowrap.

Thanks,
- Tom
  

On 4/25/23 09:09, Tom de Vries via Gdb-patches wrote:
> On 4/18/23 08:10, Tom de Vries via Gdb-patches wrote:
>> So, this patch could be simplified by just using _rl_term_autowrap.
> 
> This version uses _rl_term_autowrap.

This ( 
https://sourceware.org/pipermail/gdb-patches/2023-April/199229.html ) is 
the version I've ended up pushing.

It propagates the same change to tui_async_resize_screen, and adds a 
test-case for that.

Thanks,
- Tom