> The patch is okay with these fixed.  Go ahead and apply it
> (but still post the final version).
Thanks Pedro. I have made the suggested changes and applied the patch.  
I am attacing the version that I committed.

Regards,
Abid


On 08/05/13 16:13:27, Pedro Alves wrote:
> 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
> 
>