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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2018-12-04 10:54, Pedro Alves wrote:
> On 12/04/2018 03:43 PM, Simon Marchi wrote:
>> On 2018-12-04 06:33, Andrew Burgess wrote:
>>> 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
>     ^^^^^^^^^^^^^^^^^^^^^^^
>     ...

That's very confusing, to say the least.

Simon