Re: [patch] circ.exp

["Abid, Hafiz" <hafiz_abid at mentor dot com>]

Hi Pedro,
Thanks for your review and explanation. Here is an updated patch.

On 17/04/13 16:48:07, Pedro Alves wrote:
> We should update the comments and test messages too then:
Done.

> Then might as well make all similar tfinds in the non-circular cases
> use "tfind tracepoint" too for consistency.
Done.

> I feel that we should only use commands.  There's no good reason
> to limit the test to the remote protocol.  As soon as someone  
> implements
> tracepoints on some other target (native or something else), we'd
> need to stop using direct RSP packets anyway.  The test used a direct
> packet because there was no user-accessible support for changing the
> trace buffer size.  It was just a hack.
Done.

> BTW, I realized this is the existing style in the test, but
> these "if (...) then return 1" sprinkled around are really
> unnecessary.
>
> +    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 }
>
> I'd zap them.
Done.

> I see.  The issue is that when haven't collected any registers in the
> the tracepoint (collect $regs), we end up with a frames that are
> un-unwindable (UNWIND_UNAVAILABLE).  All frames in that case have the  
> same
> id (outer_frame_id), even if they represent different functions.  I  
> started
> out with a local fix in tfind_1, but that got ugly quick.  I think we
> should fix this in the frame machinery itself.  Here's a WIP patch  
> for that.
> It makes us build a frame id that represents a function, with an  
> <unavailable>
> stack address.  I haven't thought every detail through, but I think  
> it's
> on the right path.
Your wip patch seems to solve the problem. I run the testsuite with it  
and no regression.
However, this testcase does not depend on this behavior and both  
patches can go in independantly.

Regards,
Abid


2013-04-18  Hafiz Abid Qadeer  <abidh@codesourcery.com>

	* gdb.trace/circ.exp: Remove unnecessary 'if then' checks.
	(run_trace_experiment): Test setup_tracepoints and 'break end'
	in it.
	(trace_buffer_normal): Remove.
	(gdb_trace_circular_tests): Remove.  Move tests to...
	(top level): ...here.  Call 'runto_main' before checking for
	trace support.  Use commands to check the support for circular
	trace buffer and changing of trace buffer size.  Add test
	to calculate size of single frame.  Use this size to
	calculate the size of trace buffer.  Use 'tfind tracepoint 9'
	instead of 'tfind 9'.  Use 'with_test_prefix'.


diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/circ.exp
index 4c47228..c2d1575 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,194 +22,232 @@ 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,
-#    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.
+# 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 frame for the first tracepoint is collected,
+#    but frames for the last several tracepoints 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 frame for the last
+#    tracepoint is collected but not for the first one.
 #
 
-# 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
+# Set a tracepoint on given func.  Return 0 for success,
+# 1 for failure.
+proc set_a_tracepoint { func } {
+    gdb_test "trace $func" "Tracepoint \[0-9\]+ at .*" \
+	"set tracepoint at $func"
+    gdb_trace_setactions "set actions for $func" "" "collect testload" "^$"
 }
 
-# return 0 for success, 1 for failure
-proc set_a_tracepoint { func } {
-    if [gdb_test "trace $func" \
-	    "Tracepoint \[0-9\]+ at .*" \
-	    "set tracepoint at $func"] then {
+# Sets the tracepoints from func0 to func9 using
+# set_a_tracepoint.  Return 0 for success, 1 for failure.
+proc setup_tracepoints { } {
+    gdb_delete_tracepoints
+    set_a_tracepoint func0
+    set_a_tracepoint func1
+    set_a_tracepoint func2
+    set_a_tracepoint func3
+    set_a_tracepoint func4
+    set_a_tracepoint func5
+    set_a_tracepoint func6
+    set_a_tracepoint func7
+    set_a_tracepoint func8
+    set_a_tracepoint func9
+}
+
+# Start the trace, run to end and then stop the trace.
+# Return 0 for success, 1 for failure.
+proc run_trace_experiment { } {
+    global decimal
+
+    setup_tracepoints
+    gdb_test "break end" "Breakpoint $decimal.*" "breakpoint at end"
+    gdb_test "tstart" "\[\r\n\]*" "start trace experiment"
+    gdb_test "continue" "Continuing.*Breakpoint \[0-9\]+, end.*" \
+	"run to end"
+    gdb_test "tstop" "\[\r\n\]*" "stop trace experiment"
+}
+
+if { ![runto_main] } {
+    fail "can't run to main to check for trace support"
+    return -1
+}
+
+if { ![gdb_target_supports_trace] } {
+    unsupported "target does not support trace"
+    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)"
+
+if [gdb_test "tstatus" ".*Trace buffer is circular.*" \
+    "circular buffer is supported"] {
+	unsupported "target does not support circular trace buffer"
 	return 1
-    }
-    if [gdb_trace_setactions "set actions for $func" \
-	    "" \
-	    "collect testload" "^$"] then {
+}
+
+# Check if changing trace buffer size is supported.  This step is
+# repeated twice.  This helps in case if trace buffer size is 100.
+set test_size 100
+gdb_test_no_output "set trace-buffer-size $test_size" \
+    "change buffer size to $test_size"
+
+if [gdb_test "tstatus" \
+    ".*Trace buffer has $decimal bytes of $test_size bytes free.*" \
+    "buffer size is $test_size"] {
+	unsupported "target does not support changing trace buffer size"
 	return 1
-    }
-    return 0
 }
 
-# return 0 for success, 1 for failure
-proc setup_tracepoints { } {
-    gdb_delete_tracepoints
-    if [set_a_tracepoint func0] then { return 1 }
-    if [set_a_tracepoint func1] then { return 1 }
-    if [set_a_tracepoint func2] then { return 1 }
-    if [set_a_tracepoint func3] then { return 1 }
-    if [set_a_tracepoint func4] then { return 1 }
-    if [set_a_tracepoint func5] then { return 1 }
-    if [set_a_tracepoint func6] then { return 1 }
-    if [set_a_tracepoint func7] then { return 1 }
-    if [set_a_tracepoint func8] then { return 1 }
-    if [set_a_tracepoint func9] then { return 1 }
-    return 0
+set test_size 200
+gdb_test_no_output "set trace-buffer-size $test_size" \
+    "change buffer size to $test_size"
+
+if [gdb_test "tstatus" \
+    ".*Trace buffer has $decimal bytes of $test_size bytes free.*" \
+    "buffer size is $test_size"] {
+	unsupported "target does not support changing trace buffer size"
+	return 1
 }
 
-# return 0 for success, 1 for failure
-proc trace_buffer_normal { } {
-    global gdb_prompt
+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
 
-    set ok 0
-    set test "maint packet QTBuffer:size:ffffffff"
-    gdb_test_multiple $test $test {
-	-re "received: .OK.\r\n$gdb_prompt $" {
-	    set ok 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" {
+    set_a_tracepoint func0
+
+    gdb_test "break end" "Breakpoint $decimal.*" "breakpoint at end"
+
+    gdb_test "tstart" "\[\r\n\]*" "start trace"
+
+    gdb_test "continue" "Continuing.*Breakpoint \[0-9\]+, end.*" \
+	"run to end"
+
+    gdb_test "tstop" "\[\r\n\]*" "stop trace"
+
+    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
     }
 
-    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
-	    pass $test
-	}
-	-re "\r\n$gdb_prompt $" {
-	}
-    }
-    if { !$ok } {
-	unsupported $test
+    if { $free_size < 0 } {
 	return 1
     }
-
-    return 0
 }
 
-# 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 }
-
-    gdb_test "break begin" ".*" ""
-    gdb_test "break end"   ".*" ""
-    gdb_test "tstop"       ".*" ""
-    gdb_test "tfind none"  ".*" ""
+    run_trace_experiment
 
-    if [setup_tracepoints] then { return 1 }
+    # Check that frame 0 is actually at func0.
+    gdb_test "tfind start" ".*#0  func0 .*" \
+	"find first frame"
 
-    # 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 }
+    gdb_test "tfind tracepoint 9" \
+	".*Found trace frame $decimal, tracepoint 9.*" \
+	"find frame for tracepoint 9"
+}
 
-    if [gdb_test "tfind 9" \
-	    "#0  func9 .*" \
-	    "find frame nine, pass 1"] then { return 1 }
+# 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 "tfind none" \
-	    "#0  end .*" \
-	    "quit trace debugging, pass 1"] then { return 1 }
+set buffer_size "((4 * $frame_size) + 10)"
+with_test_prefix "small buffer" {
+    clean_restart $testfile
 
-    # 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 { 
-	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 { 
+    if { ![runto_main] } {
+	fail "can't run to main"
 	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_no_output "set trace-buffer-size $buffer_size" \
+	"shrink the target trace buffer"
+
+    run_trace_experiment
 
-    if [gdb_test "tfind none" \
-	    "#0  end .*" \
-	    "quit trace debugging, pass 3"] then { return 1 }
+    gdb_test "tfind start" ".*#0  func0 .*" \
+	"find first frame"
 
-    return 0
+    gdb_test "tfind none" ".*"
+
+    gdb_test "tfind tracepoint 9" ".* failed to find .*" \
+	"find frame for tracepoint 9";
 }
 
-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 for a tracepoint other than 1
+# 3) the frame for tracepoint 9 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)"
+    run_trace_experiment
 
-# 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 for tracepoint nine"
+}