Re: [patch] circ.exp

[Pedro Alves <palves@redhat.com>]

On 05/08/2013 11:11 AM, Abid, Hafiz wrote:

> I have used similar code sequence as you described above. But I was wondering if we can
> issue an unsupported call in the 2nd '-re' and then return. That should eliminate the need
> for 'circular_supported'. It also generates an extra pass while we then call unsupported below.

It's really not a biggie either way.  This,

 PASS: check whether circular buffer is supported
 FAIL: check whether circular buffer is supported
 UNSUPPORTED: check whether circular buffer is supported

reads to me as if it's the checking itself that is not supported.
(Very) Pedantically, that 2nd -re means the "check whether circular
buffer is supported" test succeeded.  And from that successful
test, we can infer that the target in fact does not support a
circular buffer.  Contrast with an alternative message
that focuses on what we want to outcome to be, and it'd
make a little more sense to me to issue the unsupported
in the 2nd -re:

 PASS: target buffer is set to circular
 FAIL: target buffer is set to circular
 UNSUPPORTED: target buffer is set to circular

Here FAIL would be an unexpected outcome (caught by gdb_test_multiple
internally, so adorned with "(timeout)" or something else).

Note a separate a variable is still somewhat useful to bail in the case
gdb_test_multiple catches a FAILs internally (timeout, internal error).
We can also check those in the return of gdb_test_multiple, but a
separate variable usually reads nicer, IMO.

> -# return 0 for success, 1 for failure
> -proc run_trace_experiment { pass } {
> -    gdb_run_cmd 
> +# Set a tracepoint on given func.  The tracepoint is set at entry
> +# address and not 'after prologue' address.

I'd find extending the comment with something like:

 ", because we use 'tfind pc func' to find the corresponding trace frame
 afterwards, and that looks for entry address."

to be useful.

> +# Check if changing the trace buffer size is supported.  This step is
> +# repeated twice.  This helps in case if trace buffer size is 100.

s/This helps in case if/The helps in case the/

> +    # Check that we get the total_size and free_size

Missing period.

> +    if { $total_size < 0 } {
>  	return 1
>      }
>  
> -    return 0
> +    if { $free_size < 0 } {
> +	return 1
> +    }
>  }
>  
> -# return 0 for success, 1 for failure
> -proc gdb_trace_circular_tests { } {
> -    if { ![gdb_target_supports_trace] } then { 
> -	unsupported "Current target does not support trace"
> +# Calculate the size of a single frame

Missing period.  Might as well just give it a quick audit
over all strings and add the missing periods.  :-)

> +# Shrink the trace buffer so that it will not hold
> +# all ten trace frames.  Verify that frame for func0 is still
> +# collected, but frame for func9 is not.

s/that frame for/that the frame for/
s/but frame for/but the the frame for/

> +# 1) the first frame will be overwritten and therefore unavailable

Missing period here too.

> +    # Check that teh first frame is actually at func0.

typo: "teh".

The patch is okay with these fixed.  Go ahead and apply it
(but still post the final version).

Thanks,
-- 
Pedro Alves