[review,v2] Make TUI resizing tests more robust

Message ID 20191106142740.07A9325B28@gnutoolchain-gerrit.osci.io
State Superseded
Headers show
Series
  • [review,v2] Make TUI resizing tests more robust
Related show

Commit Message

Sourceware to Gerrit sync (Code Review) Nov. 6, 2019, 2:27 p.m.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/361
......................................................................

Make TUI resizing tests more robust

As Sergio pointed out, the TUI resizing tests are flaky.  Debugging
this showed three main problems.

1. expect's "stty" command processes its arguments one-by-one.  So,
rather than requesting a single resize, it sends two separate resize
requests (one for rows and one for columns).  This means gdb sees two
SIGWINCH signals and resizes the terminal twice.

I consider this a bug in expect, but I couldn't readily see how to
report a bug; and anyway the fix wouldn't propagate very quickly.

This patch works around this problem by explicitly doing two separate
resizes (so it will be robust if expect ever does change); and then by
waiting for each resize to complete before continuing.

2. gdb uses curses to drive the console rendering.  Currently the test
suite looks for terminal text insertion sequences to decide when a
command has completed.  However, it turns out that, sometimes, curses
can output things in non-obvious ways.  I didn't debug into curses but
I guess this can happen due to output optimizations.  No matter the
reason, sometimes the current approach of only tracking text
insertions is not enough to detect that gdb has finished rendering.

This patch fixes this problem by arranging to detect the termination
output after any curses command, not just insertion.

3. Detecting when a resize has completed is tricky.  In fact, I could
not find a way to reliably do this.

This patch fixes this problem by adding a special maint
"tui-resize-message" setting to gdb.  When this is enabled, gdb will
print a message after each SIGWINCH has been fully processed.  The
test suite enables this mode and then waits for the message in order
to know when control can be returned to the calling test.

This patch also adds a timeout, to avoid the situation where the
terminal code fails to notice a change for some reason.  This lets the
test at least try to continue.

gdb/ChangeLog
2019-11-06  Tom Tromey  <tom@tromey.com>

	* tui/tui-win.c (resize_message): New global.
	(show_tui_resize_message): New function.
	(tui_async_resize_screen): Print message if requested.
	(_initialize_tui_win): Add tui-resize-message setting.
	* NEWS: Add entry for new commands.

gdb/doc/ChangeLog
2019-11-06  Tom Tromey  <tom@tromey.com>

	* gdb.texinfo (Maintenance Commands): Document new command.

gdb/testsuite/ChangeLog
2019-11-06  Tom Tromey  <tom@tromey.com>

	* lib/tuiterm.exp (_accept): Add wait_for parameter.  Check output
	after any command.  Expect prompt after WAIT_FOR is seen.
	(enter_tui): Enable resize messages.
	(command): Expect command in output.
	(get_line): Avoid error when cursor appears to be off-screen.
	(dump_screen): Include screen size in title.
	(_do_resize): New proc, from "resize".
	(resize): Rewrite.  Do resize in two steps.
	* gdb.tui/empty.exp (layouts): Fix entries.
	(check_boxes): Remove xfail.
	(check_text): Dump screen on failure.

Change-Id: I420e0259cb99b21adcd28f671b99161eefa7a51d
---
M gdb/ChangeLog
M gdb/NEWS
M gdb/doc/ChangeLog
M gdb/doc/gdb.texinfo
M gdb/testsuite/ChangeLog
M gdb/testsuite/gdb.tui/empty.exp
M gdb/testsuite/lib/tuiterm.exp
M gdb/tui/tui-win.c
8 files changed, 176 insertions(+), 48 deletions(-)




-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I420e0259cb99b21adcd28f671b99161eefa7a51d
Gerrit-Change-Number: 361
Gerrit-PatchSet: 2
Gerrit-Owner: Tom Tromey <tromey@sourceware.org>
Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
Gerrit-MessageType: newpatchset

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index e886480..d23bd57 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@ 
+2019-11-06  Tom Tromey  <tom@tromey.com>
+
+	* tui/tui-win.c (resize_message): New global.
+	(show_tui_resize_message): New function.
+	(tui_async_resize_screen): Print message if requested.
+	(_initialize_tui_win): Add tui-resize-message setting.
+	* NEWS: Add entry for new commands.
+
 2019-11-05  Tom Tromey  <tom@tromey.com>
 
 	* tui/tui-disasm.c (struct tui_asm_line) <addr_size>: New member.
diff --git a/gdb/NEWS b/gdb/NEWS
index dc63179..2efeb4f 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -151,6 +151,12 @@ 
   A set of commands used by the testsuite for exercising the settings
   infrastructure.
 
+maint set tui-resize-message [on|off]
+maint show tui-resize-message
+  Control whether GDB prints a message each time the terminal is
+  resized when in TUI mode.  This is primarily useful for testing the
+  TUI.
+
 set print frame-info [short-location|location|location-and-address
                         |source-and-location|source-line|auto]
 show print frame-info
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index ce89ee4..83be997 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@ 
+2019-11-06  Tom Tromey  <tom@tromey.com>
+
+	* gdb.texinfo (Maintenance Commands): Document new command.
+
 2019-10-31  Andrew Burgess  <andrew.burgess@embecosm.com>
 
 	* gdb.texinfo (Symbols): Document new 'info module variables' and
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 70e4be1..8e47a45 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -37911,6 +37911,19 @@ 
 target supports it.
 @end table
 
+@kindex maint set tui-resize-message
+@kindex maint show tui-resize-message
+@item maint set tui-resize-message
+@item maint show tui-resize-message
+Control whether @value{GDBN} displays a message each time the terminal
+is resized when in TUI mode.  The default is @code{off}, which means
+that @value{GDBN} is silent during resizes.  When @code{on},
+@value{GDBN} will display a message after a resize is completed; the
+message will include a number indicating how many times the terminal
+has been resized.  This setting is intended for use by the test suite,
+where it would otherwise be difficult to determine when a resize and
+refresh has been completed.
+
 @kindex maint set per-command
 @kindex maint show per-command
 @item maint set per-command
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 9b7bcf6..54e00a1 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,17 @@ 
+2019-11-06  Tom Tromey  <tom@tromey.com>
+
+	* lib/tuiterm.exp (_accept): Add wait_for parameter.  Check output
+	after any command.  Expect prompt after WAIT_FOR is seen.
+	(enter_tui): Enable resize messages.
+	(command): Expect command in output.
+	(get_line): Avoid error when cursor appears to be off-screen.
+	(dump_screen): Include screen size in title.
+	(_do_resize): New proc, from "resize".
+	(resize): Rewrite.  Do resize in two steps.
+	* gdb.tui/empty.exp (layouts): Fix entries.
+	(check_boxes): Remove xfail.
+	(check_text): Dump screen on failure.
+
 2019-11-02  Tom de Vries  <tdevries@suse.de>
 
 	* gdb.base/advance.exp: Drop superfluous 3rd argument to gdb_test.
diff --git a/gdb/testsuite/gdb.tui/empty.exp b/gdb/testsuite/gdb.tui/empty.exp
index 0918522..b6ee350 100644
--- a/gdb/testsuite/gdb.tui/empty.exp
+++ b/gdb/testsuite/gdb.tui/empty.exp
@@ -34,24 +34,26 @@ 
 set layouts {
     {src src {{0 0 80 15}} {{0 0 90 23}}
 	{{"no source" "No Source Available"}}}
-    {regs src-regs {{0 0 80 8} {0 7 80 8}} {{0 0 90 13} {0 13 90 13}}
+    {regs src-regs {{0 0 80 8} {0 7 80 8}} {{0 0 90 13} {0 12 90 13}}
 	{
 	    {"no source" "No Source Available"}
 	    {"no regs" "Register Values Unavailable"}
 	}}
     {asm asm {{0 0 80 15}} {{0 0 90 23}}
-	{"no asm" "No Assembly Available"}}
-    {regs asm-regs {{0 0 80 8} {0 7 80 9}} {{0 0 90 13} {0 13 90 14}}
+	{
+	    {"no asm" "No Assembly Available"}
+	}}
+    {regs asm-regs {{0 0 80 8} {0 7 80 8}} {{0 0 90 13} {0 12 90 13}}
 	{
 	    {"no asm" "No Assembly Available"}
 	    {"no regs" "Register Values Unavailable"}
 	}}
-    {split split {{0 0 80 8} {0 7 80 9}} {{0 0 90 13} {0 13 90 14}}
+    {split split {{0 0 80 8} {0 7 80 8}} {{0 0 90 13} {0 12 90 13}}
 	{
 	    {"no source" "No Source Available"}
 	    {"no asm" "No Assembly Available"}
 	}}
-    {regs split-regs {{0 0 80 8} {0 7 80 9}} {{0 0 90 13} {0 13 90 14}}
+    {regs split-regs {{0 0 80 8} {0 7 80 8}} {{0 0 90 13} {0 12 90 13}}
 	{
 	    {"no asm" "No Assembly Available"}
 	    {"no regs" "Register Values Unavailable"}
@@ -62,11 +64,6 @@ 
 proc check_boxes {boxes} {
     set boxno 1
     foreach box $boxes {
-	if {$boxno > 1} {
-	    # The upper-left corner of the second box may not render
-	    # properly, due to overlap.
-	    setup_xfail *-*-*
-	}
 	eval Term::check_box [list "box $boxno"] $box
 	incr boxno
     }
@@ -77,7 +74,9 @@ 
     set text [Term::get_all_lines]
     foreach item $text_list {
 	lassign $item testname check
-	gdb_assert {[string first $check $text]} $testname
+	if {![gdb_assert {[regexp -- $check $text]} $testname]} {
+	    Term::dump_screen
+	}
     }
 }
 
diff --git a/gdb/testsuite/lib/tuiterm.exp b/gdb/testsuite/lib/tuiterm.exp
index c6a938e..dcba028 100644
--- a/gdb/testsuite/lib/tuiterm.exp
+++ b/gdb/testsuite/lib/tuiterm.exp
@@ -40,6 +40,8 @@ 
 
     variable _last_char
 
+    variable _resize_count
+
     # If ARG is empty, return DEF: otherwise ARG.  This is useful for
     # defaulting arguments in CSIs.
     proc _default {arg def} {
@@ -368,11 +370,13 @@ 
 	variable _cur_x
 	variable _cur_y
 	variable _attrs
+	variable _resize_count
 
 	set _rows $rows
 	set _cols $cols
 	set _cur_x 0
 	set _cur_y 0
+	set _resize_count 0
 	array set _attrs {
 	    intensity normal
 	    fg default
@@ -385,39 +389,58 @@ 
     }
 
     # Accept some output from gdb and update the screen.
-    proc _accept {} {
+    proc _accept {wait_for} {
 	global expect_out
-	gdb_expect {
-	    -re "^\[\x07\x08\x0a\x0d\]" {
-		scan $expect_out(0,string) %c val
-		set hexval [format "%02x" $val]
-		verbose "+++ _ctl_0x${hexval}"
-		_ctl_0x${hexval}
-		exp_continue
-	    }
-	    -re "^\x1b(\[0-9a-zA-Z\])" {
-		verbose "+++ unsupported escape"
-		error "unsupported escape"
-	    }
-	    -re "^\x1b\\\[(\[0-9;\]*)(\[a-zA-Z@\])" {
-		set cmd $expect_out(2,string)
-		set params [split $expect_out(1,string) ";"]
-		verbose "+++ _csi_$cmd <<<$expect_out(1,string)>>>"
-		eval _csi_$cmd $params
-		exp_continue
-	    }
-	    -re "^\[^\x07\x08\x0a\x0d\x1b\]+" {
-		_insert $expect_out(0,string)
-		variable _last_char
-		set _last_char [string index $expect_out(0,string) end]
-		# If the prompt was just inserted, return.
-		variable _cur_x
-		variable _cur_y
-		global gdb_prompt
-		set prev [get_line $_cur_y $_cur_x]
-		if {![regexp -- "$gdb_prompt \$" $prev]} {
-		    exp_continue
+	global gdb_prompt
+	variable _cur_x
+	variable _cur_y
+
+	set prompt_wait_for "$gdb_prompt \$"
+
+	while 1 {
+	    gdb_expect {
+		-re "^\[\x07\x08\x0a\x0d\]" {
+		    scan $expect_out(0,string) %c val
+		    set hexval [format "%02x" $val]
+		    verbose "+++ _ctl_0x${hexval}"
+		    _ctl_0x${hexval}
 		}
+		-re "^\x1b(\[0-9a-zA-Z\])" {
+		    verbose "+++ unsupported escape"
+		    error "unsupported escape"
+		}
+		-re "^\x1b\\\[(\[0-9;\]*)(\[a-zA-Z@\])" {
+		    set cmd $expect_out(2,string)
+		    set params [split $expect_out(1,string) ";"]
+		    verbose "+++ _csi_$cmd <<<$expect_out(1,string)>>>"
+		    eval _csi_$cmd $params
+		}
+		-re "^\[^\x07\x08\x0a\x0d\x1b\]+" {
+		    _insert $expect_out(0,string)
+		    variable _last_char
+		    set _last_char [string index $expect_out(0,string) end]
+		}
+
+		timeout {
+		    # Assume a timeout means we somehow missed the
+		    # expected result, and carry on.
+		    return
+		}
+	    }
+
+	    # If the cursor appears just after the prompt, return.  It
+	    # isn't reliable to check this only after an insertion,
+	    # because curses may make "unusual" redrawing decisions.
+	    if {$wait_for == "$prompt_wait_for"} {
+		set prev [get_line $_cur_y $_cur_x]
+	    } else {
+		set prev [get_line $_cur_y]
+	    }
+	    if {[regexp -- $wait_for $prev]} {
+		if {$wait_for == "$prompt_wait_for"} {
+		    break
+		}
+		set wait_for $prompt_wait_for
 	    }
 	}
     }
@@ -447,6 +470,7 @@ 
 	}
 
 	gdb_test_no_output "set tui border-kind ascii"
+	gdb_test_no_output "maint set tui-resize-message on"
 	command "tui enable"
 	return 1
     }
@@ -456,13 +480,20 @@ 
     # be supplied by this function.
     proc command {cmd} {
 	send_gdb "$cmd\n"
-	_accept
+	_accept [string_to_regexp $cmd]
     }
 
     # Return the text of screen line N, without attributes.  Lines are
     # 0-based.  If C is given, stop before column C.  Columns are also
     # zero-based.
     proc get_line {n {c ""}} {
+	variable _rows
+	# This can happen during resizing, if the cursor seems to
+	# temporarily be off-screen.
+	if {$n >= $_rows} {
+	    return ""
+	}
+
 	set result ""
 	variable _cols
 	variable _chars
@@ -567,7 +598,8 @@ 
     # numbers.
     proc dump_screen {} {
 	variable _rows
-	verbose "Screen Dump:"
+	variable _cols
+	verbose "Screen Dump ($_cols x $_rows):"
 	for {set y 0} {$y < $_rows} {incr y} {
 	    set fmt [format %5d $y]
 	    verbose "$fmt [get_line $y]"
@@ -575,7 +607,7 @@ 
     }
 
     # Resize the terminal.
-    proc resize {rows cols} {
+    proc _do_resize {rows cols} {
 	variable _chars
 	variable _rows
 	variable _cols
@@ -596,13 +628,31 @@ 
 		set _chars($x,$y) $local_chars($x,$y)
 	    }
 	}
+    }
+
+    proc resize {rows cols} {
+	variable _rows
+	variable _cols
+	variable _resize_count
 
 	global gdb_spawn_name
+	# expect handles each argument to stty separately.  This means
+	# that gdb will see SIGWINCH twice.  Rather than rely on this
+	# behavior (which, after all, could be changed), we make it
+	# explicit here.  This also simplifies waiting for the redraw.
+	_do_resize $rows $_cols
+	stty rows $_rows < $gdb_spawn_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.
+	_accept "@@ 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
 	# details from the caller.
-	stty rows $_rows columns [expr {$_cols + 1}] \
-	    < $gdb_spawn_name
-	_accept
+	_do_resize $_rows $cols
+	stty columns [expr {$_cols + 1}] < $gdb_spawn_name
+	_accept "@@ 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 6df5ea2..feeee34 100644
--- a/gdb/tui/tui-win.c
+++ b/gdb/tui/tui-win.c
@@ -33,6 +33,7 @@ 
 #include "top.h"
 #include "source.h"
 #include "event-loop.h"
+#include "gdbcmd.h"
 
 #include "tui/tui.h"
 #include "tui/tui-io.h"
@@ -340,6 +341,22 @@ 
     tui_rehighlight_all ();
 }
 
+
+
+/* True if TUI resizes should print a message.  This is used by the
+   test suite.  */
+
+static bool resize_message;
+
+static void
+show_tui_resize_message (struct ui_file *file, int from_tty,
+			 struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("TUI resize messaging is %s.\n"), value);
+}
+
+
+
 /* Generic window name completion function.  Complete window name pointed
    to by TEXT and WORD.  If INCLUDE_NEXT_PREV_P is true then the special
    window names 'next' and 'prev' will also be considered as possible
@@ -677,6 +694,13 @@ 
       tui_resize_all ();
       tui_refresh_all_win ();
       tui_update_gdb_sizes ();
+      if (resize_message)
+	{
+	  static int count;
+	  printf_unfiltered ("@@ resize done %d, size = %dx%d\n", count,
+			     tui_term_width (), tui_term_height ());
+	  ++count;
+	}
       tui_redisplay_readline ();
     }
 }
@@ -1434,4 +1458,14 @@ 
 This variable controls how many spaces are used to display a tab character."),
 			     tui_set_tab_width, tui_show_tab_width,
 			     &tui_setlist, &tui_showlist);
+
+  add_setshow_boolean_cmd ("tui-resize-message", class_maintenance,
+			   &resize_message, _("\
+Set TUI resize messaging."), _("\
+Show TUI resize messaging."), _("\
+When enabled GDB will print a message when the terminal is resized."),
+			   nullptr,
+			   show_tui_resize_message,
+			   &maintenance_set_cmdlist,
+			   &maintenance_show_cmdlist);
 }