Re: [PATCH] Increase timeout in watch-bitfields.exp for software watchpoint

[Pedro Alves <palves@redhat.com>]

On 04/14/2015 05:35 PM, Yao Qi wrote:

>>>  
>>> +# Run tests in BODY with timeout increased by factor of FACTOR.  When
>>> +# BODY is finished, restore timeout.
>>> +
>>> +proc with_timeout_factor { factor body } {
>>> +    global timeout
>>> +
>>> +    set savedtimeout $timeout
>>> +    if { [target_info exists gdb,timeout]
>>> +	 && $timeout < [target_info gdb,timeout] } {
>>> +	set oldtimeout [target_info gdb,timeout]
>>> +    } else {
>>> +	set oldtimeout $timeout
>>> +    }
>>> +    set timeout [expr $oldtimeout * $factor]
>>
>> The "timeout" variable is special.  gdb_test/gdb_test_multiple/expect
>> will take into account a local "timeout" variable in the callers
>> scope too, not just the global.  So this should be taking that
>> into account as well.  The old code didn't need to do that because it
>> was code at the global scope.  See the upvars in gdb_expect.  I think
>> we should do the same here.  We should probably move
>> that "get me highest timeout" bit of code to a shared
>> procedure (adjusted to "upvar 2 timeout timeout", most likely).
> 
> I don't think I fully understand you...  Why do we need such shared proc
> to get timeout?  Isn't simpler to just use "upvar timeout timeout" at
> the beginning of with_timeout_factor?  like this:
> 
> proc with_timeout_factor { factor body } {
>     upvar timeout timeout
> 
> and in watch-bitfields.exp proc test_watch_location and
> test_regular_watch, use "global timeout"?

With a global timeout of 60, this:

 proc foo {} {
   set timeout 1

   gdb_test ...
 }

still runs gdb_test with a timeout of 60, because, 60 > 1.

so, what should this mean:

 proc foo {} {
   set timeout 1

   gdb_test ...
   with_timeout_factor 10 {
     gdb_test ...
   }
}

is the timeout within the with block max(1, 60 * 10), or max(1 * 10, 60) ?

And this:

 proc foo {} {
   set timeout 100

   gdb_test ...
   with_timeout_factor 10 {
     gdb_test ...
   }
}

is it max(100, 60 * 10), or max(100 * 10, 60) ?


It seems to me that we should define what this does, and document it.

gdb_test etc. always take the max of local, global and board
timeouts.  In your current patch, the factor is applied after
selecting the max of global and board timeouts.  So I think that it
should be simplest to say that the the factor is applied after determining
the maximum between local, global and board timeouts.
So the shared proc would look like this:

# Of all the timeouts select the largest.  Uses the local "timeout"
# variable of the scope two levels above.

proc get_largest_timeout {} {
    upvar #0 timeout gtimeout
    upvar 2 timeout timeout
    set tmt 0
    if [info exists timeout] {
      set tmt $timeout
    }
    if { [info exists gtimeout] && $gtimeout > $tmt } {
        set tmt $gtimeout
    }
    if { [target_info exists gdb,timeout]
         && [target_info gdb,timeout] > $tmt } {
        set tmt [target_info gdb,timeout]
    }
    if { $tmt == 0 } {
        # Eeeeew.
        set tmt 60
    }

    return $tmt
}


Used like:

proc with_timeout_factor { factor body } {
    global timeout

    set savedtimeout $timeout
    set timeout [get_largest_timeout]

    set code [catch {uplevel 1 $body} result]
    if {$code == 1} {
	global errorInfo errorCode
	return -code $code -errorinfo $errorInfo -errorcode $errorCode $result
    } else {
	return -code $code $result
    }

    set timeout $savedtimeout
}


and:

proc gdb_expect { args } {
    if { [llength $args] == 2  && [lindex $args 0] != "-re" } {
	set atimeout [lindex $args 0]
	set expcode [list [lindex $args 1]]
    } else {
	set expcode $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 you don't need those "global" in
test_watch_location / test_regular_watch either.

Thanks,
Pedro Alves