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

[Pedro Alves <palves at redhat dot com>]

On 12/04/2018 04:08 PM, Simon Marchi wrote:
> 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.

Don't shoot the messenger.  :-)

Thanks,
Pedro Alves