Re: [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi

[Tom de Vries <tdevries@suse.de>]

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