Re: [patch] circ.exp

Abid, Hafiz
2013-04-16T18:03:33Z

[-- Attachment #1: Type: text/plain, Size: 7281 bytes --]

Hi Pedro,
Thanks for the review and explnation about the circular behavior.

> 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.
Here are the problems that I observed.
1) The gdb_target_supports_trace failed and we skipped all the tests.
2) Test was sending QTBuffer:size packet with ffffffff while we use
literal -1.
3) When buffer is small and we can not have all 10 frames in it, it was
testing
for "tfind 9" which tries to find the 10th frame which is never there
due to small
buffer size. I may be wrong but I think it should rather test for
"tfind tracepoint
9" which will give it a frame related to tracepoint 9.
4) Many tests were expecting current stack frame to be printed as
result of
tfind command e.g. "#0 func9 .*". But this only happens when
traceframe_number
is -1. In the following code, old_frame_id equal to current frame id
unless
traceframe_number < 0.

if (!(type == tfind_number && num == -1)
&& (has_stack_frames () || traceframe_number >= 0))
old_frame_id = get_frame_id (get_current_frame ());
...
if (frame_id_eq (old_frame_id,
get_frame_id (get_current_frame ())))
print_what = SRC_LINE;
else
print_what = SRC_AND_LOC;
>

> 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 ...
>
> 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...
Thanks for the explanation. I tried changing the test to cater this.
Now we first
try to find the the size of single trace frame and then use ((4 *size)
+ 10) as
the size of trace buffer instead of hardcoded 512. This makes sure
that we dont
get all the frame with many having only headers.

>
> 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.
Fixed.

>
> > (gdb_trace_circular_tests): Remove. Move tests to...
>
> Double space after period.
Fixed.

>
> > (top level): ... here. Call 'runto_main' before checking for
>
> Be consistent with space after '...'.
Fixed.

>
> > trace support. Call 'support_trace_packets' to check the
>
> Spurious spaces after period.
I don't see it on my system. Perhaps mailer did something.

>
> In the patch itself, many instances of missing double-space
> after period.
Fixed. I thought we have some leeway in comments:)

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

>
> > 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.
Fixed.

>
> > proc setup_tracepoints { } {
>
>
>
> > +# Test if the target support the gien packet.
>
> "supports the given packet".
Fixed.

>
> > +# Return 0 for success, 1 for failure
> > +proc support_trace_packet { packet } {
>
> s/support/supports
Fixed.

>
> > + 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?
Done. I have to edit them a bit to look ok in the current flow.

>
>
> > - "#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:
I thought we meant to test both through commands and packets. But if you
feel that we should only use commands then I can do that in next
version.

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

Regards,
Abid

gdb/testsuite:

2013-04-16 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 parts to...
(support_trace_packet): ...this.
(gdb_trace_circular_tests): Remove. Move tests to...
(top level): ...here. Call 'runto_main' before checking for
trace support. Call 'supports_trace_packets' to check the
support for QTBuffer:size and QTBuffer:circular. Add test
to calculate size of single frame. Use this size to
calculate the size of trace buffer. Use 'with_test_prefix'.

[-- Attachment #2: circ_v2.patch --]
[-- Type: text/x-patch, Size: 12158 bytes --]

diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/circ.exp
index 4c47228..86e2fe1 100644
--- a/gdb/testsuite/gdb.trace/circ.exp
+++ b/gdb/testsuite/gdb.trace/circ.exp
@@ -15,7 +15,6 @@

load_lib "trace-support.exp"

-
standard_testfile

if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug nowarnings}]} {
@@ -23,35 +22,22 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug nowarnings}]} {
}

# Tests:
-# 1) Set up a trace experiment that will collect approximately 10 frames,
+# 1) Calculate the size taken by one trace frame.
+# 2) Set up a trace experiment that will collect approximately 10 frames,
# requiring more than 512 but less than 1024 bytes of cache buffer.
# (most targets should have at least 1024 bytes of cache buffer!)
# Run and confirm that it collects all 10 frames.
-# 2) Artificially limit the trace buffer to 512 bytes, and rerun the
-# experiment. Confirm that the first several frames are collected,
+# 3) Artificially limit the trace buffer to 4x + a bytes. Here x is the size
+# of single trace frame and a is a small constant. Rerun the
+# experiment. Confirm that the first frames is collected,
# but that the last several are not.
-# 3) Set trace buffer to circular mode, still with the artificial limit
-# of 512 bytes, and rerun the experiment. Confirm that the last
-# several frames are collected, but the first several are not.
+# 4) Set trace buffer to circular mode, with the size buffer size as in
+# step 3 above. Rerun the experiment. Confirm that the last
+# frame is collected, but the first is not.
#

-# return 0 for success, 1 for failure
-proc run_trace_experiment { pass } {
- gdb_run_cmd
-
- if [gdb_test "tstart" \
- "\[\r\n\]*" \
- "start trace experiment, pass $pass"] then { return 1 }
- if [gdb_test "continue" \
- "Continuing.*Breakpoint \[0-9\]+, end.*" \
- "run to end, pass $pass"] then { return 1 }
- if [gdb_test "tstop" \
- "\[\r\n\]*" \
- "stop trace experiment, pass $pass"] then { return 1 }
- return 0
-}
-
-# return 0 for success, 1 for failure
+# Set a tracepoint on given func. Return 0 for success,
+# 1 for failure.
proc set_a_tracepoint { func } {
if [gdb_test "trace $func" \
"Tracepoint \[0-9\]+ at .*" \
@@ -66,7 +52,8 @@ proc set_a_tracepoint { func } {
return 0
}

-# return 0 for success, 1 for failure
+# Sets the tracepoints from func0 to func9 using
+# set_a_tracepoint. Return 0 for success, 1 for failure.
proc setup_tracepoints { } {
gdb_delete_tracepoints
if [set_a_tracepoint func0] then { return 1 }
@@ -82,12 +69,34 @@ proc setup_tracepoints { } {
return 0
}

-# return 0 for success, 1 for failure
-proc trace_buffer_normal { } {
+# Start the trace, run to end and then stop the trace.
+# Return 0 for success, 1 for failure.
+proc run_trace_experiment { } {
+ global decimal
+
+ if [setup_tracepoints] then { return 1 }
+ if [gdb_test "break end" "Breakpoint $decimal.*" \
+ "breakpoint at end"] then { return 1 }
+ if [gdb_test "tstart" \
+ "\[\r\n\]*" \
+ "start trace experiment"] then { return 1 }
+ if [gdb_test "continue" \
+ "Continuing.*Breakpoint \[0-9\]+, end.*" \
+ "run to end"] then { return 1 }
+ if [gdb_test "tstop" \
+ "\[\r\n\]*" \
+ "stop trace experiment"] then { return 1 }
+ return 0
+}
+
+
+# Test if the target supports the given packet.
+# Return 0 for success, 1 for failure
+proc supports_trace_packet { packet } {
global gdb_prompt

set ok 0
- set test "maint packet QTBuffer:size:ffffffff"
+ set test "maint packet $packet"
gdb_test_multiple $test $test {
-re "received: .OK.\r\n$gdb_prompt $" {
set ok 1
@@ -101,116 +110,185 @@ proc trace_buffer_normal { } {
return 1
}

- set ok 0
- set test "maint packet QTBuffer:circular:0"
- gdb_test_multiple $test $test {
- -re "received: .OK.\r\n$gdb_prompt $" {
- set ok 1
+ return 0
+}
+
+if { ![runto_main] } {
+ fail "can't run to main to check for trace support"
+ return -1
+}
+
+if { ![gdb_target_supports_trace] } then {
+ unsupported "target does not support trace"
+ return 1
+}
+
+if { [supports_trace_packet "QTBuffer:size:-1"] } then {
+ unsupported "target does not support changing trace buffer size"
+ return 1
+}
+
+if { [supports_trace_packet "QTBuffer:circular:0"] } then {
+ unsupported "target does not support circular trace buffer"
+ return 1
+}
+
+gdb_test_no_output "set circular-trace-buffer on" \
+ "set circular-trace-buffer on"
+
+gdb_test "show circular-trace-buffer" \
+ "Target's use of circular trace buffer is on." \
+ "show circular-trace-buffer (on)"
+
+gdb_test_no_output "set circular-trace-buffer off" \
+ "set circular-trace-buffer off"
+
+gdb_test "show circular-trace-buffer" \
+ "Target's use of circular trace buffer is off." \
+ "show circular-trace-buffer (off)"
+
+set total_size -1
+set free_size -1
+set frame_size -1
+
+# Determine the size used by a single frame. Put single tracepoint,
+# run and then check the total and free size using tstatus command.
+# Then subtracting free from total gives us the size of a frame.
+with_test_prefix "frame size" {
+ if [set_a_tracepoint func0] {
+ return 1
+ }
+
+ if [gdb_test "break end" "Breakpoint $decimal.*" \
+ "breakpoint at end"] {
+ return 1
+ }
+
+ if [gdb_test "tstart" \
+ "\[\r\n\]*" \
+ "start trace"] {
+ return 1
+ }
+
+ if [gdb_test "continue" \
+ "Continuing.*Breakpoint \[0-9\]+, end.*" \
+ "run to end"] {
+ return 1
+ }
+
+ if [gdb_test "tstop" \
+ "\[\r\n\]*" \
+ "stop trace"] {
+ return 1
+ }
+
+ set test "get buffer size"
+
+ gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*$gdb_prompt $" {
+ set free_size $expect_out(1,string)
+ set total_size $expect_out(2,string)
pass $test
}
- -re "\r\n$gdb_prompt $" {
- }
}
- if { !$ok } {
- unsupported $test
+
+ # Check that we get the total_size and free_size
+ 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
+set frame_size "($total_size - $free_size)"
+
+with_test_prefix "normal buffer" {
+ clean_restart $testfile
+
+ if { ![runto_main] } {
+ fail "can't run to main"
return 1
}

- if [trace_buffer_normal] then { return 1 }
+ if { [run_trace_experiment] } then {
+ return 1
+ }

- gdb_test "break begin" ".*" ""
- gdb_test "break end" ".*" ""
- gdb_test "tstop" ".*" ""
- gdb_test "tfind none" ".*" ""
+ # Check that frame 0 is actually at func0.
+ gdb_test "tfind start" ".*#0 func0 .*" \
+ "find frame zero"

- if [setup_tracepoints] then { return 1 }
+ gdb_test "tfind 9" ".*Found trace frame 9.*" \
+ "find frame nine"
+}

- # First, run the trace experiment with default attributes:
- # Make sure it behaves as expected.
- if [run_trace_experiment 1] then { return 1 }
- if [gdb_test "tfind start" \
- "#0 func0 .*" \
- "find frame zero, pass 1"] then { return 1 }
-
- if [gdb_test "tfind 9" \
- "#0 func9 .*" \
- "find frame nine, pass 1"] then { return 1 }
-
- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 1"] then { return 1 }
-
- # 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.
- if [gdb_test "maint packet QTBuffer:size:200" \
- "received: .OK." "shrink the target trace buffer"] 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.
+
+set buffer_size "((4 * $frame_size) + 10)"
+with_test_prefix "small buffer" {
+ clean_restart $testfile
+
+ if { ![runto_main] } {
+ fail "can't run to main"
return 1
}
- if [run_trace_experiment 2] then { return 1 }
- if [gdb_test "tfind start" \
- "#0 func0 .*" \
- "find frame zero, pass 2"] then { return 1 }
-
- if [gdb_test "tfind 9" \
- ".* failed to find .*" \
- "fail to find frame nine, pass 2"] then { return 1 }
-
- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 2"] then { return 1 }
-
- # 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).
- if [gdb_test "maint packet QTBuffer:circular:1" \
- "received: .OK." "make the target trace buffer circular"] then {
- return 1
+
+ gdb_test_no_output "set trace-buffer-size $buffer_size" \
+ "shrink the target trace buffer"
+
+ if { [run_trace_experiment] } then {
+ return 1
}
- if [run_trace_experiment 3] then { return 1 }
- if [gdb_test "tfind start" \
- "#0 func\[1-9\] .*" \
- "first frame is NOT frame zero, pass 3"] then { return 1 }

- if [gdb_test "tfind 9" \
- "#0 func9 .*" \
- "find frame nine, pass 3"] then { return 1 }
+ gdb_test "tfind start" ".*#0 func0 .*" \
+ "find frame zero"

- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 3"] then { return 1 }
+ gdb_test "tfind none" ".*"

- return 0
+ gdb_test "tfind tracepoint 9" ".* failed to find .*" \
+ "find frame nine";
}

-gdb_test_no_output "set circular-trace-buffer on" \
- "set circular-trace-buffer on"
+# 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 "small buffer" case).
+with_test_prefix "circular buffer" {
+ clean_restart $testfile
+
+ if { ![runto_main] } {
+ fail "can't run to main"
+ return 1
+ }

-gdb_test "show circular-trace-buffer" "Target's use of circular trace buffer is on." "show circular-trace-buffer (on)"
+ gdb_test_no_output "set trace-buffer-size $buffer_size" \
+ "shrink the target trace buffer"

-gdb_test_no_output "set circular-trace-buffer off" \
- "set circular-trace-buffer off"
+ gdb_test_no_output "set circular-trace-buffer on" \
+ "make the target trace buffer circular"

-gdb_test "show circular-trace-buffer" "Target's use of circular trace buffer is off." "show circular-trace-buffer (off)"
+ if { [run_trace_experiment] } then {
+ return 1
+ }

-# Body of test encased in a proc so we can return prematurely.
-if { ![gdb_trace_circular_tests] } then {
- # Set trace buffer attributes back to normal
- trace_buffer_normal;
-}
+ gdb_test "tstatus" \
+ ".*Buffer contains $decimal trace frames \\(of $decimal created total\\).*Trace buffer is circular.*" \
+ "trace buffer is circular"

-# Finished!
-gdb_test "tfind none" ".*" ""
+ # Frame 0 should not be at func0
+ gdb_test "tfind start" ".*#0 func\[1-9\] .*" \
+ "first frame is NOT at func0";
+
+ gdb_test "tfind none" ".*"
+
+ gdb_test \
+ "tfind tracepoint 9" ".*Found trace frame $decimal, tracepoint 9.*" \
+ "find frame nine"
+}