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

Pedro Alves
2018-12-04T16:11:58Z

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