[patch] circ.exp
Abid, Hafiz
hafiz_abid@mentor.com
Wed May 8 16:18:00 GMT 2013
> 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
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: circ_v5.patch
Type: text/x-patch
Size: 15252 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20130508/f921fdf6/attachment.bin>
More information about the Gdb-patches
mailing list