Re: [patch] circ.exp

[Pedro Alves <palves at redhat dot com>]

On 03/20/2013 06:23 PM, Abid, Hafiz wrote:
> Hi All,
> This is patch to refactor circ.exp. I noticed that it was quitting early as gdb_target_supports_trace was failing.  There were some other issues too.  After the changes, it is running to completion.  How does it look?

Thanks.

It'd have been useful if you had pointed out what were the issues.
You must have come to the conclusion to you were going to refactor
the code _after_ you've analyzed them.

> One test is failing that probably shows a defect.  Following session, which is edited for clarity, shows the problem.  When there is not enough buffer left (3rd frame onward), target does not stop the trace.  It continues but only uses 6 bytes for the frame.

We generally prefer not adding new known FAILs to the testsuite.
What we do when we can't afford fixing it, is open a bugzilla bug
report, and make the test KFAIL, referring the PR.

However ...

> (gdb) set trace-buffer-size 200
> (gdb) tstart
> (gdb) c
> ...
> (gdb) tstatus
> Collected 1 trace frames.
> Trace buffer has 131 bytes of 200 bytes free (34% full).
> ...
> (gdb) c
> ...
> (gdb) tstatus
> Collected 2 trace frames.
> Trace buffer has 62 bytes of 200 bytes free (69% full).
> ...
> (gdb) c
> ...
> (gdb) tstatus
> Trace is running on the target.
> Collected 3 trace frames.
> Trace buffer has 56 bytes of 200 bytes free (72% full).
> ...
> (gdb) c
> ...
> (gdb) tstatus
> Collected 4 trace frames.
> Trace buffer has 50 bytes of 200 bytes free (75% full).
> ...
> (gdb) c
> ...
> (gdb) tstatus
> Collected 5 trace frames.
> Trace buffer has 44 bytes of 200 bytes free (78% full).
> ...
> (gdb) tstop
> (gdb) tfind start
> Found trace frame 0, tracepoint 11
> #0  func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:11
> 11    }
> (gdb) p testload
> $4 = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13}
> (gdb) tfind 4
> Found trace frame 4, tracepoint 5
> 27    }
> (gdb) p testload
> $5 = {<unavailable> <repeats 13 times>}

This is expected.  Otherwise, exaggerating, say you have several
tracepoints that have actions that collect few a few bytes, and one
tracepoint that wants to collect 100MB worth of trace frame for each
hit.  If the trace buffer has 99MB free, and that big tracepoint hits,
it's data won't fit, but it'd be wasteful to stop collecting the other
tracepoints.  Later at tfind time, you'll be able to see the partial
results, and know which bits were not collected looking for <unavailable>.
Same rationale  can be applied to multiple collect actions in the same
tracepoint.

The 6 bytes are the traceframe headers:

/* Add a raw traceframe for the given tracepoint.  */

static struct traceframe *
add_traceframe (struct tracepoint *tpoint)
{
  struct traceframe *tframe;

  tframe = trace_buffer_alloc (sizeof (struct traceframe));


So the test needs to be rearchitected to account for this...

Follows a review of the mechanics of the patch anyway.

> gdb/testsuite:
> 
> 2013-03-20  Hafiz Abid Qadeer  <abidh@codesourcery.com>
> 
>     * gdb.trace/circ.exp: (run_trace_experiment): Test
>     setup_tracepoints and 'break end' in it.
>     (trace_buffer_normal): Refactor it to...

     (trace_buffer_normal): Refactor parts to...

>     (support_trace_packet). ..this.

     (support_trace_packet): ...this.

>     (gdb_trace_circular_tests): Remove. Move tests to...

Double space after period.

>     (top level): ... here.  Call 'runto_main' before checking for

Be consistent with space after '...'.

>     trace support.     Call 'support_trace_packets' to check the

Spurious spaces after period.

In the patch itself, many instances of missing double-space
after period.

> -# return 0 for success, 1 for failure
> +# Set a tracepoint on given func. Return 0 for success,
> +# 1 for failure.

Missing double-space.

>  proc set_a_tracepoint { func } {

> +# Sets the tracepoints from func0 to func9 using
> +# set_a_tracepoint. Return 0 for success, 1 for failure.

Ditto.  Etc.

>  proc setup_tracepoints { } {



> +# Test if the target support the gien packet.

"supports the given packet".

> +# Return 0 for success, 1 for failure
> +proc support_trace_packet { packet } {

s/support/supports

> +    global gdb_prompt




> -    # Then, shrink the trace buffer so that it will not hold
> -    # all ten trace frames.  Verify that frame zero is still
> -    # collected, but frame nine is not.

> -
> -    # Finally, make the buffer circular.  Now when it runs out of
> -    # space, it should wrap around and overwrite the earliest frames.
> -    # This means that:
> -    #   1) frame zero will be overwritten and therefore unavailable
> -    #   2) the earliest frame in the buffer will be other-than-zero
> -    #   3) frame nine will be available (unlike on pass 2).

vs

> +# Pass 2.  We will have smaller buffer with circular mode off.
> +# We should get frame 0 at func0 but should not get frame 9.

> +
> +# Shrink the trace buffer so that it will not hold
> +# all ten trace frames.

> +# Pass 3. We will have smaller and circular buffer.
> +# Now when it runs out of space, it should wrap around
> +# and overwrite the earliest frames.

I find the original comments better all-around.  Could you preserve
them please?


> -	    "#0  end .*" \
> -	    "quit trace debugging, pass 3"] then { return 1 }
> +if { [support_trace_packet "QTBuffer:size:-1"] } then {
> +    unsupported "target does not support changing trace buffer size"
> +    return 1
> +}
>
> -    return 0
> +if { [support_trace_packet "QTBuffer:circular:0"] } then {
> +    unsupported "target does not support circular trace buffer"
> +    return 1
>  }

As a follow up, it'd be nice to use gdb features/commands instead
of manually sending packets to probe support.  E.g, use tstatus to
check for support:

(gdb) set circular-trace-buffer on
(gdb) tstatus
No trace has been run on the target.
Collected 0 trace frames.
Trace buffer has 5242880 bytes of 5242880 bytes free (0% full).
                                  ^^^^^^^
Trace will stop if GDB disconnects.
Trace buffer is circular.
^^^^^^^^^^^^^^^^^^^^^^^^
Not looking at any trace frame.


Another good follow up would be to convert the test to use
with_test_prefix instead of the manual ", pass 1" etc. handling.

-- 
Pedro Alves