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

Simon Marchi
2018-12-04T16:08:38Z

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