RFC: Fix testsuite timeout clobbers

RFC: Fix testsuite timeout clobbers

[Daniel Jacobowitz]

There are many GDB tests that have a "set timeout" in them.
Unfortunately, because we have a single global variable for the
timeout, this means that tests have different timeouts if they are run
independently or in order.

This patch cleans out many (probably not all, but all I could find)
cases of tests which modify the timeout and fail to reset it.

The timeouts are old and somewhat made up.  Still, I'm sure that
this will cause something to time out; if it does, we'll add timeouts
on a case-by-case basis.

Any comments on this?  Otherwise, I'll check it in next week.

-- 
Daniel Jacobowitz
CodeSourcery

2010-01-28  Daniel Jacobowitz  <dan@codesourcery.com>

	gdb/testsuite/
	* gdb.base/call-strs.exp, gdb.base/default.exp,
	gdb.base/ending-run.exp, gdb.base/finish.exp, gdb.base/funcargs.exp,
	gdb.base/huge.exp, gdb.base/nodebug.exp, gdb.base/ptype.exp,
	gdb.base/restore.exp, gdb.base/return.exp, gdb.base/setvar.exp,
	gdb.base/watchpoints.exp, gdb.threads/gcore-thread.exp,
	gdb.base/watchpoint-solib.exp: Save and restore timeout.
	* gdb.base/ending-run.exp: Correct restore of timeout.
	* gdb.base/page.exp: Remove unnecessary timeout setting.

---
 gdb/testsuite/gdb.base/call-strs.exp        |    9 ++++++---
 gdb/testsuite/gdb.base/default.exp          |    3 +++
 gdb/testsuite/gdb.base/ending-run.exp       |    4 ++--
 gdb/testsuite/gdb.base/finish.exp           |    2 ++
 gdb/testsuite/gdb.base/freebpcmd.exp        |    3 +++
 gdb/testsuite/gdb.base/funcargs.exp         |    3 +++
 gdb/testsuite/gdb.base/huge.exp             |    2 ++
 gdb/testsuite/gdb.base/nodebug.exp          |    4 +++-
 gdb/testsuite/gdb.base/page.exp             |    5 -----
 gdb/testsuite/gdb.base/ptype.exp            |    5 ++++-
 gdb/testsuite/gdb.base/restore.exp          |    2 ++
 gdb/testsuite/gdb.base/return.exp           |    2 ++
 gdb/testsuite/gdb.base/setvar.exp           |    5 ++++-
 gdb/testsuite/gdb.base/watchpoint-solib.exp |    8 ++++----
 gdb/testsuite/gdb.base/watchpoints.exp      |    3 ++-
 gdb/testsuite/gdb.threads/gcore-thread.exp  |    2 ++
 16 files changed, 44 insertions(+), 18 deletions(-)

Index: gdb-mainline/gdb/testsuite/gdb.base/call-strs.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/call-strs.exp	2010-01-01 00:27:53.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/call-strs.exp	2010-01-12 14:30:07.000000000 -0800
@@ -95,13 +95,14 @@ send_gdb "set print sevenbit-strings\n" 
 send_gdb "set print address off\n" ; gdb_expect -re "$gdb_prompt $"
 send_gdb "set width 0\n" ; gdb_expect -re "$gdb_prompt $"
 
-set timeout 120
-
 if ![runto_main] then {
     perror "couldn't run to breakpoint"
     continue
 }
 
+set prev_timeout $timeout
+set timeout 120
+
 #step
 send_gdb "step\n"
 gdb_expect {
@@ -263,4 +264,6 @@ if ![gdb_skip_stdio_test "call str_func(
 }
 
 gdb_exit
-return 0
+
+set timeout $prev_timeout
+
Index: gdb-mainline/gdb/testsuite/gdb.base/default.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/default.exp	2010-01-01 00:27:53.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/default.exp	2010-01-12 14:32:03.000000000 -0800
@@ -20,6 +20,7 @@
 gdb_exit
 gdb_start
 
+set prev_timeout $timeout
 set timeout 60
 
 #
@@ -801,3 +802,5 @@ gdb_test "where" "No stack." "where"
 gdb_test "x" "Argument required .starting display address.*" "x"
 
 gdb_exit
+
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/ending-run.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/ending-run.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/ending-run.exp	2010-01-12 14:33:19.000000000 -0800
@@ -261,8 +261,6 @@ if {! [target_info exists use_gdb_stub]
 	unsupported "continue after exit"
     }
 
-    set timeout $old_timeout
-
     if {$program_exited_normally} {
         gdb_test "n" ".*The program is not being run.*" "don't step after run"
     } elseif {$program_not_exited} {
@@ -285,6 +283,8 @@ if {! [target_info exists use_gdb_stub]
     }
 }
 
+set timeout $old_timeout
+
 #remote_exec build "rm -f ${binfile}"
 return 0
 
Index: gdb-mainline/gdb/testsuite/gdb.base/finish.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/finish.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/finish.exp	2010-01-12 14:33:44.000000000 -0800
@@ -124,5 +124,7 @@ proc finish_tests { } {
     finish_abbreviation "fin"
 }
 
+set prev_timeout $timeout
 set timeout 30
 finish_tests
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/freebpcmd.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/freebpcmd.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/freebpcmd.exp	2010-01-12 14:36:05.000000000 -0800
@@ -108,6 +108,7 @@ if {$i >= [llength $lines]} {
 
 gdb_run_cmd
 
+set prev_timeout $timeout
 set timeout 120
 
 gdb_test_multiple "" "run program with breakpoint commands" {
@@ -121,3 +122,5 @@ gdb_test_multiple "" "run program with b
         kfail "gdb/1489" "run program with breakpoint commands (GDB died)"
     }
 }
+
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/funcargs.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/funcargs.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/funcargs.exp	2010-01-12 15:02:00.000000000 -0800
@@ -1149,6 +1149,7 @@ gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
+set prev_timeout $timeout
 if [istarget "mips*tx39-*"] {
     set timeout 300
 } else {
@@ -1223,3 +1224,5 @@ funcargs_reload
 localvars_in_indirect_call
 funcargs_reload
 test_stepping_over_trampolines
+
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/huge.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/huge.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/huge.exp	2010-01-12 14:36:47.000000000 -0800
@@ -50,6 +50,7 @@ gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
+set prev_timeout $timeout
 set timeout 30
 
 if { ! [ runto_main ] } then {
@@ -59,3 +60,4 @@ if { ! [ runto_main ] } then {
 
 gdb_test "print a" ".1 = .0 .repeats \[0123456789\]+ times.." "print a very large data object"
 
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/nodebug.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/nodebug.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/nodebug.exp	2010-01-12 14:46:22.000000000 -0800
@@ -202,8 +202,10 @@ if [runto inner] then {
 	} else {
 	    # We need to up this because this can be really slow on some boards.
 	    # (malloc() is called as part of the test).
-	    set timeout 60;    
+	    set prev_timeout $timeout
+	    set timeout 60
 	    gdb_test {p/c array_index("abcdef",2)} " = 99 'c'"
+	    set timeout $prev_timeout
 	}
     }
     
Index: gdb-mainline/gdb/testsuite/gdb.base/page.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/page.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/page.exp	2010-01-12 14:47:22.000000000 -0800
@@ -20,11 +20,6 @@ if $tracelevel {
     strace $tracelevel
 }
 
-global message
-global timeout
-
-set timeout 200
-
 gdb_exit
 gdb_start
 
Index: gdb-mainline/gdb/testsuite/gdb.base/ptype.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/ptype.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/ptype.exp	2010-01-12 14:49:06.000000000 -0800
@@ -626,7 +626,8 @@ if [runto_main] then {
 
   # We need to up this because this can be really slow on some boards.
   # (malloc() is called as part of the test).
-  set timeout 60;
+  set prev_timeout $timeout
+  set timeout 60
 
   gdb_test "ptype \"abc\""	"type = char \\\[4\\\]"
   gdb_test "ptype {'a','b','c'}"	"type = char \\\[3\\\]"
@@ -637,6 +638,8 @@ if [runto_main] then {
   gdb_test "ptype {4,5,6}\[2\]"	"type = int"
   gdb_test "ptype *&{4,5,6}\[1\]"	"Attempt to take address of value not located in memory."
 
+  set timeout $prev_timeout
+
   # Test ptype of user register
   gdb_test "ptype \$pc" "void \\(\\*\\)\\(\\)" "ptype \$pc"
 }
Index: gdb-mainline/gdb/testsuite/gdb.base/restore.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/restore.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/restore.exp	2010-01-12 14:50:00.000000000 -0800
@@ -116,5 +116,7 @@ gdb_start
 gdb_reinitialize_dir $srcdir/$subdir
 gdb_load ${binfile}
 
+set prev_timeout $timeout
 set timeout 30
 restore_tests
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/return.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/return.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/return.exp	2010-01-12 15:02:13.000000000 -0800
@@ -91,5 +91,7 @@ proc return_tests { } {
     gdb_test "p tmp3" ".* = 5.*" "correct value returned double test (known problem with sparc solaris)"
 }
 
+set prev_timeout $timeout
 set timeout 30
 return_tests
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/setvar.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/setvar.exp	2010-01-01 00:27:54.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/setvar.exp	2010-01-12 14:51:30.000000000 -0800
@@ -374,7 +374,8 @@ v_long_member = 3,.*v_float_member = 4,.
 
 # We need to up this because this can be really slow on some boards.
 # (malloc() is called as part of the test).
-set timeout 60;
+set prev_timeout $timeout
+set timeout 60
 
 # Change the values
 test_set "set variable v_struct1 = {32, 33, 34, 35, 36, 37}" \
@@ -392,6 +393,8 @@ test_set "set variable v_struct1 = {'h',
 v_long_member = 3,.*v_float_member = 4,.*v_double_member = 5.*\\}" \
   "set print structure #3"
 
+set timeout $prev_timeout
+
 # Test printing of enumeration bitfields.
 # GNU C supports them, some other compilers don't.
 
Index: gdb-mainline/gdb/testsuite/gdb.base/watchpoints.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/watchpoints.exp	2010-01-01 00:27:55.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/watchpoints.exp	2010-01-12 14:52:52.000000000 -0800
@@ -53,6 +53,7 @@ gdb_load $binfile
     gdb_test "watch ival1" "" ""
     gdb_test "watch ival3" "" ""
 
+    set prev_timeout $timeout
     set timeout 600
 
     gdb_test "cont" "Continuing.*\[Ww\]atchpoint.*ival1.*Old value = -1.*New value = 0.*ival1 = count; ival2 = count;.*" "watchpoint hit, first time"
@@ -102,4 +103,4 @@ gdb_load $binfile
     # Check that the hit count is reported correctly
     gdb_test "info break" ".*watchpoint\[ \t\]+keep\[ \t\]+y\[ \t\]+ival3\r\n\[ \t]+breakpoint already hit 5 times.*" "Watchpoint hit count is 5"
 
-   
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.threads/gcore-thread.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.threads/gcore-thread.exp	2010-01-01 00:27:57.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.threads/gcore-thread.exp	2010-01-12 14:53:36.000000000 -0800
@@ -55,6 +55,7 @@ set horiz "\[^\n\r\]*"
 # regexp for newline
 set nl "\[\r\n\]+"
 
+set prev_timeout $timeout
 set timeout 30
 
 send_gdb "help gcore\n"
@@ -174,3 +175,4 @@ gdb_test "info threads" ".* thread2 .*" 
 gdb_test "info threads" ".*${nl}\\* ${horiz} thread2 .*" \
 	"thread2 is current thread in corefile"
 
+set timeout $prev_timeout
Index: gdb-mainline/gdb/testsuite/gdb.base/watchpoint-solib.exp
===================================================================
--- gdb-mainline.orig/gdb/testsuite/gdb.base/watchpoint-solib.exp	2010-01-13 06:25:37.000000000 -0800
+++ gdb-mainline/gdb/testsuite/gdb.base/watchpoint-solib.exp	2010-01-13 06:25:50.000000000 -0800
@@ -83,6 +83,9 @@ gdb_test_multiple "break foo" "set pendi
      }
 }
 
+set prev_timeout $timeout
+set timeout 120
+
 gdb_test "continue" ".*Breakpoint 2.*foo.*" "continue to foo"
 gdb_test "watch g" "atchpoint 3: g" "set watchpoint on g"
 gdb_test "continue" ".*New value = 1.*" "continue to watchpoint hit"
@@ -90,7 +93,4 @@ rerun_to_main
 gdb_test "continue" ".*Breakpoint 2.*foo.*" "continue to foo again"
 gdb_test "continue" ".*New value = 1.*" "continue to watchpoint hit again"
 
-
-
-
-
+set timeout $prev_timeout

Re: RFC: Fix testsuite timeout clobbers

[Joel Brobecker]

> 	gdb/testsuite/
> 	* gdb.base/call-strs.exp, gdb.base/default.exp,
> 	gdb.base/ending-run.exp, gdb.base/finish.exp, gdb.base/funcargs.exp,
> 	gdb.base/huge.exp, gdb.base/nodebug.exp, gdb.base/ptype.exp,
> 	gdb.base/restore.exp, gdb.base/return.exp, gdb.base/setvar.exp,
> 	gdb.base/watchpoints.exp, gdb.threads/gcore-thread.exp,
> 	gdb.base/watchpoint-solib.exp: Save and restore timeout.
> 	* gdb.base/ending-run.exp: Correct restore of timeout.
> 	* gdb.base/page.exp: Remove unnecessary timeout setting.

Looks fine to me.

Independently of that, do we want to try a different approache where
the timeout gets reset more systematically? For instance, I can propose:
everytime gdb_start is called, reset the timeout to the default value
(which itself should be configurable - through a site.exp or board file?)

On the same topic (timeouts), I ran the testsuite on sparc-solaris,
yesterday, and some tests were badly timing out, and each timeout
was taking what it felt like hours (the testsuite itself took more
than 2 hours, and that's after I justed killed -9 the inferiors from
the tests).  With AdaCore's testsuite, a timeout means we've lost sync
with debugger anyway - is there really an advantage to continuing a
testcase when we get a timeout? Wouldn't it just as effective to abort
the testcase after the first timeout?

-- 
Joel

Re: RFC: Fix testsuite timeout clobbers

[Daniel Jacobowitz]

On Fri, Jan 29, 2010 at 07:59:50AM +0400, Joel Brobecker wrote:
> Independently of that, do we want to try a different approache where
> the timeout gets reset more systematically? For instance, I can propose:
> everytime gdb_start is called, reset the timeout to the default value
> (which itself should be configurable - through a site.exp or board file?)

It sounds good - we'd have to audit everywhere that tests did set the
timeout, though, since if it ends up before gdb_start or there are
multiple calls to gdb_start, it'd get reset.  Does DejaGNU give us
a per-start-of-exp-file hook?

Meanwhile, I'll check this patch in now.

> On the same topic (timeouts), I ran the testsuite on sparc-solaris,
> yesterday, and some tests were badly timing out, and each timeout
> was taking what it felt like hours (the testsuite itself took more
> than 2 hours, and that's after I justed killed -9 the inferiors from
> the tests).  With AdaCore's testsuite, a timeout means we've lost sync
> with debugger anyway - is there really an advantage to continuing a
> testcase when we get a timeout? Wouldn't it just as effective to abort
> the testcase after the first timeout?

IMO this is an excellent idea.  I wonder if we shouldn't resurrect
(but, working this time!) the suppression mechanism?

-- 
Daniel Jacobowitz
CodeSourcery

[RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Joel Brobecker]

Hello,

This is a followup on a suggestion made during a previous thread
(http://www.sourceware.org/ml/gdb-patches/2010-01/msg00630.html).

The idea is to systematically reset the timeout, in order to prevent
testcases that change its value without resetting it do not affect
subsequent testcases.

The suggestion was to find a per-start-of-exp-file hook, which I eventually
found: gdb_init is always called before running any .exp file.  I should
say that I found this by reading the runtest.exp code rather than from
the reading the dejagnu documentation.  But I'm pretty sure it's fine to
rely on this.

So the approach is to use a global variable called default_test_timeout.
The default is the same value as the timeout value set by runtest.exp.
The user is allowed to override that default by setting a new value in
a site.exp file.

In chronological order:
  1. site.exp is read
  2. gdb.exp is read
  3. for each .exp testcase:
      3.a: call gdb_init
      3.b: run the .exp testcase

Based on this, I made two changes in gdb.exp:
  * set default_test_timeout during gdb.exp evaluation if not already
    overridden by the user (site.exp);
  * update gdb_init to reset timeout to default_test_timeout.

I have tested this change by observing the value of the timeout variable
at the start of a couple of testcases in a variety of situations, and
it seems to work great.

gdb/testsuite/ChangeLog:

        * lib/gdb.exp (default_test_timeout): New global variable.
        Set it to timeout if not already set.
        (gdb_init): Reset the value of timeout to default_test_timeout.

Also tested by running the entire testsuite on x86_64-linux.  I'll look
at possible documentation if the patch gets in (we might decide to use
a different variable name for the default, for instance).

OK to apply?
Thanks,
-- 
Joel

---
 gdb/testsuite/lib/gdb.exp |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9b06a2f..621fc3b 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2410,7 +2410,22 @@ proc default_gdb_init { args } {
     }
 }
 
+# The default timeout used when testing GDB commands.  We want to use
+# the same timeout as the default dejagnu timeout, unless the user has
+# already provided a specific value (probably through a site.exp file).
+global default_test_timeout
+if ![info exists default_test_timeout] {
+    set default_test_timeout $timeout
+}
+
 proc gdb_init { args } {
+    # Reset the timeout value to the default.  This way, any testcase
+    # that changes the timeout value without resetting it cannot affect
+    # the timeout used in subsequent testcases.
+    global default_test_timeout
+    global timeout
+    set timeout $default_test_timeout
+
     return [eval default_gdb_init $args];
 }
 
-- 
1.6.3.3

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> gdb/testsuite/ChangeLog:
Joel>         * lib/gdb.exp (default_test_timeout): New global variable.
Joel>         Set it to timeout if not already set.
Joel>         (gdb_init): Reset the value of timeout to default_test_timeout.

Joel> OK to apply?

FWIW, it seems reasonable to me.

Tom

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Daniel Jacobowitz]

On Thu, Feb 04, 2010 at 08:53:53AM -0700, Tom Tromey wrote:
> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> gdb/testsuite/ChangeLog:
> Joel>         * lib/gdb.exp (default_test_timeout): New global variable.
> Joel>         Set it to timeout if not already set.
> Joel>         (gdb_init): Reset the value of timeout to default_test_timeout.
> 
> Joel> OK to apply?
> 
> FWIW, it seems reasonable to me.

Ditto.  This will even work with existing board files that "set timeout".

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Joel Brobecker]

> > Joel> gdb/testsuite/ChangeLog:
> > Joel>         * lib/gdb.exp (default_test_timeout): New global variable.
> > Joel>         Set it to timeout if not already set.
> > Joel>         (gdb_init): Reset the value of timeout to default_test_timeout.
> > 
> > Joel> OK to apply?
> > 
> > FWIW, it seems reasonable to me.
> 
> Ditto.  This will even work with existing board files that "set timeout".

I haven't looked when the board files are evaluated, so I'm missing
the point. I am wondering if this has an effect on the following:

Just something that occured to me: What if I changed the implementation
to just store $timeout at the time gdb.exp is evaluated. From the user's
perspective, the difference is that he would need to override the value
of $timeout, which is an already-known variable, instead of setting the
value of the new default_test_timeout variable.  Would that be ugly?

This isn't tested (it's getting late here), but this is what I have
in mind:

    # The default timeout used when testing GDB commands.  We want to use
    # the same timeout as the default dejagnu timeout, unless the user has
    # already overridden it with a new value (probably through a site.exp
    # file).  Either way, $timeout if the timeout value we should use.
    global default_test_timeout
    set default_test_timeout $timeout
       
    proc gdb_init { args } {
        # Reset the timeout value to the default.  This way, any testcase
        # that changes the timeout value without resetting it cannot affect
        # the timeout used in subsequent testcases.
        global default_test_timeout
        global timeout
        set timeout $default_test_timeout

I can test this if you like this approach better.  Otherwise, I'll commit
the first patch tomorrow.

-- 
Joel

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Daniel Jacobowitz]

On Thu, Feb 04, 2010 at 09:42:54PM +0400, Joel Brobecker wrote:
> Just something that occured to me: What if I changed the implementation
> to just store $timeout at the time gdb.exp is evaluated. From the user's
> perspective, the difference is that he would need to override the value
> of $timeout, which is an already-known variable, instead of setting the
> value of the new default_test_timeout variable.  Would that be ugly?
> 
> This isn't tested (it's getting late here), but this is what I have
> in mind:

Right, I think this would work fine and I like it.

Here's another trick that will make it less confusing about
initialization order.  How about we info exists to set
default_test_timeout from gdb_init, if we haven't yet?

I know I've worked with board files that loaded gdb.exp themselves.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Joel Brobecker]

> > Just something that occured to me: What if I changed the implementation
> > to just store $timeout at the time gdb.exp is evaluated?
[...]
> Here's another trick that will make it less confusing about
> initialization order.  How about we info exists to set
> default_test_timeout from gdb_init, if we haven't yet?
> 
> I know I've worked with board files that loaded gdb.exp themselves.

In the end, I no longer think that this is such a good idea. I found
several issues, mostly related to the order in which various .exp files
are being loaded.  The order is:
  
  1. site.exp
  2. various dejagnu .exp files. In my case:
        Found /usr/share/dejagnu/site.exp
        Loading /usr/share/dejagnu/utils.exp
        Loading /usr/share/dejagnu/framework.exp
        Loading /usr/share/dejagnu/debugger.exp
        Loading /usr/share/dejagnu/remote.exp
        Loading /usr/share/dejagnu/telnet.exp
        Loading /usr/share/dejagnu/rlogin.exp
        Loading /usr/share/dejagnu/kermit.exp
        Loading /usr/share/dejagnu/tip.exp
        Loading /usr/share/dejagnu/rsh.exp
        Loading /usr/share/dejagnu/ftp.exp
        Loading /usr/share/dejagnu/target.exp
        Loading /usr/share/dejagnu/targetdb.exp
        Loading /usr/share/dejagnu/libgloss.exp
  3. [runtest.exp sets the timeout to 10]
  4. target is unix =>
     (a) dejagnu's unix.exp files
     (b) gdb/testsuite/config/unix.exp

After that, it starts evaluating the testcases themselves.

This implies:

  - Setting default_test_timeout from within gdb_init does not work,
    because this function is called after runtest.exp has reset
    the timeout back to 10 seconds (this is done after site.exp
    is loaded).

  - Also, even if we set the value of default_test_timeout earlier,
    during lib/gdb.exp loading (as opposed to during the first call
    to gdb_init), we still have another .exp file that resets the
    timeout from under our site.exp: gdb/testsuite/config/unix.exp.

      # Set a default timeout to be used for the tests under UNIX, rather than
      # accepting whatever default dejagnu gives us (apparently 10 seconds).
      # When running the tests over NFS, under somewhat heavy load, 10 seconds
      # does not seem to be enough.  Try starting with 60.
      set timeout 60
      verbose "Timeout is now $timeout seconds" 2
      load_lib gdb.exp

I managed to make it work, by removing the set timeout in config/unix.exp.
But in the end, I think that's too fragile, and it also prevents us from
defining our own general timeout - let's say we don't like the default
timeout of 10 seconds and we want the default to be 30 seconds (I think
60s is way too much). With this second scheme, I couldn't see a way to do
that.  For this to be possible, we'd need to have control over a .exp
file that gets sourced before site.exp.  Of course, we could simply add
a line in the generated site.exp to set the GDB timeout default, but that
still leaves us with a fairly fragile scheme, IMO (and any breakage would
probably go unnoticed).

So I suggest we stay with the initial patch: A new global named
default_test_timeout that the user can set in site.exp.

-- 
Joel

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Daniel Jacobowitz]

On Fri, Feb 05, 2010 at 11:22:23AM +0400, Joel Brobecker wrote:
> So I suggest we stay with the initial patch: A new global named
> default_test_timeout that the user can set in site.exp.

How about gdb_test_timeout, then?

I didn't realize you were talking about site.exp; I've never tried
setting a timeout there.  We use "set timeout" in board files.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA/testsuite] Reset the timeout duration at the start of each testcase.

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 89 bytes --]

> How about gdb_test_timeout, then?

Sure. Attached is what I have checked in.

-- 
Joel

[-- Attachment #2: gdb_test_timeout.diff --]
[-- Type: text/x-diff, Size: 1223 bytes --]

gdb/testsuite/ChangeLog:

        * lib/gdb.exp (gdb_test_timeout): New global variable.
        Set it to timeout if not already set.
        (gdb_init): Reset the value of timeout to gdb_test_timeout.

---
 gdb/testsuite/lib/gdb.exp |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 9b06a2f..621fc3b 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2410,7 +2410,22 @@ proc default_gdb_init { args } {
     }
 }
 
+# The default timeout used when testing GDB commands.  We want to use
+# the same timeout as the default dejagnu timeout, unless the user has
+# already provided a specific value (probably through a site.exp file).
+global gdb_test_timeout
+if ![info exists gdb_test_timeout] {
+    set gdb_test_timeout $timeout
+}
+
 proc gdb_init { args } {
+    # Reset the timeout value to the default.  This way, any testcase
+    # that changes the timeout value without resetting it cannot affect
+    # the timeout used in subsequent testcases.
+    global gdb_test_timeout
+    global timeout
+    set timeout $gdb_test_timeout
+
     return [eval default_gdb_init $args];
 }
 
-- 
1.6.3.3

Re: RFC: Fix testsuite timeout clobbers

[Joel Brobecker]

> > On the same topic (timeouts), I ran the testsuite on sparc-solaris,
> > yesterday, and some tests were badly timing out, and each timeout
> > was taking what it felt like hours (the testsuite itself took more
> > than 2 hours, and that's after I justed killed -9 the inferiors from
> > the tests).  With AdaCore's testsuite, a timeout means we've lost sync
> > with debugger anyway - is there really an advantage to continuing a
> > testcase when we get a timeout? Wouldn't it just as effective to abort
> > the testcase after the first timeout?
> 
> IMO this is an excellent idea.  I wonder if we shouldn't resurrect
> (but, working this time!) the suppression mechanism?

I'd take any approach that works - problem is: I don't really know
off-hand of any way that might work. Any ideas?  Otherwise, I'll try
digging a little when I have another minute.

-- 
Joel

Re: RFC: Fix testsuite timeout clobbers

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> With AdaCore's testsuite, a timeout means we've lost sync with
Joel> debugger anyway - is there really an advantage to continuing a
Joel> testcase when we get a timeout? Wouldn't it just as effective to
Joel> abort the testcase after the first timeout?

This would be nice to have.
Whenever I manage to introduce a bug causing a timeout, I usually have
to log into the tester and kill a bunch of stuff by hand, since
otherwise it will just grind away for hours and hours... ugh.

Tom