* [PATCH 1/8] [gdb/testsuite] Add warning for timeout in accept_gdb_output
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 2/8] [gdb/testsuite] Add debug prints in Term::wait_for Tom de Vries
` (7 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
In accept_gdb_output we have:
...
timeout {
# Assume a timeout means we somehow missed the
# expected result, and carry on.
return 0
}
...
The timeout is silent, and though in some places the return value is checked,
this is not done consistently, and consequently there are silent timeouts
when running the TUI testsuite (gdb.tui/*.exp and gdb.python/tui*.exp).
Each timeout is 10 seconds, and there are 5 in total in the TUI tests, taking
50 seconds overall:
...
real 1m0.275s
user 0m10.440s
sys 0m1.343s
...
With an entire testsuite run taking about 30 minutes, that is about 2.5% of
the time spent waiting in TUI tests.
Let's make the timeouts visible using a warning, such that they can be fixed.
Tested on x86_64-linux.
---
gdb/testsuite/lib/tuiterm.exp | 2 ++
1 file changed, 2 insertions(+)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index 05edfe9a5b1..ff38af082da 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -707,6 +707,8 @@ namespace eval Term {
timeout {
# Assume a timeout means we somehow missed the
# expected result, and carry on.
+ warning "timeout in accept_gdb_output"
+ dump_screen
return 0
}
}
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 2/8] [gdb/testsuite] Add debug prints in Term::wait_for
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
2023-04-13 14:08 ` [PATCH 1/8] [gdb/testsuite] Add warning for timeout in accept_gdb_output Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 3/8] [gdb/testsuite] Fix timeout in gdb.tui/corefile-run.exp Tom de Vries
` (6 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
The semantics of wait_for are non-trivial, and a bit hard to understand
sometimes.
Add some debug prints in wait_for that make it clear:
- what regexps we're trying to match,
- what strings we compare to the regexps, and
- whether there's a match or mismatch.
I've added this ad-hoc a couple of times, and it seems that it's worth having
readily available.
The debug prints are enabled by adding DEBUG_TUI_MATCHING=1 to the
RUNTESTFLAGS:
...
$ make check RUNTESTFLAGS="gdb.tui/empty.exp DEBUG_TUI_MATCHING=1"
...
Tested on x86_64-linux.
---
gdb/testsuite/lib/tuiterm.exp | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index ff38af082da..bb462911046 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -716,6 +716,20 @@ namespace eval Term {
return 1
}
+ # Print arg using "verbose -log" if DEBUG_TUI_MATCHING == 1.
+ proc debug_tui_matching { arg } {
+ set debug 0
+ if { [info exists ::DEBUG_TUI_MATCHING] } {
+ set debug $::DEBUG_TUI_MATCHING
+ }
+
+ if { ! $debug } {
+ return
+ }
+
+ verbose -log "$arg"
+ }
+
# Accept some output from gdb and update the screen. WAIT_FOR is
# a regexp matching the line to wait for. Return 0 on timeout, 1
# on success.
@@ -724,7 +738,10 @@ namespace eval Term {
variable _cur_col
variable _cur_row
+ set fn "wait_for"
+
set prompt_wait_for "$gdb_prompt \$"
+ debug_tui_matching "$fn: regexp: '$wait_for'"
while 1 {
if { [accept_gdb_output] == 0 } {
@@ -740,10 +757,14 @@ namespace eval Term {
set prev [get_line $_cur_row]
}
if {[regexp -- $wait_for $prev]} {
+ debug_tui_matching "$fn: match: '$prev'"
if {$wait_for == "$prompt_wait_for"} {
break
}
set wait_for $prompt_wait_for
+ debug_tui_matching "$fn: regexp prompt: '$wait_for'"
+ } else {
+ debug_tui_matching "$fn: mismatch: '$prev'"
}
}
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 3/8] [gdb/testsuite] Fix timeout in gdb.tui/corefile-run.exp
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
2023-04-13 14:08 ` [PATCH 1/8] [gdb/testsuite] Add warning for timeout in accept_gdb_output Tom de Vries
2023-04-13 14:08 ` [PATCH 2/8] [gdb/testsuite] Add debug prints in Term::wait_for Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 4/8] [gdb/testsuite] Fix timeout in gdb.tui/main.exp Tom de Vries
` (5 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
With test-case gdb.tui/corefile-run.exp we run into:
...
WARNING: timeout in accept_gdb_output
PASS: gdb.tui/corefile-run.exp: load corefile
...
The timeout happens in this command:
...
Term::command "core-file $core"
...
because it tries to match "(gdb) $cmd" but $cmd is split over two lines:
...
16 (gdb) core-file /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.tui/co
17 refile-run/corefile-run.core
18 [New LWP 5370]
19 Core was generated by `/data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb
20 .tui/corefile-run/coref'.
21 Program terminated with signal SIGTRAP, Trace/breakpoint trap.
22 #0 main () at tui-layout.c:21
23 (gdb)
...
Fix this by using send_gdb "$cmd\n" and wait_for "Program terminated" instead.
Tested on x86_64-linux.
---
gdb/testsuite/gdb.tui/corefile-run.exp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gdb/testsuite/gdb.tui/corefile-run.exp b/gdb/testsuite/gdb.tui/corefile-run.exp
index 6769dfbd800..a81d7e7c208 100644
--- a/gdb/testsuite/gdb.tui/corefile-run.exp
+++ b/gdb/testsuite/gdb.tui/corefile-run.exp
@@ -63,7 +63,8 @@ set text [Term::get_all_lines]
gdb_assert {![string match "No Source Available" $text]} \
"initial source listing"
-Term::command "core-file $core"
+send_gdb "core-file $core\n"
+Term::wait_for "Program terminated"
Term::check_contents "load corefile" "$src_line_nr *$src_line.*$gdb_prompt .*"
Term::command "run"
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 4/8] [gdb/testsuite] Fix timeout in gdb.tui/main.exp
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
` (2 preceding siblings ...)
2023-04-13 14:08 ` [PATCH 3/8] [gdb/testsuite] Fix timeout in gdb.tui/corefile-run.exp Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 5/8] [gdb/testsuite] Fix timeout in gdb.tui/new-layout.exp Tom de Vries
` (4 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
With test-case gdb.tui/main.exp we run into:
...
WARNING: timeout in accept_gdb_output
PASS: gdb.tui/main.exp: show main after file
...
The problem is that this command:
...
Term::command "file [standard_output_file $testfile]"
...
tries to match "(gdb) $cmd", but due to the long file name, $cmd is split up
over two lines:
...
16 (gdb) file /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.tui/main/ma
17 in
18 Reading symbols from /data/vries/gdb/leap-15-4/build/gdb/testsuite/outputs/gdb.t
19 ui/main/main...
20 (gdb)
...
Fix this by matching "Reading symbols from" instead.
Tested on x86_64-linux.
---
gdb/testsuite/gdb.tui/main.exp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gdb/testsuite/gdb.tui/main.exp b/gdb/testsuite/gdb.tui/main.exp
index e5f26c7aac3..a7cacf0a8b8 100644
--- a/gdb/testsuite/gdb.tui/main.exp
+++ b/gdb/testsuite/gdb.tui/main.exp
@@ -34,7 +34,8 @@ if {![Term::enter_tui]} {
return
}
-Term::command "file [standard_output_file $testfile]"
+send_gdb "file [standard_output_file $testfile]\n"
+gdb_assert { [Term::wait_for "Reading symbols from"] } "file command"
Term::check_contents "show main after file" "\\|.*21 *return 0"
# Ensure that "file" clears the source window.
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 5/8] [gdb/testsuite] Fix timeout in gdb.tui/new-layout.exp
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
` (3 preceding siblings ...)
2023-04-13 14:08 ` [PATCH 4/8] [gdb/testsuite] Fix timeout in gdb.tui/main.exp Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 6/8] [gdb/testsuite] Fix timeout in gdb.tui/completion.exp Tom de Vries
` (3 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
In test-case gdb.tui/new-layout.exp we run into:
...
WARNING: timeout in accept_gdb_output
PASS: gdb.tui/new-layout.exp: layout=cmd_only {cmd 1} {} {}: \
bottom of cmd window is blank
...
The timeout happens here:
...
Term::command "layout src"
...
Before the "layout src" command we have:
...
Screen Dump (size 80 columns x 24 rows, cursor at column 46, row 7):
0 +-tui-layout.c-------------------------+(gdb) layout example3
1 | 20 { |(gdb) layout src
2 | 21 return 0; |(gdb) winheight cmd 8
3 | 22 } |(gdb) layout example4
4 | 23 |(gdb) layout src
5 | 24 |(gdb) winheight cmd 8
6 | 25 |(gdb) layout example5
7 | 26 |(gdb)
8 | 27 |
9 | 28 |
10 | 29 |
11 | 30 |
12 | 31 |
13 | 32 |
14 | 33 |
15 | 34 |
16 | 35 |
17 | 36 |
18 | 37 |
19 | 38 |
20 | 39 |
21 | 40 |
22 +--------------------------------------+
23 exec No process In: L?? PC: ??
...
and after:
...
Screen Dump (size 80 columns x 24 rows, cursor at column 6, row 16):
0 +-tui-layout.c-----------------------------------------------------------------+
1 | 20 { |
2 | 21 return 0; |
3 | 22 } |
4 | 23 |
5 | 24 |
6 | 25 |
7 | 26 |
8 | 27 |
9 | 28 |
10 | 29 |
11 | 30 |
12 | 31 |
13 | 32 |
14 +------------------------------------------------------------------------------+
15 exec No process In: L?? PC: ??
16 (gdb)
17
18
19
20
21
22
23
...
The Term::command "layout src" is waiting to match:
- "(gdb) layout src", and then
- "(gdb) ".
The first part fails to match on a line:
...
| 26 |(gdb) layout src
...
because it expects the prompt at the start of the line.
Fix this by allowing the prompt at the start of a window as well.
Tested by x86_64-linux.
---
gdb/testsuite/lib/tuiterm.exp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index bb462911046..fa703fc060e 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -850,7 +850,7 @@ namespace eval Term {
global gdb_prompt
send_gdb "$cmd\n"
set str [string_to_regexp $cmd]
- set str "^$gdb_prompt $str"
+ set str "(^|\|)$gdb_prompt $str"
wait_for $str
}
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 6/8] [gdb/testsuite] Fix timeout in gdb.tui/completion.exp
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
` (4 preceding siblings ...)
2023-04-13 14:08 ` [PATCH 5/8] [gdb/testsuite] Fix timeout in gdb.tui/new-layout.exp Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 7/8] [gdb/testsuite] Fix timeout in gdb.tui/empty.exp Tom de Vries
` (2 subsequent siblings)
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
With test-case gdb.tui/completion.exp, we run into:
...
WARNING: timeout in accept_gdb_output
PASS: gdb.tui/completion.exp: check focus completions
...
The timeout happens in this command:
...
Term::command "layout src"
...
which waits for:
- "(gdb) layout src", and then
- "(gdb) ".
Because the "layout src" command enables the TUI there's just a prompt.
Fix this by using Term::command_no_prompt_prefix.
Tested on x86_64-linux.
---
gdb/testsuite/gdb.tui/completion.exp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdb/testsuite/gdb.tui/completion.exp b/gdb/testsuite/gdb.tui/completion.exp
index 85abd4a958b..9cf8dc2ee25 100644
--- a/gdb/testsuite/gdb.tui/completion.exp
+++ b/gdb/testsuite/gdb.tui/completion.exp
@@ -66,7 +66,7 @@ if {![Term::prepare_for_tui]} {
return
}
-Term::command "layout src"
+Term::command_no_prompt_prefix "layout src"
Term::command "complete focus "
Term::dump_screen
Term::check_region_contents "check focus completions" 0 17 80 5 \
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 7/8] [gdb/testsuite] Fix timeout in gdb.tui/empty.exp
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
` (5 preceding siblings ...)
2023-04-13 14:08 ` [PATCH 6/8] [gdb/testsuite] Fix timeout in gdb.tui/completion.exp Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:08 ` [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi Tom de Vries
2023-04-25 6:35 ` [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
In test-case gdb.tui/empty.exp we run into:
...
WARNING: timeout in accept_gdb_output
PASS: gdb.tui/empty.exp: src: 90x40: box 1
...
We timeout here in Term::resize:
...
# Due to the strange column resizing behavior, and because we
# don't care about this intermediate resize, we don't check
# the size here.
wait_for "@@ resize done $_resize_count"
...
because the string we're trying to match is split over two lines:
...
25 -----------------------------------------------------------------------------+No
26 ne No process In: L?? PC: ?? @@
27 resize done 0, size = 79x40
28 (gdb)
...
Fix this by dropping the "@@ " prefix.
Tested on x86_64-linux.
---
gdb/testsuite/lib/tuiterm.exp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index fa703fc060e..d8a99d2798a 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -1124,8 +1124,8 @@ namespace eval Term {
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 here.
- wait_for "@@ resize done $_resize_count"
+ # the size and the "@@ " prefix here.
+ wait_for "resize done $_resize_count"
incr _resize_count
# Somehow the number of columns transmitted to gdb is one less
# than what we request from expect. We hide this weird
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
` (6 preceding siblings ...)
2023-04-13 14:08 ` [PATCH 7/8] [gdb/testsuite] Fix timeout in gdb.tui/empty.exp Tom de Vries
@ 2023-04-13 14:08 ` Tom de Vries
2023-04-13 14:17 ` Tom de Vries
2023-04-25 6:35 ` [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
8 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:08 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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(-)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index d8a99d2798a..f59a66a0958 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -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
}
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 3b17cb8dd29..7186fb97d68 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)
diff --git a/gdb/utils.c b/gdb/utils.c
index 6ec1cc0d48d..05a2b27bc65 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1116,6 +1116,10 @@ static bool filter_initialized = false;
\f
+/* 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;
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 */
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi
2023-04-13 14:08 ` [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi Tom de Vries
@ 2023-04-13 14:17 ` Tom de Vries
2023-04-14 11:02 ` Tom de Vries
0 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2023-04-13 14:17 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi
2023-04-13 14:17 ` Tom de Vries
@ 2023-04-14 11:02 ` Tom de Vries
2023-04-18 6:10 ` Tom de Vries
0 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2023-04-14 11:02 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]
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
[-- Attachment #2: 0008-gdb-tui-Fix-TUI-for-TERM-ansi.patch --]
[-- Type: text/x-patch, Size: 5598 bytes --]
From 1c58c8e9bf9fdf593593fb404e06db506685475d Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 12 Apr 2023 14:44:42 +0200
Subject: [PATCH 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 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.
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.
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 | 22 ++++++++++++++++++++++
gdb/utils.h | 7 +++++++
4 files changed, 34 insertions(+), 8 deletions(-)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index d8a99d2798a..f59a66a0958 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -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
}
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 3b17cb8dd29..7186fb97d68 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)
diff --git a/gdb/utils.c b/gdb/utils.c
index 6ec1cc0d48d..82138a1fc2c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1116,6 +1116,10 @@ static bool filter_initialized = false;
\f
+/* See utils.h. */
+
+int readline_hidden_cols = 0;
+
/* Initialize the number of lines per page and chars per line. */
void
@@ -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;
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 */
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi
2023-04-14 11:02 ` Tom de Vries
@ 2023-04-18 6:10 ` Tom de Vries
2023-04-25 7:09 ` Tom de Vries
0 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2023-04-18 6:10 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi
2023-04-18 6:10 ` Tom de Vries
@ 2023-04-25 7:09 ` Tom de Vries
2023-04-30 11:08 ` Tom de Vries
0 siblings, 1 reply; 15+ messages in thread
From: Tom de Vries @ 2023-04-25 7:09 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
[-- Attachment #1: Type: text/plain, Size: 178 bytes --]
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
[-- Attachment #2: 0001-gdb-tui-Fix-TUI-for-TERM-ansi.patch --]
[-- Type: text/x-patch, Size: 5502 bytes --]
From 706916401057521955b9f74bb4079e2619c52039 Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 12 Apr 2023 14:44:42 +0200
Subject: [PATCH] [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 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.
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.
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 | 21 +++++++++++++++++++++
gdb/utils.h | 7 +++++++
4 files changed, 33 insertions(+), 8 deletions(-)
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index d8a99d2798a..f59a66a0958 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -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
}
diff --git a/gdb/tui/tui-win.c b/gdb/tui/tui-win.c
index 3b17cb8dd29..7186fb97d68 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)
diff --git a/gdb/utils.c b/gdb/utils.c
index b5bb84ce85d..282c4ca70b9 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: 538edc49dc610b987f8929434f883c8bbc211be8
--
2.35.3
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests
2023-04-13 14:08 [PATCH 0/8] [gdb/testsuite] Fix timeouts in TUI tests Tom de Vries
` (7 preceding siblings ...)
2023-04-13 14:08 ` [PATCH 8/8] [gdb/tui] Fix TUI for TERM=ansi Tom de Vries
@ 2023-04-25 6:35 ` Tom de Vries
8 siblings, 0 replies; 15+ messages in thread
From: Tom de Vries @ 2023-04-25 6:35 UTC (permalink / raw)
To: gdb-patches; +Cc: Tom Tromey
On 4/13/23 16:08, Tom de Vries via Gdb-patches wrote:
> [gdb/testsuite] Add warning for timeout in accept_gdb_output
> [gdb/testsuite] Add debug prints in Term::wait_for
> [gdb/testsuite] Fix timeout in gdb.tui/corefile-run.exp
> [gdb/testsuite] Fix timeout in gdb.tui/main.exp
> [gdb/testsuite] Fix timeout in gdb.tui/new-layout.exp
> [gdb/testsuite] Fix timeout in gdb.tui/completion.exp
> [gdb/testsuite] Fix timeout in gdb.tui/empty.exp
> [gdb/tui] Fix TUI for TERM=ansi
I've committed all but the last, I'll do resubmission of that one.
Thanks,
- Tom
^ permalink raw reply [flat|nested] 15+ messages in thread