[1/2] GDB/testsuite: Avoid timeout lowering
Commit Message
Hi,
The recent change to introduce `gdb_reverse_timeout' turned out
ineffective for board setups that set the `gdb,timeout' target variable.
A lower `gdb,timeout' setting takes precedence and defeats the effect of
`gdb_reverse_timeout'. This is because the global timeout is overridden
in gdb_test_multiple and then again in gdb_expect.
Three timeout variables are taken into account in these two places, in
this precedence:
1. The `gdb,timeout' target variable.
2. The caller's local `timeout' variable (upvar timeout)
3. The global `timeout' variable.
This precedence is obeyed by gdb_test_multiple strictly. OTOH gdb_expect
will select the higher of the two formers and will only take the latter
into account if none of the formers is present. However the two timeout
selections are conceptually the same and gdb_test_multiple does its only
for the purpose of passing it down to gdb_expect.
Therefore I decided there is no point to keep carrying on this
duplication and removed the sequence from gdb_test_multiple, however
retaining the `upvar timeout' variable definition. This way gdb_expect
will still access gdb_test_multiple's caller `timeout' variable (if any)
via its own `upvar timeout' reference.
Now as to the sequence in gdb_expect. In addition to the three variables
described above it also takes a timeout argument into account, as the
fourth value to choose from. It is currently used if it is higher than
the timeout selected from the variables as described above.
With the timeout selection code from gdb_test_multiple gone, gone is also
the most prominent use of this timeout argument, it's now used in a couple
of places only, mostly within this test framework library code itself for
preparatory commands or suchlike. With this being the case this timeout
selection code can be simplified as follows:
1. Among the three timeout variables, the highest is always chosen. This
is so that a test case doesn't inadvertently lower a high value timeout
needed by slow target boards. This is what all test cases use.
2. Any timeout argument takes precedence. This is for special cases such
as within the framework library code, e.g. it doesn't make sense to
send `set height 0' with a timeout of 7200 seconds. This is a local
command that does not interact with the target and setting a high
timeout here only risks a test suite run taking ages if it goes astray
for some reason.
3. The fallback timeout of 60s remains.
I have successfully regression tested this change on arm-linux-gnueabi and
mips-linux-gnu targets running gdbserver on real hardware and in the
former case also in QEMU run in the system emulation mode.
OK to apply?
2014-07-24 Maciej W. Rozycki <macro@codesourcery.com>
gdb/testsuite/
* lib/gdb.exp (gdb_test_multiple): Remove code to select the
timeout, don't pass one down to gdb_expect.
(gdb_expect): Rework timeout selection.
Maciej
gdb-test-timeout.diff
Comments
On 07/25/2014 06:38 AM, Maciej W. Rozycki wrote:
> With the timeout selection code from gdb_test_multiple gone, gone is also
> the most prominent use of this timeout argument, it's now used in a couple
> of places only, mostly within this test framework library code itself for
> preparatory commands or suchlike. With this being the case this timeout
> selection code can be simplified as follows:
>
> 1. Among the three timeout variables, the highest is always chosen. This
> is so that a test case doesn't inadvertently lower a high value timeout
> needed by slow target boards. This is what all test cases use.
>
> 2. Any timeout argument takes precedence. This is for special cases such
> as within the framework library code, e.g. it doesn't make sense to
> send `set height 0' with a timeout of 7200 seconds. This is a local
> command that does not interact with the target and setting a high
> timeout here only risks a test suite run taking ages if it goes astray
> for some reason.
>
> 3. The fallback timeout of 60s remains.
Maciej,
IWBN to put the descriptions about timeout selection into the comments
of proc gdb_expect.
I don't see anything wrong in this patch.
On 07/25/2014 02:37 AM, Yao Qi wrote:
> On 07/25/2014 06:38 AM, Maciej W. Rozycki wrote:
>> With the timeout selection code from gdb_test_multiple gone, gone is also
>> the most prominent use of this timeout argument, it's now used in a couple
>> of places only, mostly within this test framework library code itself for
>> preparatory commands or suchlike. With this being the case this timeout
>> selection code can be simplified as follows:
>>
>> 1. Among the three timeout variables, the highest is always chosen. This
>> is so that a test case doesn't inadvertently lower a high value timeout
>> needed by slow target boards. This is what all test cases use.
>>
>> 2. Any timeout argument takes precedence. This is for special cases such
>> as within the framework library code, e.g. it doesn't make sense to
>> send `set height 0' with a timeout of 7200 seconds. This is a local
>> command that does not interact with the target and setting a high
>> timeout here only risks a test suite run taking ages if it goes astray
>> for some reason.
Indeed. It feels like a host vs target timeout concept. That is, we
can still have a slow remote host, but that's a different vector of
slow vs a slow target.
>>
>> 3. The fallback timeout of 60s remains.
>
> Maciej,
> IWBN to put the descriptions about timeout selection into the comments
> of proc gdb_expect.
Agreed. Or even somewhere more central, and have gdb_expect
gdb_test_multiple, etc. refer to that.
> I don't see anything wrong in this patch.
Me neither.
Thanks,
Pedro Alves
On Tue, 29 Jul 2014, Pedro Alves wrote:
> >> 2. Any timeout argument takes precedence. This is for special cases such
> >> as within the framework library code, e.g. it doesn't make sense to
> >> send `set height 0' with a timeout of 7200 seconds. This is a local
> >> command that does not interact with the target and setting a high
> >> timeout here only risks a test suite run taking ages if it goes astray
> >> for some reason.
>
> Indeed. It feels like a host vs target timeout concept. That is, we
> can still have a slow remote host, but that's a different vector of
> slow vs a slow target.
Hmm, we may consider making the distinction more prominent somehow. No
idea outright exactly how, however I'll see if anything smart pops into my
mind sometime.
> >> 3. The fallback timeout of 60s remains.
> >
> > Maciej,
> > IWBN to put the descriptions about timeout selection into the comments
> > of proc gdb_expect.
>
> Agreed. Or even somewhere more central, and have gdb_expect
> gdb_test_multiple, etc. refer to that.
I'll think about it, unless any of you beats me to it. ;)
> > I don't see anything wrong in this patch.
>
> Me neither.
Applied now, thanks for your review.
Maciej
===================================================================
@@ -789,21 +789,6 @@ proc gdb_test_multiple { command message
}
}
- if [target_info exists gdb,timeout] {
- set tmt [target_info gdb,timeout]
- } else {
- if [info exists timeout] {
- set tmt $timeout
- } else {
- global timeout
- if [info exists timeout] {
- set tmt $timeout
- } else {
- set tmt 60
- }
- }
- }
-
set code {
-re ".*A problem internal to GDB has been detected" {
fail "$message (GDB internal error)"
@@ -909,7 +894,7 @@ proc gdb_test_multiple { command message
}
set result 0
- set code [catch {gdb_expect $tmt $code} string]
+ set code [catch {gdb_expect $code} string]
if {$code == 1} {
global errorInfo errorCode
return -code error -errorinfo $errorInfo -errorcode $errorCode $string
@@ -3070,35 +3055,27 @@ proc gdb_expect { args } {
set expcode $args
}
+ # A timeout argument takes precedence, otherwise of all the timeouts
+ # select the largest.
+ upvar #0 timeout gtimeout
upvar timeout timeout
-
- if [target_info exists gdb,timeout] {
+ if [info exists atimeout] {
+ set tmt $atimeout
+ } else {
+ set tmt 0
if [info exists timeout] {
- if { $timeout < [target_info gdb,timeout] } {
- set gtimeout [target_info gdb,timeout]
- } else {
- set gtimeout $timeout
- }
- } else {
- set gtimeout [target_info gdb,timeout]
+ set tmt $timeout
}
- }
-
- if ![info exists gtimeout] {
- global timeout
- if [info exists timeout] {
- set gtimeout $timeout
+ if { [info exists gtimeout] && $gtimeout > $tmt } {
+ set tmt $gtimeout
}
- }
-
- if [info exists atimeout] {
- if { ![info exists gtimeout] || $gtimeout < $atimeout } {
- set gtimeout $atimeout
+ if { [target_info exists gdb,timeout]
+ && [target_info gdb,timeout] > $tmt } {
+ set tmt [target_info gdb,timeout]
}
- } else {
- if ![info exists gtimeout] {
+ if { $tmt == 0 } {
# Eeeeew.
- set gtimeout 60
+ set tmt 60
}
}
@@ -3113,7 +3090,7 @@ proc gdb_expect { args } {
}
}
set code [catch \
- {uplevel remote_expect host $gtimeout $expcode} string]
+ {uplevel remote_expect host $tmt $expcode} string]
if [info exists old_val] {
set remote_suppress_flag $old_val
} else {