[8/8,gdb/tui] Fix TUI for TERM=ansi
Commit Message
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(-)
Comments
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
@@ -1122,16 +1122,10 @@ 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"
+ 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
+ stty columns $_cols < $::gdb_tty_name
wait_for "@@ resize done $_resize_count, size = ${_cols}x${rows}"
incr _resize_count
}
@@ -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)
@@ -1116,6 +1116,10 @@ static bool filter_initialized = false;
+/* See utils.h. */
+
+int readline_hidden_cols = 0;
+
/* Initialize the number of lines per page and chars per line. */
void
@@ -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;
@@ -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 */