Make gdb.base/corefile.exp work on terminals with few rows

Message ID 20190206034857.14608-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • Make gdb.base/corefile.exp work on terminals with few rows
Related show

Commit Message

Simon Marchi Feb. 6, 2019, 3:48 a.m.
When creating a pty to spawn a subprocess (such as gdb), Expect
copies the settings of its own controlling terminal, including the
number of rows and columns.  If you "make check" on a terminal with just
a few rows (e.g. 4), GDB will paginate before reaching the initial
prompt.  In default_gdb_start, used by most tests, this is already
handled: if we see the pagination prompt, we sent \n to continue.

Philippe reported that gdb.base/corefile.exp didn't work in terminals
with just a few rows.  This test spawns GDB by hand, because it needs to
check things before the initial prompt, which it couldn't do if it used
default_gdb_start.

In this case I think it's not safe to use the same technique as in
default_gdb_start.  Even if we could send a \n if we see a pagination
prompt, we match some multiline regexes in there.  So if a pagination
slips in there, it might make the regexes not match and fail the test.

It's also not possible to use -ex "set height 0" or -iex "set height 0",
it is handled after the introduction text is shown.

The simplest way I found to avoid showing the pagination completely is
to set stty_init (documented in expect's man page) to initialize gdb's
pty with a fixed number of rows.

gdb/testsuite/ChangeLog:

	* gdb.base/corefile.exp: Set stty_init.
---
 gdb/testsuite/gdb.base/corefile.exp | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.20.1

Comments

Pedro Alves Feb. 6, 2019, 6:42 p.m. | #1
On 02/06/2019 03:48 AM, Simon Marchi wrote:
> When creating a pty to spawn a subprocess (such as gdb), Expect

> copies the settings of its own controlling terminal, including the

> number of rows and columns.  If you "make check" on a terminal with just

> a few rows (e.g. 4), GDB will paginate before reaching the initial

> prompt.  In default_gdb_start, used by most tests, this is already

> handled: if we see the pagination prompt, we sent \n to continue.

> 

> Philippe reported that gdb.base/corefile.exp didn't work in terminals

> with just a few rows.  This test spawns GDB by hand, because it needs to

> check things before the initial prompt, which it couldn't do if it used

> default_gdb_start.

> 

> In this case I think it's not safe to use the same technique as in

> default_gdb_start.  Even if we could send a \n if we see a pagination

> prompt, we match some multiline regexes in there.  So if a pagination

> slips in there, it might make the regexes not match and fail the test.

> 

> It's also not possible to use -ex "set height 0" or -iex "set height 0",

> it is handled after the introduction text is shown.

> 

> The simplest way I found to avoid showing the pagination completely is

> to set stty_init (documented in expect's man page) to initialize gdb's

> pty with a fixed number of rows.


Hmm, good idea.  But, if you have a small terminal with just a few
columns (as opposed to rows), then the testsuite all breaks, AFAICT.

E.g., I just tried running gdb.base/break.exp with a small window and
the test hangs starting GDB.

But with:

 --- c/gdb/testsuite/lib/gdb.exp
 +++ w/gdb/testsuite/lib/gdb.exp
 @@ -4752,6 +4752,9 @@ proc gdb_init { test_file_name } {
      # tests.
      setenv TERM "dumb"
  
 +    global stty_init
 +    set stty_init "rows 25 cols 80"
 +
      # Some tests (for example gdb.base/maint.exp) shell out from gdb to use
      # grep.  Clear GREP_OPTIONS to make the behavior predictable,
      # especially having color output turned on can cause tests to fail.

... it passes.  And so does gdb.base/corefile.exp.

Is there any reason we'd ever want GDB's terminal size to match
whatever the user's term size was?  I'd think that sanitizing / forcing
the same sizes everywhere would just lead to more stable testing,
and thus be a good thing.

Thanks,
Pedro Alves
Simon Marchi Feb. 6, 2019, 11:11 p.m. | #2
On 2019-02-06 1:42 p.m., Pedro Alves wrote:
> On 02/06/2019 03:48 AM, Simon Marchi wrote:

>> When creating a pty to spawn a subprocess (such as gdb), Expect

>> copies the settings of its own controlling terminal, including the

>> number of rows and columns.  If you "make check" on a terminal with just

>> a few rows (e.g. 4), GDB will paginate before reaching the initial

>> prompt.  In default_gdb_start, used by most tests, this is already

>> handled: if we see the pagination prompt, we sent \n to continue.

>>

>> Philippe reported that gdb.base/corefile.exp didn't work in terminals

>> with just a few rows.  This test spawns GDB by hand, because it needs to

>> check things before the initial prompt, which it couldn't do if it used

>> default_gdb_start.

>>

>> In this case I think it's not safe to use the same technique as in

>> default_gdb_start.  Even if we could send a \n if we see a pagination

>> prompt, we match some multiline regexes in there.  So if a pagination

>> slips in there, it might make the regexes not match and fail the test.

>>

>> It's also not possible to use -ex "set height 0" or -iex "set height 0",

>> it is handled after the introduction text is shown.

>>

>> The simplest way I found to avoid showing the pagination completely is

>> to set stty_init (documented in expect's man page) to initialize gdb's

>> pty with a fixed number of rows.

> 

> Hmm, good idea.  But, if you have a small terminal with just a few

> columns (as opposed to rows), then the testsuite all breaks, AFAICT.

> 

> E.g., I just tried running gdb.base/break.exp with a small window and

> the test hangs starting GDB.

> 

> But with:

> 

>  --- c/gdb/testsuite/lib/gdb.exp

>  +++ w/gdb/testsuite/lib/gdb.exp

>  @@ -4752,6 +4752,9 @@ proc gdb_init { test_file_name } {

>       # tests.

>       setenv TERM "dumb"

>   

>  +    global stty_init

>  +    set stty_init "rows 25 cols 80"

>  +

>       # Some tests (for example gdb.base/maint.exp) shell out from gdb to use

>       # grep.  Clear GREP_OPTIONS to make the behavior predictable,

>       # especially having color output turned on can cause tests to fail.

> 

> ... it passes.  And so does gdb.base/corefile.exp.

> 

> Is there any reason we'd ever want GDB's terminal size to match

> whatever the user's term size was?  I'd think that sanitizing / forcing

> the same sizes everywhere would just lead to more stable testing,

> and thus be a good thing.


Agreed.  If we do this, we can remove the part where we match the pagination
prompt during startup, like so:


From 67bb1dde957fcd8a0168fb04afa3f8e3a4ccf9bb Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>

Date: Tue, 5 Feb 2019 22:48:57 -0500
Subject: [PATCH] Make gdb.base/corefile.exp work on terminals with few rows

When creating a pty to spawn a subprocess (such as gdb), Expect
copies the settings of its own controlling terminal, including the
number of rows and columns.  If you "make check" on a terminal with just
a few rows (e.g. 4), GDB will paginate before reaching the initial
prompt.  In default_gdb_start, used by most tests, this is already
handled: if we see the pagination prompt, we sent \n to continue.

Philippe reported that gdb.base/corefile.exp didn't work in terminals
with just a few rows.  This test spawns GDB by hand, because it needs to
check things before the initial prompt, which it couldn't do if it used
default_gdb_start.

In this case I think it's not safe to use the same technique as in
default_gdb_start.  Even if we could send a \n if we see a pagination
prompt, we match some multiline regexes in there.  So if a pagination
slips in there, it might make the regexes not match and fail the test.

It's also not possible to use -ex "set height 0" or -iex "set height 0",
it is handled after the introduction text is shown.

The simplest way I found to avoid showing the pagination completely is
to set stty_init (documented in expect's man page) to initialize gdb's
pty with a fixed number of rows.

And actually, if we set stty_init in gdb_init, it works nicely as a
general solution applicable to all tests.  We can therefore remove the
solution introduced in e882ef3cfc3 ("testsuite: expect possible
pagination when starting gdb") where we matched the pagination prompt
during startup.

gdb/testsuite/ChangeLog:

	* lib/gdb.exp (default_gdb_start): Don't match pagination
	prompt.
	(gdb_init): Set stty_init.
---
 gdb/testsuite/lib/gdb.exp | 44 ++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index bc7ba12d480..d05854329d8 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1638,7 +1638,7 @@ proc default_gdb_spawn { } {
 # Default gdb_start procedure.

 proc default_gdb_start { } {
-    global gdb_prompt pagination_prompt
+    global gdb_prompt
     global gdb_spawn_id
     global inferior_spawn_id

@@ -1659,29 +1659,20 @@ proc default_gdb_start { } {
     # When running over NFS, particularly if running many simultaneous
     # tests on different hosts all using the same server, things can
     # get really slow.  Give gdb at least 3 minutes to start up.
-    set loop_again 1
-    while { $loop_again } {
-	set loop_again 0
-	gdb_expect 360 {
-	    -re "$pagination_prompt" {
-		verbose "Hit pagination during startup. Pressing enter to continue."
-		send_gdb "\n"
-		set loop_again 1
-	    }
-	    -re "\[\r\n\]$gdb_prompt $" {
-		verbose "GDB initialized."
-	    }
-	    -re "$gdb_prompt $"	{
-		perror "GDB never initialized."
-		unset gdb_spawn_id
-		return -1
-	    }
-	    timeout	{
-		perror "(timeout) GDB never initialized after 10 seconds."
-		remote_close host
-		unset gdb_spawn_id
-		return -1
-	    }
+    gdb_expect 360 {
+	-re "\[\r\n\]$gdb_prompt $" {
+	    verbose "GDB initialized."
+	}
+	-re "$gdb_prompt $"	{
+	    perror "GDB never initialized."
+	    unset gdb_spawn_id
+	    return -1
+	}
+	timeout	{
+	    perror "(timeout) GDB never initialized after 10 seconds."
+	    remote_close host
+	    unset gdb_spawn_id
+	    return -1
 	}
     }

@@ -4752,6 +4743,11 @@ proc gdb_init { test_file_name } {
     # tests.
     setenv TERM "dumb"

+    # Initialize GDB's pty with a fixed size, to make sure we avoid pagination
+    # during startup.  See "man expect" for details about stty_init.
+    global stty_init
+    set stty_init "rows 25 cols 80"
+
     # Some tests (for example gdb.base/maint.exp) shell out from gdb to use
     # grep.  Clear GREP_OPTIONS to make the behavior predictable,
     # especially having color output turned on can cause tests to fail.
-- 
2.20.1
Pedro Alves Feb. 7, 2019, 1:26 p.m. | #3
On 02/06/2019 11:11 PM, Simon Marchi wrote:

> Agreed.  If we do this, we can remove the part where we match the pagination

> prompt during startup, like so:


LGTM.

Thanks,
Pedro Alves
Simon Marchi Feb. 7, 2019, 2:23 p.m. | #4
On 2019-02-07 08:26, Pedro Alves wrote:
> On 02/06/2019 11:11 PM, Simon Marchi wrote:

> 

>> Agreed.  If we do this, we can remove the part where we match the 

>> pagination

>> prompt during startup, like so:

> 

> LGTM.

> 

> Thanks,

> Pedro Alves


Thanks, pushed.

Simon

Patch

diff --git a/gdb/testsuite/gdb.base/corefile.exp b/gdb/testsuite/gdb.base/corefile.exp
index 0053f0594586..c494a875bd31 100644
--- a/gdb/testsuite/gdb.base/corefile.exp
+++ b/gdb/testsuite/gdb.base/corefile.exp
@@ -39,6 +39,10 @@  if {$corefile == ""} {
     return 0
 }
 
+# Initialize GDB's pty with a fixed size, to make sure we avoid pagination
+# during startup.  See "man expect" for details about stty_init.
+set stty_init "rows 25 cols 80"
+
 # Test that we can simply startup with a "-core=$corefile" command line arg
 # and recognize that the core file is a valid, usable core file.
 # To do this, we must shutdown the currently running gdb and restart