[PATCH] gdb/testsuite/sim: Remove redundant setting of timeout

[Andrew Burgess]

In the config/sim.exp file two functions are defined. Both of these
functions define local timeout variables and then call gdb_expect,
which (through a call to get_largest_timeout) will find the local
definition of timeout.

However, both of these functions set the local timeout to some
arbitrary value and print a log message for this "new" timeout just
before returning.

As in both cases, the timeout is a local variable, this final setting
of the timeout has no effect and can be removed.

gdb/testsuite/ChangeLog:

* config/sim.exp (gdb_target_sim): Remove redundant adjustment of
local timeout variable before return.
(gdb_load): Likewise.
---
gdb/testsuite/ChangeLog | 6 ++++++
gdb/testsuite/config/sim.exp | 4 ----
2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp
index d9072febc6a..47146c6662e 100644
--- a/gdb/testsuite/config/sim.exp
+++ b/gdb/testsuite/config/sim.exp
@@ -37,8 +37,6 @@ proc gdb_target_sim { } {
return -1
}
}
- set timeout 10
- verbose "Timeout is now $timeout seconds" 2
return 0
}

@@ -67,8 +65,6 @@ proc gdb_load { arg } {
if $verbose>1 then {
send_user "Loaded $arg into $GDB\n"
}
- set timeout 30
- verbose "Timeout is now $timeout seconds" 2
return 0
}
-re "$gdb_prompt $" {

[Simon Marchi]

Post by Andrew Burgess
In the config/sim.exp file two functions are defined. Both of these
functions define local timeout variables and then call gdb_expect,
which (through a call to get_largest_timeout) will find the local
definition of timeout.
However, both of these functions set the local timeout to some
arbitrary value and print a log message for this "new" timeout just
before returning.
As in both cases, the timeout is a local variable, this final setting
of the timeout has no effect and can be removed.

Hi Andrew,

Can you verify whether the remaining "set timeout" in those functions
have any effect at all? As you said, they are just local variables, so
I don't expect them to influence the behavior of gdb_expect. Either we
need "global timeout", or we pass the timeout directly as an argument to
gdb_expect (the latter sounds better).

Simon

[Pedro Alves]

Post by Simon Marchi

In the config/sim.exp file two functions are defined.  Both of these
functions define local timeout variables and then call gdb_expect,
which (through a call to get_largest_timeout) will find the local
definition of timeout.
However, both of these functions set the local timeout to some
arbitrary value and print a log message for this "new" timeout just
before returning.
As in both cases, the timeout is a local variable, this final setting
of the timeout has no effect and can be removed.

Hi Andrew,
Can you verify whether the remaining "set timeout" in those functions have any effect at all?  As you said, they are just local variables, so I don't expect them to influence the behavior of gdb_expect.  Either we need "global timeout", or we pass the timeout directly as an argument to gdb_expect (the latter sounds better).

Keep this in mind, from man expect:

Expect takes a rather liberal view of scoping. In particular,
variables read by commands specific to the Expect program will be sought
first from the local scope, and if not found, in the global scope. For
example, this obviates the need to place "global timeout" in every procedure
you write that uses expect. On the other hand, variables written are always
in the local scope (unless a "global" command has been issued). The most
common problem this causes is when spawn is executed in a procedure. Outside
the procedure, spawn_id no longer exists, so the spawned process is no longer
accessible simply because of scoping. Add a "global spawn_id" to such a procedure.


Mimicking that behavior, gdb_test, gdb_test_multiple and gdb_expect pick the
local timeout variable in the caller via upvar. E.g.:

proc gdb_test { args } {
global gdb_prompt
upvar timeout timeout

gdb_expect is a little more disguised, but it does the same, here,
in the get_largest_timeout path:

proc gdb_expect { args } {
...
# A timeout argument takes precedence, otherwise of all the timeouts
# select the largest.
if [info exists atimeout] {
set tmt $atimeout
} else {
set tmt [get_largest_timeout]
}
...
}

and then get_largest_timeout does:

proc get_largest_timeout {} {
upvar #0 timeout gtimeout
upvar 2 timeout timeout
^^^^^^^^^^^^^^^^^^^^^^^
...

Thanks,
Pedro Alves

[Simon Marchi]

Post by Pedro Alves

Post by Simon Marchi

In the config/sim.exp file two functions are defined.  Both of these
functions define local timeout variables and then call gdb_expect,
which (through a call to get_largest_timeout) will find the local
definition of timeout.
However, both of these functions set the local timeout to some
arbitrary value and print a log message for this "new" timeout just
before returning.
As in both cases, the timeout is a local variable, this final setting
of the timeout has no effect and can be removed.

Hi Andrew,
Can you verify whether the remaining "set timeout" in those functions
have any effect at all?  As you said, they are just local variables,
so I don't expect them to influence the behavior of gdb_expect. 
Either we need "global timeout", or we pass the timeout directly as an
argument to gdb_expect (the latter sounds better).

Expect takes a rather liberal view of scoping. In
particular,
variables read by commands specific to the Expect program will be sought
first from the local scope, and if not found, in the global scope. For
example, this obviates the need to place "global timeout" in every procedure
you write that uses expect. On the other hand, variables written are always
in the local scope (unless a "global" command has been issued).
The most
common problem this causes is when spawn is executed in a procedure. Outside
the procedure, spawn_id no longer exists, so the spawned process is no longer
accessible simply because of scoping. Add a "global spawn_id" to such a procedure.
Mimicking that behavior, gdb_test, gdb_test_multiple and gdb_expect pick the
proc gdb_test { args } {
global gdb_prompt
upvar timeout timeout
gdb_expect is a little more disguised, but it does the same, here,
proc gdb_expect { args } {
...
# A timeout argument takes precedence, otherwise of all the timeouts
# select the largest.
if [info exists atimeout] {
set tmt $atimeout
} else {
set tmt [get_largest_timeout]
}
...
}
proc get_largest_timeout {} {
upvar #0 timeout gtimeout
upvar 2 timeout timeout
^^^^^^^^^^^^^^^^^^^^^^^
...

That's very confusing, to say the least.

Simon

[Pedro Alves]

Post by Simon Marchi

Post by Simon Marchi

In the config/sim.exp file two functions are defined.  Both of these
functions define local timeout variables and then call gdb_expect,
which (through a call to get_largest_timeout) will find the local
definition of timeout.
However, both of these functions set the local timeout to some
arbitrary value and print a log message for this "new" timeout just
before returning.
As in both cases, the timeout is a local variable, this final setting
of the timeout has no effect and can be removed.

Hi Andrew,
Can you verify whether the remaining "set timeout" in those functions have any effect at all?  As you said, they are just local variables, so I don't expect them to influence the behavior of gdb_expect.  Either we need "global timeout", or we pass the timeout directly as an argument to gdb_expect (the latter sounds better).

       Expect  takes  a  rather  liberal view of scoping.  In particular,
       variables read by commands specific to the Expect program will be sought
       first from the local scope, and if not found, in the global scope.  For
       example, this obviates the need to place "global timeout" in
every procedure
       you write that uses expect.   On the  other hand, variables
written are always
       in the local scope (unless a "global" command has been issued).  The most
       common problem this causes is when spawn is executed in a
procedure.  Outside
       the procedure, spawn_id no longer exists, so the spawned
process is no longer
       accessible simply because of scoping.  Add a "global spawn_id"
to such a procedure.
Mimicking that behavior, gdb_test, gdb_test_multiple and gdb_expect pick the
proc gdb_test { args } {
    global gdb_prompt
    upvar timeout timeout
gdb_expect is a little more disguised, but it does the same, here,
proc gdb_expect { args } {
...
    # A timeout argument takes precedence, otherwise of all the timeouts
    # select the largest.
    if [info exists atimeout] {
    set tmt $atimeout
    } else {
    set tmt [get_largest_timeout]
    }
...
}
proc get_largest_timeout {} {
    upvar #0 timeout gtimeout
    upvar 2 timeout timeout
    ^^^^^^^^^^^^^^^^^^^^^^^
    ...

That's very confusing, to say the least.

Don't shoot the messenger. :-)

Thanks,
Pedro Alves

[Simon Marchi]

Post by Pedro Alves

Post by Simon Marchi
That's very confusing, to say the least.

Don't shoot the messenger. :-)

Hehe, of course.

In light of this information, I think Andrew's patch is fine. Do you?

Simon

[Pedro Alves]

Post by Simon Marchi

Post by Simon Marchi
That's very confusing, to say the least.

Don't shoot the messenger.  :-)

Hehe, of course.
In light of this information, I think Andrew's patch is fine.  Do you?

Sort of. At least with the removing the tail "set timeout" part,
I agree it's not doing anything.

As for the verbose call, we print "Timeout is now ..." messages
in a lot of places, and if you're looking at the log, I think
seeing a "Timeout is now ..." indication without seeing it changed
again reads like the timeout was never restored...

That's a preexisting problem, of course, since currently
we give the impression that we actually changed the timeout
at the end of the function but we actually didn't...

Still, IMHO, one of these would be a better change:

a) - remove the initial verbose call too, or,
b) - add "global timeout" at the start of the function, and restore
the on-entry value on exit. That way both "Timeout is now ..."
messages will be truthful. This is what e.g.,
testsuiteconfig/sid.exp does.

In either case, there will be no imbalance in the verbose output.

Thanks,
Pedro Alves

[Andrew Burgess]

Post by Pedro Alves

Post by Simon Marchi

Post by Simon Marchi
That's very confusing, to say the least.

Don't shoot the messenger.  :-)

Hehe, of course.
In light of this information, I think Andrew's patch is fine.  Do you?

Sort of. At least with the removing the tail "set timeout" part,
I agree it's not doing anything.
As for the verbose call, we print "Timeout is now ..." messages
in a lot of places, and if you're looking at the log, I think
seeing a "Timeout is now ..." indication without seeing it changed
again reads like the timeout was never restored...
That's a preexisting problem, of course, since currently
we give the impression that we actually changed the timeout
at the end of the function but we actually didn't...
a) - remove the initial verbose call too, or,
b) - add "global timeout" at the start of the function, and restore
the on-entry value on exit. That way both "Timeout is now ..."
messages will be truthful. This is what e.g.,
testsuiteconfig/sid.exp does.
In either case, there will be no imbalance in the verbose output.

Thanks both for the feedback.

In the end I went for (a) - making timeout global, backing it up, etc
just to print a log message seemed like overkill, especially when we
adjust the timeout in lots of other places without any logging at all.

With the logging gone, folding the timeout into the gdb_expect call
seemed like an obvious cleanup.

The new patch is below.

Thanks,
Andrew

--

gdb/testsuite/sim: Remove redundant setting of timeout

In the config/sim.exp file two functions are defined. Both of these
functions define local timeout variables and then call gdb_expect,
which (through a call to get_largest_timeout) will find the local
definition of timeout.

However, both of these functions set the local timeout to some
arbitrary value and print a log message for this "new" timeout just
before returning.

As in both cases, the timeout is a local variable, this final setting
of the timeout has no effect and can be removed.

As having log messages about the timeout being adjusted could cause
confusion I've removed all logging related to timeouts in this
function, timeouts are adjusted thoughout the testsuite without any
logging, there doesn't seem to be any good reason why these functions
should get their own logging.

With the logging gone there seems to be little need to a local timeout
variable at all, and so I've folded the local timeout directly into
the call to gdb_expect.

gdb/testsuite/ChangeLog:

* config/sim.exp (gdb_target_sim): Remove redundant adjustment of
local timeout variable before return, and remove all local timeout
variable entirely.
(gdb_load): Likewise.
---
gdb/testsuite/ChangeLog | 7 +++++++
gdb/testsuite/config/sim.exp | 12 ++----------
2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/gdb/testsuite/config/sim.exp b/gdb/testsuite/config/sim.exp
index d9072febc6a..fd4d506ebb8 100644
--- a/gdb/testsuite/config/sim.exp
+++ b/gdb/testsuite/config/sim.exp
@@ -26,9 +26,7 @@ proc gdb_target_sim { } {
set target_sim_options "[board_info target gdb,target_sim_options]"

send_gdb "target sim $target_sim_options\n"
- set timeout 60
- verbose "Timeout is now $timeout seconds" 2
- gdb_expect {
+ gdb_expect 60 {
-re "Connected to the simulator.*$gdb_prompt $" {
verbose "Set target to sim"
}
@@ -37,8 +35,6 @@ proc gdb_target_sim { } {
return -1
}
}
- set timeout 10
- verbose "Timeout is now $timeout seconds" 2
return 0
}

@@ -60,15 +56,11 @@ proc gdb_load { arg } {
if [gdb_target_sim] then { return -1 }

send_gdb "load\n"
- set timeout 2400
- verbose "Timeout is now $timeout seconds" 2
- gdb_expect {
+ gdb_expect 2400 {
-re ".*$gdb_prompt $" {
if $verbose>1 then {
send_user "Loaded $arg into $GDB\n"
}
- set timeout 30
- verbose "Timeout is now $timeout seconds" 2
return 0
}
-re "$gdb_prompt $" {

[Pedro Alves]

Post by Andrew Burgess
Thanks both for the feedback.
In the end I went for (a) - making timeout global, backing it up, etc
just to print a log message seemed like overkill, especially when we
adjust the timeout in lots of other places without any logging at all.
With the logging gone, folding the timeout into the gdb_expect call
seemed like an obvious cleanup.
The new patch is below.

Fine with me.

Post by Andrew Burgess
As having log messages about the timeout being adjusted could cause
confusion I've removed all logging related to timeouts in this
function, timeouts are adjusted thoughout the testsuite without any

Typo: "throughout"

Thanks,
Pedro Alves