From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6857 invoked by alias); 16 Apr 2013 18:03:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 6845 invoked by uid 89); 16 Apr 2013 18:03:33 -0000 X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 16 Apr 2013 18:03:31 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1USAE5-0004mX-D2 from Hafiz_Abid@mentor.com ; Tue, 16 Apr 2013 11:03:29 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 16 Apr 2013 11:03:29 -0700 Received: from abidh-ubunto1104 (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server (TLS) id 14.1.289.1; Tue, 16 Apr 2013 19:03:28 +0100 Date: Tue, 16 Apr 2013 20:53:00 -0000 From: "Abid, Hafiz" Subject: Re: [patch] circ.exp To: Pedro Alves CC: In-Reply-To: <516C57DD.1040505@redhat.com> (from palves@redhat.com on Mon Apr 15 20:41:17 2013) Message-ID: <1366135406.4863.1@abidh-ubunto1104> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-zqEWniSs7hQ5sV12ztDI" X-Virus-Found: No X-SW-Source: 2013-04/txt/msg00498.txt.bz2 --=-zqEWniSs7hQ5sV12ztDI Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-length: 7222 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=20=20 literal -1. 3) When buffer is small and we can not have all 10 frames in it, it was=20= =20 testing for "tfind 9" which tries to find the 10th frame which is never there=20=20 due to small buffer size. I may be wrong but I think it should rather test for=20=20 "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=20=20 result of tfind command e.g. "#0 func9 .*". But this only happens when=20=20 traceframe_number is -1. In the following code, old_frame_id equal to current frame id=20=20 unless traceframe_number < 0. if (!(type =3D=3D tfind_number && num =3D=3D -1) && (has_stack_frames () || traceframe_number >=3D 0)) old_frame_id =3D get_frame_id (get_current_frame ()); ... if (frame_id_eq (old_frame_id, get_frame_id (get_current_frame ()))) print_what =3D SRC_LINE; else print_what =3D SRC_AND_LOC; >=20 > 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. >=20 > However ... >=20 > 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=20=20 > . > Same rationale can be applied to multiple collect actions in the same > tracepoint. >=20 > The 6 bytes are the traceframe headers: >=20 > /* Add a raw traceframe for the given tracepoint. */ >=20 > static struct traceframe * > add_traceframe (struct tracepoint *tpoint) > { > struct traceframe *tframe; >=20 > tframe =3D trace_buffer_alloc (sizeof (struct traceframe)); >=20 >=20 > So the test needs to be rearchitected to account for this... Thanks for the explanation. I tried changing the test to cater this.=20=20= =20 Now we first try to find the the size of single trace frame and then use ((4 *size)=20=20 + 10) as the size of trace buffer instead of hardcoded 512. This makes sure=20=20 that we dont get all the frame with many having only headers. >=20 > Follows a review of the mechanics of the patch anyway. >=20 > > gdb/testsuite: > > > > 2013-03-20 Hafiz Abid Qadeer > > > > * gdb.trace/circ.exp: (run_trace_experiment): Test > > setup_tracepoints and 'break end' in it. > > (trace_buffer_normal): Refactor it to... >=20 > (trace_buffer_normal): Refactor parts to... >=20 > > (support_trace_packet). ..this. >=20 > (support_trace_packet): ...this. Fixed. >=20 > > (gdb_trace_circular_tests): Remove. Move tests to... >=20 > Double space after period. Fixed. >=20 > > (top level): ... here. Call 'runto_main' before checking for >=20 > Be consistent with space after '...'. Fixed. >=20 > > trace support. Call 'support_trace_packets' to check the >=20 > Spurious spaces after period. I don't see it on my system. Perhaps mailer did something. >=20 > In the patch itself, many instances of missing double-space > after period. Fixed. I thought we have some leeway in comments:) >=20 > > -# return 0 for success, 1 for failure > > +# Set a tracepoint on given func. Return 0 for success, > > +# 1 for failure. >=20 > Missing double-space. Fixed. >=20 > > proc set_a_tracepoint { func } { >=20 > > +# Sets the tracepoints from func0 to func9 using > > +# set_a_tracepoint. Return 0 for success, 1 for failure. >=20 > Ditto. Etc. Fixed. >=20 > > proc setup_tracepoints { } { >=20 >=20 >=20 > > +# Test if the target support the gien packet. >=20 > "supports the given packet". Fixed. >=20 > > +# Return 0 for success, 1 for failure > > +proc support_trace_packet { packet } { >=20 > s/support/supports Fixed. >=20 > > + global gdb_prompt >=20 >=20 >=20 >=20 > > - # 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. >=20 > > - > > - # Finally, make the buffer circular. Now when it runs out of > > - # space, it should wrap around and overwrite the earliest=20=20 > 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). >=20 > vs >=20 > > +# Pass 2. We will have smaller buffer with circular mode off. > > +# We should get frame 0 at func0 but should not get frame 9. >=20 > > + > > +# Shrink the trace buffer so that it will not hold > > +# all ten trace frames. >=20 > > +# 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. >=20 > 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. >=20 >=20 > > - "#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=20=20 > size" > > + return 1 > > +} > > > > - return 0 > > +if { [support_trace_packet "QTBuffer:circular:0"] } then { > > + unsupported "target does not support circular trace buffer" > > + return 1 > > } >=20 > 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=20=20 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 * 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'. --=-zqEWniSs7hQ5sV12ztDI Content-Type: text/x-patch; charset="us-ascii"; name="circ_v2.patch" Content-Disposition: attachment; filename="circ_v2.patch" Content-Transfer-Encoding: quoted-printable Content-length: 11868 diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/cir= c.exp index 4c47228..86e2fe1 100644 --- a/gdb/testsuite/gdb.trace/circ.exp +++ b/gdb/testsuite/gdb.trace/circ.exp @@ -15,7 +15,6 @@ =20 load_lib "trace-support.exp" =20 - standard_testfile =20 if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug nowarning= s}]} { @@ -23,35 +22,22 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfi= le {debug nowarnings}]} { } =20 # 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 s= ize +# 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=20 -# several frames are collected, but the first several are not. +# 4) Set trace buffer to circular mode, with the size buffer size as in=20 +# step 3 above. Rerun the experiment. Confirm that the last=20 +# frame is collected, but the first is not. # =20 -# return 0 for success, 1 for failure -proc run_trace_experiment { pass } { - gdb_run_cmd=20 - - 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 } =20 -# 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 } =20 -# 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 =20 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 } =20 - 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 {=20 + 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"=20 + return 1 +} + +if { [supports_trace_packet "QTBuffer:circular:0"] } then { + unsupported "target does not support circular trace buffer"=20 + 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"] {=20 + 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_p= rompt $" { + 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 } =20 - return 0 + if { $free_size < 0 } { + return 1 + } } =20 -# return 0 for success, 1 for failure -proc gdb_trace_circular_tests { } { - if { ![gdb_target_supports_trace] } then {=20 - 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 } =20 - if [trace_buffer_normal] then { return 1 } + if { [run_trace_experiment] } then { + return 1 + } =20 - 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" =20 - if [setup_tracepoints] then { return 1 } + gdb_test "tfind 9" ".*Found trace frame 9.*" \ + "find frame nine" +} =20 - # 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 {=20 +# 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 {=20 - return 1 + + gdb_test_no_output "set trace-buffer-size $buffer_size" \ + "shrink the target trace buffer" + + if { [run_trace_experiment] } then { + return 1=20 } - 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 } =20 - if [gdb_test "tfind 9" \ - "#0 func9 .*" \ - "find frame nine, pass 3"] then { return 1 } + gdb_test "tfind start" ".*#0 func0 .*" \ + "find frame zero" =20 - if [gdb_test "tfind none" \ - "#0 end .*" \ - "quit trace debugging, pass 3"] then { return 1 } + gdb_test "tfind none" ".*" =20 - return 0 + gdb_test "tfind tracepoint 9" ".* failed to find .*" \ + "find frame nine"; } =20 -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 + } =20 -gdb_test "show circular-trace-buffer" "Target's use of circular trace buff= er is on." "show circular-trace-buffer (on)" + gdb_test_no_output "set trace-buffer-size $buffer_size" \ + "shrink the target trace buffer" =20 -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" =20 -gdb_test "show circular-trace-buffer" "Target's use of circular trace buff= er is off." "show circular-trace-buffer (off)" + if { [run_trace_experiment] } then { + return 1 + } =20 -# 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" =20 -# 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" +} --=-zqEWniSs7hQ5sV12ztDI--