* [patch] circ.exp
@ 2013-03-20 18:26 Abid, Hafiz
2013-04-11 22:59 ` Abid, Hafiz
2013-04-16 7:52 ` Pedro Alves
0 siblings, 2 replies; 23+ messages in thread
From: Abid, Hafiz @ 2013-03-20 18:26 UTC (permalink / raw)
To: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2004 bytes --]
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?
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.
(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>}
Regards,
Abid
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...
(support_trace_packet). ..this.
(gdb_trace_circular_tests): Remove. Move tests to...
(top level): ... here. Call 'runto_main' before checking for
trace support. Call 'support_trace_packets' to check the
support for QTBuffer:size and QTBuffer:circular.
[-- Attachment #2: circ.patch --]
[-- Type: text/x-patch, Size: 9067 bytes --]
diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/circ.exp
index 4c47228..5c50132 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}]} {
@@ -35,23 +34,8 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug nowarnings}]} {
# several frames are collected, but the first several are 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 +50,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,27 +67,33 @@ proc setup_tracepoints { } {
return 0
}
-# return 0 for success, 1 for failure
-proc trace_buffer_normal { } {
- global gdb_prompt
+# Start the trace, run to end and then stop the trace.
+# Return 0 for success, 1 for failure.
+proc run_trace_experiment { pass } {
+ global decimal
- 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
- pass $test
- }
- -re "\r\n$gdb_prompt $" {
- }
- }
- if { !$ok } {
- unsupported $test
- return 1
- }
+ if [setup_tracepoints] then { return 1 }
+ if [gdb_test "break end" "Breakpoint $decimal.*" \
+ "breakpoint at end, pass $pass"] then { return 1 }
+ if [gdb_test "tstart" \
+ "\[\r\n\]*" \
+ "start trace experiment $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"] then { return 1 }
+ return 0
+}
+
+# Test if the target support the gien packet.
+# Return 0 for success, 1 for failure
+proc support_trace_packet { packet } {
+ global gdb_prompt
set ok 0
- set test "maint packet QTBuffer:circular:0"
+ set test "maint packet $packet"
gdb_test_multiple $test $test {
-re "received: .OK.\r\n$gdb_prompt $" {
set ok 1
@@ -119,98 +110,103 @@ proc trace_buffer_normal { } {
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"
- return 1
- }
-
- if [trace_buffer_normal] then { return 1 }
-
- gdb_test "break begin" ".*" ""
- gdb_test "break end" ".*" ""
- gdb_test "tstop" ".*" ""
- gdb_test "tfind none" ".*" ""
-
- if [setup_tracepoints] then { return 1 }
-
- # 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 {
- 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
- }
- 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 { ![runto_main] } {
+ fail "can't run to main to check for trace support"
+ return -1
+}
- if [gdb_test "tfind 9" \
- "#0 func9 .*" \
- "find frame nine, pass 3"] then { return 1 }
+if { ![gdb_target_supports_trace] } then {
+ unsupported "target does not support trace"
+ return 1
+}
- if [gdb_test "tfind none" \
- "#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
}
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 "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)"
+gdb_test "show circular-trace-buffer" \
+ "Target's use of circular trace buffer is off." \
+ "show circular-trace-buffer (off)"
+
+set pass 1
+if { [run_trace_experiment $pass] } then {
+ return 1
+}
+
+# Check that frame 0 is actually at func0.
+gdb_test "tfind start" ".*#0 func0 .*" \
+ "find frame zero $pass"
+
+gdb_test "tfind 9" ".*Found trace frame 9.*" \
+ "find frame nine $pass"
+
+# Pass 2. We will have smaller buffer with circular mode off.
+# We should get frame 0 at func0 but should not get frame 9.
+set buffer_size 512
+set pass 2
+clean_restart $testfile
+
+if { ![runto_main] } {
+ fail "can't run to main $pass"
+ return -1
+}
+
+# Shrink the trace buffer so that it will not hold
+# all ten trace frames.
+gdb_test_no_output "set trace-buffer-size $buffer_size" \
+ "shrink the target trace buffer, $pass"
-# 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;
+if { [run_trace_experiment $pass] } then {
+ return 1
}
-# Finished!
-gdb_test "tfind none" ".*" ""
+gdb_test "tfind start" ".*#0 func0 .*" \
+ "find frame zero $pass"
+
+gdb_test "tfind 9" ".* failed to find .*" \
+ "find frame nine $pass";
+
+# 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.
+set pass 3
+clean_restart $testfile
+
+if { ![runto_main] } {
+ fail "can't run to main $pass"
+ return -1
+}
+
+gdb_test_no_output "set trace-buffer-size $buffer_size" \
+ "shrink the target trace buffer $pass"
+
+gdb_test_no_output "set circular-trace-buffer on" \
+ "make the target trace buffer circular"
+
+if { [run_trace_experiment $pass] } then {
+ return 1
+}
+
+gdb_test "tstatus" \
+ ".*Buffer contains $decimal trace frames \\(of $decimal created total\\).*Trace buffer is circular.*" \
+ "trace buffer is circular"
+
+# Frame 0 should not be at func0
+gdb_test "tfind start" ".*#0 func\[1-9\] .*" \
+ "first frame is NOT frame zero";
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-03-20 18:26 [patch] circ.exp Abid, Hafiz
@ 2013-04-11 22:59 ` Abid, Hafiz
2013-04-16 7:52 ` Pedro Alves
1 sibling, 0 replies; 23+ messages in thread
From: Abid, Hafiz @ 2013-04-11 22:59 UTC (permalink / raw)
To: gdb-patches
Ping. Original post at
http://sourceware.org/ml/gdb-patches/2013-03/msg00760.html
On 20/03/13 18:23:15, 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?
>
> 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.
>
> (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>}
>
>
> Regards,
> Abid
>
> 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...
> (support_trace_packet). ..this.
> (gdb_trace_circular_tests): Remove. Move tests to...
> (top level): ... here. Call 'runto_main' before checking for
> trace support. Call 'support_trace_packets' to check the
> support for QTBuffer:size and QTBuffer:circular.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-03-20 18:26 [patch] circ.exp Abid, Hafiz
2013-04-11 22:59 ` Abid, Hafiz
@ 2013-04-16 7:52 ` Pedro Alves
2013-04-16 20:53 ` Abid, Hafiz
1 sibling, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-04-16 7:52 UTC (permalink / raw)
To: Abid, Hafiz; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-16 7:52 ` Pedro Alves
@ 2013-04-16 20:53 ` Abid, Hafiz
2013-04-17 20:33 ` Pedro Alves
0 siblings, 1 reply; 23+ messages in thread
From: Abid, Hafiz @ 2013-04-16 20:53 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- 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"
+}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-16 20:53 ` Abid, Hafiz
@ 2013-04-17 20:33 ` Pedro Alves
2013-04-17 20:54 ` Abid, Hafiz
2013-04-19 14:27 ` [patch] circ.exp Abid, Hafiz
0 siblings, 2 replies; 23+ messages in thread
From: Pedro Alves @ 2013-04-17 20:33 UTC (permalink / raw)
To: Abid, Hafiz; +Cc: gdb-patches
On 04/16/2013 07:03 PM, Abid, Hafiz wrote:
> 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.
Thanks, that's very useful.
> 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.
How interesting. The trace frame number is a detail of the
target, actually. There's nothing in GDB (IIRC) that expects
the first traceframe to be traceframe number 1. When you do
"tfind 4", that asks the target to select traceframe 4 -- the 4
is sent to the target. It must have been that the original target that
supported tracepoints, the one this test was written for originally years
and years ago recorded the trace frame number in the traceframe itself.
But that's not how GDBserver works. GDBserver counts traceframes from the
first recorded traceframe instead:
/* Given a traceframe number NUM, find the NUMth traceframe in the
buffer. */
static struct traceframe *
find_traceframe (int num)
{
struct traceframe *tframe;
int tfnum = 0;
for (tframe = FIRST_TRACEFRAME ();
tframe->tpnum != 0;
tframe = NEXT_TRACEFRAME (tframe))
{
if (tfnum == num)
return tframe;
++tfnum;
}
We should update the comments and test messages too then:
# 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).
s/other-than-zero/for a tracepoint other than 1/
s/frame nine/the frame for tracepoint 9/
gdb_test \
"tfind tracepoint 9" ".*Found trace frame $decimal, tracepoint 9.*" \
"find frame nine"
s/frame /frame for tracepoint/
etc.
Then might as well make all similar tfinds in the non-circular cases
use "tfind tracepoint" too for consistency.
> 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.
>
I'm confused. Does the tfind in question switch to a different
frame or not?
> 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;
>> 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.
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.
Thanks. This is looking great.
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.
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-17 20:33 ` Pedro Alves
@ 2013-04-17 20:54 ` Abid, Hafiz
2013-04-18 10:30 ` Pedro Alves
2013-04-19 14:27 ` [patch] circ.exp Abid, Hafiz
1 sibling, 1 reply; 23+ messages in thread
From: Abid, Hafiz @ 2013-04-17 20:54 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
> > 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.
> >
>
> I'm confused. Does the tfind in question switch to a different
> frame or not?
I have observed that location is only printed first time we give tfind
command (or give it after doing a tfind none).
If you see the example session below, you will see that 'tfind start'
prints the following line.
#0 func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
But next tfind does not print any such line. The test was assuming the
presence of this line every time. I am not sure what is expected
behavior is though.
(gdb) tstart
(gdb) info tracepoints
Num Type Disp Enb Address What
1 tracepoint keep y 0x080483b7 in func0
at
../.././../git/gdb/testsuite/gdb.trace/circ.c:28
collect testload
installed on target
2 tracepoint keep y 0x080483bc in func1
at
../.././../git/gdb/testsuite/gdb.trace/circ.c:32
collect testload
installed on target
(gdb) c
Continuing.
Breakpoint 3, end () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:72
72 }
(gdb) tstop
(gdb) tfind start
Found trace frame 0, tracepoint 1
#0 func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
28 }
(gdb) tfind
Found trace frame 1, tracepoint 2
32 }
(gdb)
Regards,
Abid
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-17 20:54 ` Abid, Hafiz
@ 2013-04-18 10:30 ` Pedro Alves
2013-04-18 11:09 ` Pedro Alves
2013-12-13 17:49 ` [PATCH] "tfind" across unavailable-stack frames Pedro Alves
0 siblings, 2 replies; 23+ messages in thread
From: Pedro Alves @ 2013-04-18 10:30 UTC (permalink / raw)
To: Abid, Hafiz; +Cc: gdb-patches
On 04/17/2013 05:06 PM, Abid, Hafiz wrote:
>
>> > 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.
>> >
>>
>> I'm confused. Does the tfind in question switch to a different
>> frame or not?
> I have observed that location is only printed first time we give tfind command (or give it after doing a tfind none).
> If you see the example session below, you will see that 'tfind start' prints the following line.
> #0 func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
>
> But next tfind does not print any such line. The test was assuming the presence of this line every time. I am not sure what is expected behavior is though.
>
> (gdb) tstart
> (gdb) info tracepoints
> Num Type Disp Enb Address What
> 1 tracepoint keep y 0x080483b7 in func0
> at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
> collect testload
> installed on target
> 2 tracepoint keep y 0x080483bc in func1
> at ../.././../git/gdb/testsuite/gdb.trace/circ.c:32
> collect testload
> installed on target
> (gdb) c
> Continuing.
>
> Breakpoint 3, end () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:72
> 72 }
> (gdb) tstop
> (gdb) tfind start
> Found trace frame 0, tracepoint 1
> #0 func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
> 28 }
> (gdb) tfind
> Found trace frame 1, tracepoint 2
> 32 }
> (gdb)
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.
---
gdb/frame.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++--------
gdb/frame.h | 8 +++++---
2 files changed, 51 insertions(+), 11 deletions(-)
diff --git a/gdb/frame.c b/gdb/frame.c
index 8d4e2c8..c62202c 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -47,6 +47,8 @@
static struct frame_info *get_prev_frame_1 (struct frame_info *this_frame);
static struct frame_info *get_prev_frame_raw (struct frame_info *this_frame);
+static struct frame_id frame_id_build_unavailable_stack (CORE_ADDR code_addr);
+
/* We keep a cache of stack frames, each of which is a "struct
frame_info". The innermost one gets allocated (in
wait_for_inferior) each time the inferior stops; current_frame
@@ -210,7 +212,9 @@ show_backtrace_limit (struct ui_file *file, int from_tty,
static void
fprint_field (struct ui_file *file, const char *name, int p, CORE_ADDR addr)
{
- if (p)
+ if (p < 0)
+ fprintf_unfiltered (file, "%s=<unavailable>", name);
+ else if (p > 0)
fprintf_unfiltered (file, "%s=%s", name, hex_string (addr));
else
fprintf_unfiltered (file, "!%s", name);
@@ -333,10 +337,27 @@ get_frame_id (struct frame_info *fi)
/* Find the unwinder. */
if (fi->unwind == NULL)
frame_unwind_find_by_frame (fi, &fi->prologue_cache);
- /* Find THIS frame's ID. */
- /* Default to outermost if no ID is found. */
- fi->this_id.value = outer_frame_id;
- fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+
+ fi->stop_reason = fi->unwind->stop_reason (fi,&fi->prologue_cache);
+
+ /* Find THIS frame's ID. If the stack is unavailable, build a
+ frame id that identifies the function (and that only matches
+ equal with another stackless frame id of the same
+ function). */
+ if (fi->stop_reason == UNWIND_UNAVAILABLE)
+ {
+ CORE_ADDR pc;
+
+ pc = get_frame_func (fi);
+ fi->this_id.value = frame_id_build_unavailable_stack (pc);
+ }
+ else
+ {
+ /* Default to outermost if no ID is found. */
+ fi->this_id.value = outer_frame_id;
+ fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value);
+ }
+
gdb_assert (frame_id_p (fi->this_id.value));
fi->this_id.p = 1;
if (frame_debug)
@@ -394,6 +415,22 @@ frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
return id;
}
+/* Construct a frame ID representing a frame where the stack address
+ is unavailable (but exists). The first parameter is the frame's
+ constant code address (typically the entry point). The special
+ identifier address is set to indicate a wild card. */
+
+static struct frame_id
+frame_id_build_unavailable_stack (CORE_ADDR code_addr)
+{
+ struct frame_id id = null_frame_id;
+
+ id.stack_addr_p = -1;
+ id.code_addr = code_addr;
+ id.code_addr_p = 1;
+ return id;
+}
+
struct frame_id
frame_id_build (CORE_ADDR stack_addr, CORE_ADDR code_addr)
{
@@ -461,7 +498,7 @@ frame_id_eq (struct frame_id l, struct frame_id r)
/* Like a NaN, if either ID is invalid, the result is false.
Note that a frame ID is invalid iff it is the null frame ID. */
eq = 0;
- else if (l.stack_addr != r.stack_addr)
+ else if (l.stack_addr_p != r.stack_addr_p || l.stack_addr != r.stack_addr)
/* If .stack addresses are different, the frames are different. */
eq = 0;
else if (l.code_addr_p && r.code_addr_p && l.code_addr != r.code_addr)
@@ -528,8 +565,9 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
{
int inner;
- if (!l.stack_addr_p || !r.stack_addr_p)
- /* Like NaN, any operation involving an invalid ID always fails. */
+ if (l.stack_addr_p != 1 || r.stack_addr_p != 1)
+ /* Like NaN, any operation involving an invalid ID always fails.
+ Likewise if either ID has an unavailable stack address. */
inner = 0;
else if (l.artificial_depth > r.artificial_depth
&& l.stack_addr == r.stack_addr
diff --git a/gdb/frame.h b/gdb/frame.h
index 31b9cb7..66226d2 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -97,8 +97,10 @@ struct frame_id
function pointer register or stack pointer register. They are
wrong.
- This field is valid only if stack_addr_p is true. Otherwise, this
- frame represents the null frame. */
+ This field is valid only if stack_addr_p is positive. If 0, this
+ frame represents the null frame. If -1, we don't have the stack
+ address available -- e.g., we're inspecting a trace frame where
+ enough registers hadn't been collected. */
CORE_ADDR stack_addr;
/* The frame's code address. This shall be constant through out the
@@ -129,7 +131,7 @@ struct frame_id
CORE_ADDR special_addr;
/* Flags to indicate the above fields have valid contents. */
- unsigned int stack_addr_p : 1;
+ int stack_addr_p : 2;
unsigned int code_addr_p : 1;
unsigned int special_addr_p : 1;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-18 10:30 ` Pedro Alves
@ 2013-04-18 11:09 ` Pedro Alves
2013-12-13 17:49 ` [PATCH] "tfind" across unavailable-stack frames Pedro Alves
1 sibling, 0 replies; 23+ messages in thread
From: Pedro Alves @ 2013-04-18 11:09 UTC (permalink / raw)
To: Abid, Hafiz; +Cc: gdb-patches
On 04/17/2013 10:18 PM, Pedro Alves wrote:
> 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.
Below's that version. It feels very much like a workaround.
outer_frame_id should die.
> I think we should fix this in the frame machinery itself.
---
gdb/tracepoint.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 4113999..fa2ce5d 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -2270,6 +2270,9 @@ tfind_1 (enum trace_find_type type, int num,
struct frame_id old_frame_id = null_frame_id;
struct tracepoint *tp;
struct ui_out *uiout = current_uiout;
+ enum unwind_stop_reason old_frame_stop_reason = UNWIND_NO_REASON;
+ int old_frame_func_pc_p = 0;
+ CORE_ADDR old_frame_func_pc;
/* Only try to get the current stack frame if we have a chance of
succeeding. In particular, if we're trying to get a first trace
@@ -2279,7 +2282,17 @@ tfind_1 (enum trace_find_type type, int num,
trace frame. */
if (!(type == tfind_number && num == -1)
&& (has_stack_frames () || traceframe_number >= 0))
- old_frame_id = get_frame_id (get_current_frame ());
+ {
+ struct frame_info *frame = get_current_frame ();
+
+ old_frame_id = get_frame_id (frame);
+
+ get_prev_frame (frame);
+ old_frame_stop_reason = get_frame_unwind_stop_reason (frame);
+
+ old_frame_func_pc_p = get_frame_func_if_available (frame,
+ &old_frame_func_pc);
+ }
target_frameno = target_trace_find (type, num, addr1, addr2,
&target_tracept);
@@ -2384,8 +2397,19 @@ tfind_1 (enum trace_find_type type, int num,
function and it's arguments) -- otherwise we'll just show the
new source line. */
- if (frame_id_eq (old_frame_id,
- get_frame_id (get_current_frame ())))
+ struct frame_info *frame = get_current_frame ();
+ CORE_ADDR frame_func_pc;
+
+ get_prev_frame (frame);
+
+ if ((old_frame_stop_reason == UNWIND_UNAVAILABLE
+ || get_frame_unwind_stop_reason (frame) == UNWIND_UNAVAILABLE)
+ && ((old_frame_func_pc_p
+ != get_frame_func_if_available (frame, &frame_func_pc))
+ || old_frame_func_pc != frame_func_pc))
+ print_what = SRC_AND_LOC;
+ else if (frame_id_eq (old_frame_id,
+ get_frame_id (get_current_frame ())))
print_what = SRC_LINE;
else
print_what = SRC_AND_LOC;
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-17 20:33 ` Pedro Alves
2013-04-17 20:54 ` Abid, Hafiz
@ 2013-04-19 14:27 ` Abid, Hafiz
2013-04-19 14:28 ` Pedro Alves
1 sibling, 1 reply; 23+ messages in thread
From: Abid, Hafiz @ 2013-04-19 14:27 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 2691 bytes --]
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'.
[-- Attachment #2: circ_v3.patch --]
[-- Type: text/x-patch, Size: 13474 bytes --]
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"
+}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-19 14:27 ` [patch] circ.exp Abid, Hafiz
@ 2013-04-19 14:28 ` Pedro Alves
2013-05-08 10:12 ` Abid, Hafiz
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-04-19 14:28 UTC (permalink / raw)
To: Abid, Hafiz; +Cc: gdb-patches
Hi,
On 04/18/2013 04:39 PM, Abid, Hafiz wrote:
> 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
s/size buffer size/buffer size/
> +# 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
> +if [gdb_test "tstatus" ".*Trace buffer is circular.*" \
> + "circular buffer is supported"] {
> + unsupported "target does not support circular trace buffer"
This issues a FAIL in the unsupported case. We need to instead
use gdb_test_multiple to prevent that. E.g.,
set circular_supported -1
set test "check whether circular buffer is supported"
gdb_test_multiple "tstatus" $test {
-re ".*Trace buffer is circular.*$gdb_prompt $" {
pass $test
}
-re "$gdb_prompt $" {
pass $test
}
}
if { $circular_supported < 0 } {
unsupported "target does not support circular trace buffer"
return 1
}
Please disable circular tracing support in your gdbserver (e.g.,
hack handle_tracepoint_general_set) and make sure the file
tests without FAILs.
> 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
> }
Likewise, disable buffer size support, and make sure this doesn't
issue a FAIL.
>
> -# 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
> }
Likewise.
>
> -# 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.
"Set a single tracepoint, " ... "using the 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.
s/frame 0/the first frame/
> + 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"
Sorry, this isn't right yet. The number space of tracepoints is
shared with breakpoints. It didn't used to be that way up until a couple of
years ago. So the tracepoint at func9 is not actually tracepoint 9 -- it
depends on the number of breakpoints the test has setup before creating
the tracepoints.
(gdb) PASS: gdb.trace/circ.exp: show circular-trace-buffer (off)
trace func0
Tracepoint 2 at 0x400540: file ../../../src/gdb/testsuite/gdb.trace/circ.c, line 28.
(gdb) PASS: gdb.trace/circ.exp: frame size: set tracepoint at func0
...
(gdb) PASS: gdb.trace/circ.exp: normal buffer: set actions for func8
trace func9
Tracepoint 11 at 0x400576: file ../../../src/gdb/testsuite/gdb.trace/circ.c, line 64.
(gdb) PASS: gdb.trace/circ.exp: normal buffer: set tracepoint at func9
Hardcoding a number is fragile. The test may change in the future, or the
dejagnu board file itself may create breakpoints. We could either
ask GDB for the tracepoint number (it's in $tpnum), or
use "tfind pc func9".
> +}
>
> - 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
s/frame zero/somethingelse/. "that a frame for func0"
> +# collected, but frame nine is not.
likewise for frame nine.
>
> - 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
"frame zero" here.
> +# 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
First frame.
> + 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"
> +}
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-04-19 14:28 ` Pedro Alves
@ 2013-05-08 10:12 ` Abid, Hafiz
2013-05-08 15:13 ` Pedro Alves
0 siblings, 1 reply; 23+ messages in thread
From: Abid, Hafiz @ 2013-05-08 10:12 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 4533 bytes --]
Hi Pedro,
Thanks for your review. Here is an updated patch.
> s/size buffer size/buffer size/
Fixed.
> > +if [gdb_test "tstatus" ".*Trace buffer is circular.*" \
> > + "circular buffer is supported"] {
> > + unsupported "target does not support circular trace buffer"
>
> This issues a FAIL in the unsupported case. We need to instead
> use gdb_test_multiple to prevent that. E.g.,
>
> set circular_supported -1
> set test "check whether circular buffer is supported"
> gdb_test_multiple "tstatus" $test {
> -re ".*Trace buffer is circular.*$gdb_prompt $" {
> pass $test
> }
> -re "$gdb_prompt $" {
> pass $test
> }
> }
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.
>
> if { $circular_supported < 0 } {
> unsupported "target does not support circular trace buffer"
> return 1
> }
>
> Please disable circular tracing support in your gdbserver (e.g.,
> hack handle_tracepoint_general_set) and make sure the file
> tests without FAILs.
Fixed. I tested by disabling the support of the packet in gdbserver as
you suggested.
I noticed that when support was disabled, then 'set
circular-trace-buffer on'
gives 'Target does not support this command' error.
I modified the test a bit to handle that error. Similar case with
'set trace-buffer-size' when support for QTBuffer:size is disabled.
>
> Likewise, disable buffer size support, and make sure this doesn't
> issue a FAIL.
>
Done.
>
> Likewise.
>
Done.
>
> "Set a single tracepoint, " ... "using the tstatus command".
>
Fixed.
>
> s/frame 0/the first frame/
>
Fixed.
> Sorry, this isn't right yet. The number space of tracepoints is
> shared with breakpoints. It didn't used to be that way up until a
> couple of
> years ago. So the tracepoint at func9 is not actually tracepoint 9
> -- it
> depends on the number of breakpoints the test has setup before
> creating
> the tracepoints.
>
> (gdb) PASS: gdb.trace/circ.exp: show circular-trace-buffer (off)
> trace func0
> Tracepoint 2 at 0x400540: file
> ../../../src/gdb/testsuite/gdb.trace/circ.c, line 28.
> (gdb) PASS: gdb.trace/circ.exp: frame size: set tracepoint at func0
> ...
> (gdb) PASS: gdb.trace/circ.exp: normal buffer: set actions for func8
> trace func9
> Tracepoint 11 at 0x400576: file
> ../../../src/gdb/testsuite/gdb.trace/circ.c, line 64.
> (gdb) PASS: gdb.trace/circ.exp: normal buffer: set tracepoint at func9
>
> Hardcoding a number is fragile. The test may change in the future,
> or the
> dejagnu board file itself may create breakpoints. We could either
> ask GDB for the tracepoint number (it's in $tpnum), or
> use "tfind pc func9".
I am using "tfind pc func9" now. It also forced me to put the
tracepoints
using 'trace *func9' in 'set_a_tracepoint' so that tracepoint address
matches
here.
>
> s/frame zero/somethingelse/. "that a frame for func0"
>
Fixed.
>
> likewise for frame nine.
>
Fixed.
> > +# 1) frame zero will be overwritten and therefore unavailable
>
> "frame zero" here.
>
Fixed.
> > -# Finished!
> > -gdb_test "tfind none" ".*" ""
> > + # Frame 0 should not be at func0
>
> First frame.
>
Fixed.
> > + 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"
> > +}
Thanks,
Abid
gdb/testsuite/ChangeLog:
2013-05-08 Hafiz Abid Qadeer <abidh@codesourcery.com>
* gdb.trace/circ.exp: Remove unnecessary 'if then' checks.
(set_a_tracepoint): Set tracepoint before prologue.
(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 pc func9'
instead of 'tfind 9'. Use 'with_test_prefix'.
[-- Attachment #2: circ_v4.patch --]
[-- Type: text/x-patch, Size: 14298 bytes --]
diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/circ.exp
index 4c47228..3310800 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,264 @@ 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 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
+# Set a tracepoint on given func. The tracepoint is set at entry
+# address and not 'after prologue' address.
+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" "^$"
+}
- 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
+# Sets the tracepoints from func0 to func9 using set_a_tracepoint.
+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
}
-# 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 {
+# Start the trace, run to end and then stop the trace.
+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
+}
+
+set test "set circular-trace-buffer on"
+gdb_test_multiple "set circular-trace-buffer on" $test {
+ -re ".*Target does not support this command.*$gdb_prompt $" {
+ unsupported "target does not support circular trace buffer"
return 1
}
- if [gdb_trace_setactions "set actions for $func" \
- "" \
- "collect testload" "^$"] then {
+ -re "$gdb_prompt $" {
+ pass $test
+ }
+}
+
+set circular_supported -1
+set test "check whether circular buffer is supported"
+
+gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer is circular.*$gdb_prompt $" {
+ set circular_supported 1
+ pass $test
+ }
+ -re "$gdb_prompt $" {
+ pass $test
+ }
+}
+
+if { $circular_supported < 0 } {
+ unsupported "target does not support circular trace buffer"
+ return 1
+}
+
+gdb_test "show circular-trace-buffer" \
+ "Target's use of circular trace buffer is on." \
+ "show circular-trace-buffer (on)"
+
+# Check if changing the trace buffer size is supported. This step is
+# repeated twice. This helps in case if trace buffer size is 100.
+set test_size 100
+set test "change buffer size to $test_size"
+gdb_test_multiple "set trace-buffer-size $test_size" $test {
+ -re ".*Target does not support this command.*$gdb_prompt $" {
+ unsupported "target does not support changing trace buffer size"
return 1
}
- return 0
+ -re "$gdb_prompt $" {
+ pass $test
+ }
}
-# 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
-}
-
-# return 0 for success, 1 for failure
-proc trace_buffer_normal { } {
- global gdb_prompt
-
- 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
- pass $test
- }
- -re "\r\n$gdb_prompt $" {
+set test "check whether setting trace buffer size is supported"
+gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*$gdb_prompt $" {
+ set total_size $expect_out(2,string)
+ if { $test_size != $total_size } {
+ unsupported "target does not support changing trace buffer size"
+ return 1
}
+ pass $test
}
- if { !$ok } {
- unsupported $test
- return 1
+}
+
+set test_size 400
+gdb_test_no_output "set trace-buffer-size $test_size" \
+ "change buffer size to $test_size"
+
+gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*$gdb_prompt $" {
+ set total_size $expect_out(2,string)
+ if { $test_size != $total_size } {
+ unsupported "target does not support changing trace buffer size"
+ return 1
+ }
+ pass $test
}
+}
+
+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:circular:0"
- gdb_test_multiple $test $test {
- -re "received: .OK.\r\n$gdb_prompt $" {
- set ok 1
+# Determine the size used by a single frame. Set a single tracepoint,
+# run and then check the total and free size using the 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
}
- 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 }
+ run_trace_experiment
- gdb_test "break begin" ".*" ""
- gdb_test "break end" ".*" ""
- gdb_test "tstop" ".*" ""
- gdb_test "tfind none" ".*" ""
+ # Check that teh first frame is actually at func0.
+ gdb_test "tfind start" ".*#0 func0 .*" \
+ "first frame is at func0"
- if [setup_tracepoints] then { return 1 }
-
- # 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 pc func9" \
+ ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ "find frame for func9"
+}
- 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 for func0 is still
+# collected, but frame for func9 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"
- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 3"] then { return 1 }
+ run_trace_experiment
- return 0
+ gdb_test "tfind start" ".*#0 func0 .*" \
+ "first frame is at func0"
+
+ gdb_test "tfind pc func9" ".* failed to find .*" \
+ "find frame for func9";
}
-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) the first frame will be overwritten and therefore unavailable
+# 2) the earliest frame in the buffer will not be for func0.
+# 3) the frame for func9 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" ".*" ""
+ # The first frame should not be at func0
+ gdb_test "tfind start" ".*#0 func\[1-9\] .*" \
+ "first frame is NOT at func0"
+
+ gdb_test \
+ "tfind pc func9" \
+ ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ "find frame for func9"
+}
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-05-08 10:12 ` Abid, Hafiz
@ 2013-05-08 15:13 ` Pedro Alves
2013-05-08 16:18 ` Abid, Hafiz
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-05-08 15:13 UTC (permalink / raw)
To: Abid, Hafiz; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [patch] circ.exp
2013-05-08 15:13 ` Pedro Alves
@ 2013-05-08 16:18 ` Abid, Hafiz
0 siblings, 0 replies; 23+ messages in thread
From: Abid, Hafiz @ 2013-05-08 16:18 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]
> 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
>
>
[-- Attachment #2: circ_v5.patch --]
[-- Type: text/x-patch, Size: 15707 bytes --]
? src/gdb/.new.ChangeLog
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.3651
diff -u -r1.3651 ChangeLog
--- gdb/testsuite/ChangeLog 7 May 2013 18:06:16 -0000 1.3651
+++ gdb/testsuite/ChangeLog 8 May 2013 16:12:04 -0000
@@ -1,3 +1,18 @@
+2013-05-08 Hafiz Abid Qadeer <abidh@codesourcery.com>
+
+ * gdb.trace/circ.exp: Remove unnecessary 'if then' checks.
+ (set_a_tracepoint): Set tracepoint before prologue.
+ (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 pc func9'
+ instead of 'tfind 9'. Use 'with_test_prefix'.
+
2013-05-07 Tom Tromey <tromey@redhat.com>
* lib/selftest-support.exp: New file.
Index: gdb/testsuite/gdb.trace/circ.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.trace/circ.exp,v
retrieving revision 1.25
diff -u -r1.25 circ.exp
--- gdb/testsuite/gdb.trace/circ.exp 14 Mar 2013 13:34:06 -0000 1.25
+++ gdb/testsuite/gdb.trace/circ.exp 8 May 2013 16:12:05 -0000
@@ -15,7 +15,6 @@
load_lib "trace-support.exp"
-
standard_testfile
if {[prepare_for_testing $testfile.exp $testfile $srcfile {debug nowarnings}]} {
@@ -23,194 +22,266 @@
}
# 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 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. The tracepoint is set at entry
+# address and not 'after prologue' address because we use
+# 'tfind pc func' to find the corresponding trace frame afterwards,
+# and that looks for entry address.
+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.
+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.
+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
+}
+
+set test "set circular-trace-buffer on"
+gdb_test_multiple "set circular-trace-buffer on" $test {
+ -re ".*Target does not support this command.*$gdb_prompt $" {
+ unsupported "target does not support circular trace buffer"
return 1
}
- if [gdb_trace_setactions "set actions for $func" \
- "" \
- "collect testload" "^$"] then {
- return 1
+ -re "$gdb_prompt $" {
+ pass $test
}
- 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
-}
-
-# return 0 for success, 1 for failure
-proc trace_buffer_normal { } {
- global gdb_prompt
-
- 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
- pass $test
- }
- -re "\r\n$gdb_prompt $" {
- }
+set circular_supported -1
+set test "check whether circular buffer is supported"
+
+gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer is circular.*$gdb_prompt $" {
+ set circular_supported 1
+ pass $test
}
- if { !$ok } {
- unsupported $test
- return 1
+ -re "$gdb_prompt $" {
+ pass $test
}
+}
- 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 { $circular_supported < 0 } {
+ unsupported "target does not support circular trace buffer"
+ return 1
+}
+
+gdb_test "show circular-trace-buffer" \
+ "Target's use of circular trace buffer is on." \
+ "show circular-trace-buffer (on)"
+
+# Check if changing the trace buffer size is supported. This step is
+# repeated twice. This helps in case the trace buffer size is 100.
+set test_size 100
+set test "change buffer size to $test_size"
+gdb_test_multiple "set trace-buffer-size $test_size" $test {
+ -re ".*Target does not support this command.*$gdb_prompt $" {
+ unsupported "target does not support changing trace buffer size"
return 1
}
+ -re "$gdb_prompt $" {
+ pass $test
+ }
+}
- return 0
+set test "check whether setting trace buffer size is supported"
+gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*$gdb_prompt $" {
+ set total_size $expect_out(2,string)
+ if { $test_size != $total_size } {
+ unsupported "target does not support changing trace buffer size"
+ return 1
+ }
+ pass $test
+ }
}
-# 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"
- return 1
+set test_size 400
+gdb_test_no_output "set trace-buffer-size $test_size" \
+ "change buffer size to $test_size"
+
+gdb_test_multiple "tstatus" $test {
+ -re ".*Trace buffer has ($decimal) bytes of ($decimal) bytes free.*$gdb_prompt $" {
+ set total_size $expect_out(2,string)
+ if { $test_size != $total_size } {
+ unsupported "target does not support changing trace buffer size"
+ return 1
+ }
+ pass $test
}
+}
+
+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)"
- if [trace_buffer_normal] then { return 1 }
+set total_size -1
+set free_size -1
+set frame_size -1
- gdb_test "break begin" ".*" ""
- gdb_test "break end" ".*" ""
- gdb_test "tstop" ".*" ""
- gdb_test "tfind none" ".*" ""
+# Determine the size used by a single frame. Set a single tracepoint,
+# run and then check the total and free size using the tstatus command.
+# Then subtracting free from total gives us the size of a frame.
+with_test_prefix "frame size" {
+ set_a_tracepoint func0
- if [setup_tracepoints] then { return 1 }
+ gdb_test "break end" "Breakpoint $decimal.*" "breakpoint at end"
- # 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 "tstart" "\[\r\n\]*" "start trace"
- if [gdb_test "tfind 9" \
- "#0 func9 .*" \
- "find frame nine, pass 1"] then { return 1 }
+ gdb_test "continue" "Continuing.*Breakpoint \[0-9\]+, end.*" \
+ "run to end"
- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 1"] then { return 1 }
+ gdb_test "tstop" "\[\r\n\]*" "stop trace"
- # 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 {
+ 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
+ }
+ }
+
+ # Check that we get the total_size and free_size.
+ if { $total_size < 0 } {
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 { $free_size < 0 } {
+ return 1
+ }
+}
- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 2"] then { return 1 }
+# Calculate the size of a single frame.
+set frame_size "($total_size - $free_size)"
- # 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 {
+with_test_prefix "normal buffer" {
+ clean_restart $testfile
+
+ 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 }
- if [gdb_test "tfind none" \
- "#0 end .*" \
- "quit trace debugging, pass 3"] then { return 1 }
+ run_trace_experiment
- return 0
+ # Check that the first frame is actually at func0.
+ gdb_test "tfind start" ".*#0 func0 .*" \
+ "first frame is at func0"
+
+ gdb_test "tfind pc func9" \
+ ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ "find frame for func9"
}
-gdb_test_no_output "set circular-trace-buffer on" \
- "set circular-trace-buffer on"
+# Shrink the trace buffer so that it will not hold
+# all ten trace frames. Verify that the frame for func0 is still
+# collected, but the frame for func9 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
+ }
-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"
+ run_trace_experiment
-gdb_test "show circular-trace-buffer" "Target's use of circular trace buffer is off." "show circular-trace-buffer (off)"
+ gdb_test "tfind start" ".*#0 func0 .*" \
+ "first frame is at func0"
-# 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 "tfind pc func9" ".* failed to find .*" \
+ "find frame for func9"
}
-# Finished!
-gdb_test "tfind none" ".*" ""
+# 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) the first frame will be overwritten and therefore unavailable.
+# 2) the earliest frame in the buffer will not be for func0.
+# 3) the frame for func9 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_no_output "set trace-buffer-size $buffer_size" \
+ "shrink the target trace buffer"
+
+ gdb_test_no_output "set circular-trace-buffer on" \
+ "make the target trace buffer circular"
+
+ run_trace_experiment
+
+ gdb_test "tstatus" \
+ ".*Buffer contains $decimal trace frames \\(of $decimal created total\\).*Trace buffer is circular.*" \
+ "trace buffer is circular"
+
+ # The first frame should not be at func0.
+ gdb_test "tfind start" ".*#0 func\[1-9\] .*" \
+ "first frame is NOT at func0"
+
+ gdb_test \
+ "tfind pc func9" \
+ ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ "find frame for func9"
+}
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] "tfind" across unavailable-stack frames.
2013-04-18 10:30 ` Pedro Alves
2013-04-18 11:09 ` Pedro Alves
@ 2013-12-13 17:49 ` Pedro Alves
2013-12-14 6:23 ` Yao Qi
2013-12-16 8:40 ` Metzger, Markus T
1 sibling, 2 replies; 23+ messages in thread
From: Pedro Alves @ 2013-12-13 17:49 UTC (permalink / raw)
To: gdb-patches
As discussed in the sub-thread starting at:
https://sourceware.org/ml/gdb-patches/2013-12/msg00528.html
I've cleaned up this patch, and adjusted the testsuite.
The original was here:
https://sourceware.org/ml/gdb-patches/2013-04/msg00541.html
In this version, I made the unwind->this_id() be responsible
for using frame_id_build_unavailable_stack instead. That
allows the unwinder using perhaps a more detailed frame_id
(it sounds like record-btrace will want that.), and allows
avoiding get_frame_func when the might not be available.
Comments?
-------------
"tfind" across unavailable-stack frames.
Like when stepping, the current stack frame location is expected to be
printed as result of tfind command, if that results in moving to a
different function. In tfind_1 we see:
if (from_tty
&& (has_stack_frames () || traceframe_number >= 0))
{
enum print_what print_what;
/* NOTE: in imitation of the step command, try to determine
whether we have made a transition from one function to
another. If so, we'll print the "stack frame" (ie. the new
function and it's arguments) -- otherwise we'll just show the
new source line. */
if (frame_id_eq (old_frame_id,
get_frame_id (get_current_frame ())))
print_what = SRC_LINE;
else
print_what = SRC_AND_LOC;
print_stack_frame (get_selected_frame (NULL), 1, print_what, 1);
do_displays ();
}
However, when we haven't collected any registers in the tracepoint
(collect $regs), that doesn't actually work:
(gdb) tstart
(gdb) info tracepoints
Num Type Disp Enb Address What
1 tracepoint keep y 0x080483b7 in func0
at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
collect testload
installed on target
2 tracepoint keep y 0x080483bc in func1
at ../.././../git/gdb/testsuite/gdb.trace/circ.c:32
collect testload
installed on target
(gdb) c
Continuing.
Breakpoint 3, end () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:72
72 }
(gdb) tstop
(gdb) tfind start
Found trace frame 0, tracepoint 1
#0 func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
28 }
(gdb) tfind
Found trace frame 1, tracepoint 2
32 }
(gdb)
When we don't have info about the stack available
(UNWIND_UNAVAILABLE), frames end up with outer_frame_id as frame ID.
And in the scenario above, the issue is that both frames before and
after the second tfind (the frames for func0 an func1) have the same
id (outer_frame_id), so the frame_id_eq check returns false, even
though the frames were of different functions. GDB knows that,
because the PC is inferred from the tracepoint's address, even if no
registers were collected.
To fix this, this patch adds support for frame ids with a valid code
address, but <unavailable> stack address, and then makes the unwinders
use that instead of the catch-all outer_frame_id for such frames. The
frame_id_eq check in tfind_1 then automatically does the right thing
as expected.
I tested with --directory=gdb.trace/ , before/after the patch, and
compared the resulting gdb.logs, then adjusted the tests to expect the
extra output that came out. Turns out that was only circ.exp, the
original test that actually brought this issue to light.
Tested on x86_64 Fedora 17, native and gdbserver.
gdb/
2013-12-13 Pedro Alves <palves@redhat.com>
* frame.h (enum frame_id_stack_status): New enum.
(struct frame_id) <stack_addr>: Adjust comment.
<stack_addr_p>: Delete field, replaced with ...
<stack_status>: ... this new field.
(frame_id_build_unavailable_stack): Declare.
* frame.c (frame_addr_hash, fprint_field, outer_frame_id)
(frame_id_build_special): Adjust.
(frame_id_build_unavailable_stack): New function.
(frame_id_build, frame_id_build_wild): Adjust.
(frame_id_p, frame_id_eq, frame_id_inner): Adjust to take into
account frames with unavailable stack.
* amd64-tdep.c (amd64_frame_this_id)
(amd64_sigtramp_frame_this_id, amd64_epilogue_frame_this_id): Use
frame_id_build_unavailable_stack.
* dwarf2-frame.c (dwarf2_frame_this_id): Likewise.
* i386-tdep.c (i386_frame_this_id, i386_epilogue_frame_this_id)
(i386_sigtramp_frame_this_id): Likewise.
gdb/testsuite/
2013-12-13 Pedro Alves <palves@redhat.com>
* gdb.trace/circ.exp: Expect frame info to be printed when
switching between frames with unavailable stack, but different
functions.
---
gdb/amd64-tdep.c | 32 ++++++++++++++----------
gdb/dwarf2-frame.c | 9 +++----
gdb/frame.c | 54 +++++++++++++++++++++++++++++-----------
gdb/frame.h | 30 +++++++++++++++++++---
gdb/i386-tdep.c | 33 ++++++++++++++----------
gdb/testsuite/gdb.trace/circ.exp | 4 +--
6 files changed, 112 insertions(+), 50 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 19968fc..d0dc587 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2378,13 +2378,14 @@ amd64_frame_this_id (struct frame_info *this_frame, void **this_cache,
amd64_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- /* This marks the outermost frame. */
- if (cache->base == 0)
- return;
-
- (*this_id) = frame_id_build (cache->base + 16, cache->pc);
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else if (cache->base == 0)
+ {
+ /* This marks the outermost frame. */
+ return;
+ }
+ else
+ (*this_id) = frame_id_build (cache->base + 16, cache->pc);
}
static struct value *
@@ -2499,9 +2500,14 @@ amd64_sigtramp_frame_this_id (struct frame_info *this_frame,
amd64_sigtramp_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- (*this_id) = frame_id_build (cache->base + 16, get_frame_pc (this_frame));
+ (*this_id) = frame_id_build_unavailable_stack (get_frame_pc (this_frame));
+ else if (cache->base == 0)
+ {
+ /* This marks the outermost frame. */
+ return;
+ }
+ else
+ (*this_id) = frame_id_build (cache->base + 16, get_frame_pc (this_frame));
}
static struct value *
@@ -2670,9 +2676,9 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
this_cache);
if (!cache->base_p)
- return;
-
- (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else
+ (*this_id) = frame_id_build (cache->base + 8, cache->pc);
}
static const struct frame_unwind amd64_epilogue_frame_unwind =
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index c4f8771..91b2120 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1275,12 +1275,11 @@ dwarf2_frame_this_id (struct frame_info *this_frame, void **this_cache,
dwarf2_frame_cache (this_frame, this_cache);
if (cache->unavailable_retaddr)
+ (*this_id) = frame_id_build_unavailable_stack (get_frame_func (this_frame));
+ else if (cache->undefined_retaddr)
return;
-
- if (cache->undefined_retaddr)
- return;
-
- (*this_id) = frame_id_build (cache->cfa, get_frame_func (this_frame));
+ else
+ (*this_id) = frame_id_build (cache->cfa, get_frame_func (this_frame));
}
static struct value *
diff --git a/gdb/frame.c b/gdb/frame.c
index 6a8b5ae..ed31c9e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -166,10 +166,11 @@ frame_addr_hash (const void *ap)
const struct frame_id f_id = frame->this_id.value;
hashval_t hash = 0;
- gdb_assert (f_id.stack_addr_p || f_id.code_addr_p
+ gdb_assert (f_id.stack_status != FID_STACK_INVALID
+ || f_id.code_addr_p
|| f_id.special_addr_p);
- if (f_id.stack_addr_p)
+ if (f_id.stack_status == FID_STACK_VALID)
hash = iterative_hash (&f_id.stack_addr,
sizeof (f_id.stack_addr), hash);
if (f_id.code_addr_p)
@@ -317,13 +318,23 @@ void
fprint_frame_id (struct ui_file *file, struct frame_id id)
{
fprintf_unfiltered (file, "{");
- fprint_field (file, "stack", id.stack_addr_p, id.stack_addr);
+
+ if (id.stack_status == FID_STACK_INVALID)
+ fprintf_unfiltered (file, "!stack");
+ else if (id.stack_status == FID_STACK_UNAVAILABLE)
+ fprintf_unfiltered (file, "stack=<unavailable>");
+ else
+ fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
fprintf_unfiltered (file, ",");
+
fprint_field (file, "code", id.code_addr_p, id.code_addr);
fprintf_unfiltered (file, ",");
+
fprint_field (file, "special", id.special_addr_p, id.special_addr);
+
if (id.artificial_depth)
fprintf_unfiltered (file, ",artificial=%d", id.artificial_depth);
+
fprintf_unfiltered (file, "}");
}
@@ -487,7 +498,7 @@ frame_unwind_caller_id (struct frame_info *next_frame)
}
const struct frame_id null_frame_id; /* All zeros. */
-const struct frame_id outer_frame_id = { 0, 0, 0, 0, 0, 1, 0 };
+const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_INVALID, 0, 1, 0 };
struct frame_id
frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
@@ -496,7 +507,7 @@ frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
struct frame_id id = null_frame_id;
id.stack_addr = stack_addr;
- id.stack_addr_p = 1;
+ id.stack_status = FID_STACK_VALID;
id.code_addr = code_addr;
id.code_addr_p = 1;
id.special_addr = special_addr;
@@ -504,13 +515,26 @@ frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
return id;
}
+/* See frame.h. */
+
+struct frame_id
+frame_id_build_unavailable_stack (CORE_ADDR code_addr)
+{
+ struct frame_id id = null_frame_id;
+
+ id.stack_status = FID_STACK_UNAVAILABLE;
+ id.code_addr = code_addr;
+ id.code_addr_p = 1;
+ return id;
+}
+
struct frame_id
frame_id_build (CORE_ADDR stack_addr, CORE_ADDR code_addr)
{
struct frame_id id = null_frame_id;
id.stack_addr = stack_addr;
- id.stack_addr_p = 1;
+ id.stack_status = FID_STACK_VALID;
id.code_addr = code_addr;
id.code_addr_p = 1;
return id;
@@ -522,7 +546,7 @@ frame_id_build_wild (CORE_ADDR stack_addr)
struct frame_id id = null_frame_id;
id.stack_addr = stack_addr;
- id.stack_addr_p = 1;
+ id.stack_status = FID_STACK_VALID;
return id;
}
@@ -532,7 +556,7 @@ frame_id_p (struct frame_id l)
int p;
/* The frame is valid iff it has a valid stack address. */
- p = l.stack_addr_p;
+ p = l.stack_status != FID_STACK_INVALID;
/* outer_frame_id is also valid. */
if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
p = 1;
@@ -559,19 +583,20 @@ frame_id_eq (struct frame_id l, struct frame_id r)
{
int eq;
- if (!l.stack_addr_p && l.special_addr_p
- && !r.stack_addr_p && r.special_addr_p)
+ if (l.stack_status == FID_STACK_INVALID && l.special_addr_p
+ && r.stack_status == FID_STACK_INVALID && r.special_addr_p)
/* The outermost frame marker is equal to itself. This is the
dodgy thing about outer_frame_id, since between execution steps
we might step into another function - from which we can't
unwind either. More thought required to get rid of
outer_frame_id. */
eq = 1;
- else if (!l.stack_addr_p || !r.stack_addr_p)
+ else if (l.stack_status == FID_STACK_INVALID
+ || l.stack_status == FID_STACK_INVALID)
/* Like a NaN, if either ID is invalid, the result is false.
Note that a frame ID is invalid iff it is the null frame ID. */
eq = 0;
- else if (l.stack_addr != r.stack_addr)
+ else if (l.stack_status != r.stack_status || l.stack_addr != r.stack_addr)
/* If .stack addresses are different, the frames are different. */
eq = 0;
else if (l.code_addr_p && r.code_addr_p && l.code_addr != r.code_addr)
@@ -638,8 +663,9 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
{
int inner;
- if (!l.stack_addr_p || !r.stack_addr_p)
- /* Like NaN, any operation involving an invalid ID always fails. */
+ if (l.stack_status != FID_STACK_VALID || r.stack_status != FID_STACK_VALID)
+ /* Like NaN, any operation involving an invalid ID always fails.
+ Likewise if either ID has an unavailable stack address. */
inner = 0;
else if (l.artificial_depth > r.artificial_depth
&& l.stack_addr == r.stack_addr
diff --git a/gdb/frame.h b/gdb/frame.h
index f8d5bc1..8f6429a 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -76,6 +76,24 @@ struct block;
struct gdbarch;
struct ui_file;
+/* Status of a given frame's stack. */
+
+enum frame_id_stack_status
+{
+ /* Stack address is invalid. E.g., This frame is the outermost
+ (i.e., _start), and the stack hasn't been setup yet. */
+ FID_STACK_INVALID = 0,
+
+ /* Stack address is valid, and the address is found in the
+ corresponding ..._addr field. */
+ FID_STACK_VALID = 1,
+
+ /* Stack address is unavailable. I.e., there's a valid stack, but
+ we don't know where it is (because memory or registers we'd
+ compute it from were not collected). */
+ FID_STACK_UNAVAILABLE = -1
+};
+
/* The frame object. */
struct frame_info;
@@ -97,8 +115,8 @@ struct frame_id
function pointer register or stack pointer register. They are
wrong.
- This field is valid only if stack_addr_p is true. Otherwise, this
- frame represents the null frame. */
+ This field is valid only if frame_id.stack_status is
+ FID_STACK_VALID. */
CORE_ADDR stack_addr;
/* The frame's code address. This shall be constant through out the
@@ -129,7 +147,7 @@ struct frame_id
CORE_ADDR special_addr;
/* Flags to indicate the above fields have valid contents. */
- unsigned int stack_addr_p : 1;
+ ENUM_BITFIELD(frame_id_stack_status) stack_status : 2;
unsigned int code_addr_p : 1;
unsigned int special_addr_p : 1;
@@ -169,6 +187,12 @@ extern struct frame_id frame_id_build_special (CORE_ADDR stack_addr,
CORE_ADDR code_addr,
CORE_ADDR special_addr);
+/* Construct a frame ID representing a frame where the stack address
+ exists, but is unavailable. The first parameter is the frame's
+ constant code address (typically the entry point). The special
+ identifier address is set to indicate a wild card. */
+extern struct frame_id frame_id_build_unavailable_stack (CORE_ADDR code_addr);
+
/* Construct a wild card frame ID. The parameter is the frame's constant
stack address (typically the outer-bound). The code address as well
as the special identifier address are set to indicate wild cards. */
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index a1a4453..5b4a5a3 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1905,12 +1905,17 @@ i386_frame_this_id (struct frame_info *this_frame, void **this_cache,
{
struct i386_frame_cache *cache = i386_frame_cache (this_frame, this_cache);
- /* This marks the outermost frame. */
- if (cache->base == 0)
- return;
-
- /* See the end of i386_push_dummy_call. */
- (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ if (!cache->base_p)
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else if (cache->base == 0)
+ {
+ /* This marks the outermost frame. */
+ }
+ else
+ {
+ /* See the end of i386_push_dummy_call. */
+ (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ }
}
static enum unwind_stop_reason
@@ -2091,9 +2096,9 @@ i386_epilogue_frame_this_id (struct frame_info *this_frame,
i386_epilogue_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else
+ (*this_id) = frame_id_build (cache->base + 8, cache->pc);
}
static struct value *
@@ -2284,10 +2289,12 @@ i386_sigtramp_frame_this_id (struct frame_info *this_frame, void **this_cache,
i386_sigtramp_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- /* See the end of i386_push_dummy_call. */
- (*this_id) = frame_id_build (cache->base + 8, get_frame_pc (this_frame));
+ (*this_id) = frame_id_build_unavailable_stack (get_frame_pc (this_frame));
+ else
+ {
+ /* See the end of i386_push_dummy_call. */
+ (*this_id) = frame_id_build (cache->base + 8, get_frame_pc (this_frame));
+ }
}
static struct value *
diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/circ.exp
index f605bb6..6246e35 100644
--- a/gdb/testsuite/gdb.trace/circ.exp
+++ b/gdb/testsuite/gdb.trace/circ.exp
@@ -221,7 +221,7 @@ with_test_prefix "normal buffer" {
"first frame is at func0"
gdb_test "tfind pc func9" \
- ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ ".*Found trace frame $decimal, tracepoint $decimal\r\n#0 func9 .*" \
"find frame for func9"
}
@@ -282,6 +282,6 @@ with_test_prefix "circular buffer" {
gdb_test \
"tfind pc func9" \
- ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ ".*Found trace frame $decimal, tracepoint $decimal\r\n#0 func9 .*" \
"find frame for func9"
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-13 17:49 ` [PATCH] "tfind" across unavailable-stack frames Pedro Alves
@ 2013-12-14 6:23 ` Yao Qi
2013-12-16 16:19 ` Pedro Alves
2013-12-16 8:40 ` Metzger, Markus T
1 sibling, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-12-14 6:23 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 12/14/2013 01:49 AM, Pedro Alves wrote:
> gdb/
> 2013-12-13 Pedro Alves <palves@redhat.com>
>
> * frame.h (enum frame_id_stack_status): New enum.
> (struct frame_id) <stack_addr>: Adjust comment.
> <stack_addr_p>: Delete field, replaced with ...
> <stack_status>: ... this new field.
> (frame_id_build_unavailable_stack): Declare.
> * frame.c (frame_addr_hash, fprint_field, outer_frame_id)
> (frame_id_build_special): Adjust.
> (frame_id_build_unavailable_stack): New function.
> (frame_id_build, frame_id_build_wild): Adjust.
> (frame_id_p, frame_id_eq, frame_id_inner): Adjust to take into
> account frames with unavailable stack.
>
> * amd64-tdep.c (amd64_frame_this_id)
> (amd64_sigtramp_frame_this_id, amd64_epilogue_frame_this_id): Use
> frame_id_build_unavailable_stack.
> * dwarf2-frame.c (dwarf2_frame_this_id): Likewise.
Do we need to update tailcall_frame_this_id as well? I can't find an
easy way to reproduce it on tailcall frame (using
gdb.arch/amd64-tailcall-* cases).
Patch looks right to me, two nits on comments below,
> @@ -169,6 +187,12 @@ extern struct frame_id frame_id_build_special (CORE_ADDR stack_addr,
> CORE_ADDR code_addr,
> CORE_ADDR special_addr);
>
> +/* Construct a frame ID representing a frame where the stack address
> + exists, but is unavailable. The first parameter is the frame's
Remove "first"? since we only have one parameter here.
> + constant code address (typically the entry point). The special
> + identifier address is set to indicate a wild card. */
> +extern struct frame_id frame_id_build_unavailable_stack (CORE_ADDR code_addr);
> +
What does the last sentence mean in the comments?
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH] "tfind" across unavailable-stack frames.
2013-12-13 17:49 ` [PATCH] "tfind" across unavailable-stack frames Pedro Alves
2013-12-14 6:23 ` Yao Qi
@ 2013-12-16 8:40 ` Metzger, Markus T
2013-12-16 16:25 ` Pedro Alves
1 sibling, 1 reply; 23+ messages in thread
From: Metzger, Markus T @ 2013-12-16 8:40 UTC (permalink / raw)
To: Pedro Alves, gdb-patches
> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Pedro Alves
> Sent: Friday, December 13, 2013 6:50 PM
> +/* See frame.h. */
> +
> +struct frame_id
> +frame_id_build_unavailable_stack (CORE_ADDR code_addr)
> +{
> + struct frame_id id = null_frame_id;
> +
> + id.stack_status = FID_STACK_UNAVAILABLE;
> + id.code_addr = code_addr;
> + id.code_addr_p = 1;
> + return id;
> +}
For record-btrace, we would also need to allow special_addr.
I can also add another build function for this case.
> struct frame_info;
> @@ -97,8 +115,8 @@ struct frame_id
> function pointer register or stack pointer register. They are
> wrong.
>
> - This field is valid only if stack_addr_p is true. Otherwise, this
> - frame represents the null frame. */
> + This field is valid only if frame_id.stack_status is
> + FID_STACK_VALID. */
> CORE_ADDR stack_addr;
Maybe the comment should say that this field must be zero unless
stack_status == FID_STACK_VALID.
Frame_id_eq would compare stack_addr if stack_status compares
equal.
Regards,
Markus.
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-14 6:23 ` Yao Qi
@ 2013-12-16 16:19 ` Pedro Alves
2013-12-17 9:04 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-12-16 16:19 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 12/14/2013 06:21 AM, Yao Qi wrote:
> On 12/14/2013 01:49 AM, Pedro Alves wrote:
>> gdb/
>> 2013-12-13 Pedro Alves <palves@redhat.com>
>>
>> * frame.h (enum frame_id_stack_status): New enum.
>> (struct frame_id) <stack_addr>: Adjust comment.
>> <stack_addr_p>: Delete field, replaced with ...
>> <stack_status>: ... this new field.
>> (frame_id_build_unavailable_stack): Declare.
>> * frame.c (frame_addr_hash, fprint_field, outer_frame_id)
>> (frame_id_build_special): Adjust.
>> (frame_id_build_unavailable_stack): New function.
>> (frame_id_build, frame_id_build_wild): Adjust.
>> (frame_id_p, frame_id_eq, frame_id_inner): Adjust to take into
>> account frames with unavailable stack.
>>
>> * amd64-tdep.c (amd64_frame_this_id)
>> (amd64_sigtramp_frame_this_id, amd64_epilogue_frame_this_id): Use
>> frame_id_build_unavailable_stack.
>> * dwarf2-frame.c (dwarf2_frame_this_id): Likewise.
>
> Do we need to update tailcall_frame_this_id as well?
In case of trace frame debugging, I don't think you can ever
have a tailcall frame on top of a frame with unavailable
stack, because dwarf2_tailcall_sniffer_first will throw
an error computing prev_sp. But even if that would work
somehow:
static void
tailcall_frame_this_id (struct frame_info *this_frame, void **this_cache,
struct frame_id *this_id)
{
struct tailcall_cache *cache = *this_cache;
struct frame_info *next_frame;
/* Tail call does not make sense for a sentinel frame. */
next_frame = get_next_frame (this_frame);
gdb_assert (next_frame != NULL);
*this_id = get_frame_id (next_frame);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
(*this_id).code_addr = get_frame_pc (this_frame);
(*this_id).code_addr_p = 1;
(*this_id).artificial_depth = (cache->chain_levels
- existing_next_levels (this_frame, cache));
gdb_assert ((*this_id).artificial_depth > 0);
}
... the tailcall frame's ID's stack components (address
and status) are copied from the next frame's stack components.
Seems right to me.
>> @@ -169,6 +187,12 @@ extern struct frame_id frame_id_build_special (CORE_ADDR stack_addr,
>> CORE_ADDR code_addr,
>> CORE_ADDR special_addr);
>>
>> +/* Construct a frame ID representing a frame where the stack address
>> + exists, but is unavailable. The first parameter is the frame's
>
> Remove "first"? since we only have one parameter here.
Indeed. Fixed locally.
>
>> + constant code address (typically the entry point). The special
>> + identifier address is set to indicate a wild card. */
>> +extern struct frame_id frame_id_build_unavailable_stack (CORE_ADDR code_addr);
>> +
>
> What does the last sentence mean in the comments?
>
The same comment is in frame_id_build. Re. what is means, see struct frame_id:
/* The frame's special address. This shall be constant through out the
lifetime of the frame. This is used for architectures that may have
frames that do not change the stack but are still distinct and have
some form of distinct identifier (e.g. the ia64 which uses a 2nd
stack for registers). This field is treated as unordered - i.e. will
not be used in frame ordering comparisons.
This field is valid only if special_addr_p is true. Otherwise, this
frame is considered to have a wildcard special address, i.e. one that
^^^^^^^^^^^^^
matches every address value in frame comparisons. */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
CORE_ADDR special_addr;
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-16 8:40 ` Metzger, Markus T
@ 2013-12-16 16:25 ` Pedro Alves
2013-12-16 16:42 ` [PATCH v2] " Pedro Alves
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-12-16 16:25 UTC (permalink / raw)
To: Metzger, Markus T; +Cc: gdb-patches
On 12/16/2013 08:39 AM, Metzger, Markus T wrote:
>> -----Original Message-----
>> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
>> owner@sourceware.org] On Behalf Of Pedro Alves
>> Sent: Friday, December 13, 2013 6:50 PM
>
>
>> +/* See frame.h. */
>> +
>> +struct frame_id
>> +frame_id_build_unavailable_stack (CORE_ADDR code_addr)
>> +{
>> + struct frame_id id = null_frame_id;
>> +
>> + id.stack_status = FID_STACK_UNAVAILABLE;
>> + id.code_addr = code_addr;
>> + id.code_addr_p = 1;
>> + return id;
>> +}
>
> For record-btrace, we would also need to allow special_addr.
> I can also add another build function for this case.
Yes, quite similar to the one you were adding.
>> struct frame_info;
>> @@ -97,8 +115,8 @@ struct frame_id
>> function pointer register or stack pointer register. They are
>> wrong.
>>
>> - This field is valid only if stack_addr_p is true. Otherwise, this
>> - frame represents the null frame. */
>> + This field is valid only if frame_id.stack_status is
>> + FID_STACK_VALID. */
>> CORE_ADDR stack_addr;
>
> Maybe the comment should say that this field must be zero unless
> stack_status == FID_STACK_VALID.
>
> Frame_id_eq would compare stack_addr if stack_status compares
> equal.
Indeed. I'll do that. I had considered changing frame_id_eq
to only compare stack_addr if stack_status == FID_STACK_VALID,
but then since in practice we always centrally control what we
put in stack_addr, and put 0 zero anyway, might as well just
document that, and avoid ever seeing confusing garbage there.
Thanks,
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2] "tfind" across unavailable-stack frames.
2013-12-16 16:25 ` Pedro Alves
@ 2013-12-16 16:42 ` Pedro Alves
0 siblings, 0 replies; 23+ messages in thread
From: Pedro Alves @ 2013-12-16 16:42 UTC (permalink / raw)
To: gdb-patches; +Cc: Metzger, Markus T, Yao Qi
v2: Updates comments per Yao's and Markus' reviews.
---------
"tfind" across unavailable-stack frames.
Like when stepping, the current stack frame location is expected to be
printed as result of tfind command, if that results in moving to a
different function. In tfind_1 we see:
if (from_tty
&& (has_stack_frames () || traceframe_number >= 0))
{
enum print_what print_what;
/* NOTE: in imitation of the step command, try to determine
whether we have made a transition from one function to
another. If so, we'll print the "stack frame" (ie. the new
function and it's arguments) -- otherwise we'll just show the
new source line. */
if (frame_id_eq (old_frame_id,
get_frame_id (get_current_frame ())))
print_what = SRC_LINE;
else
print_what = SRC_AND_LOC;
print_stack_frame (get_selected_frame (NULL), 1, print_what, 1);
do_displays ();
}
However, when we haven't collected any registers in the tracepoint
(collect $regs), that doesn't actually work:
(gdb) tstart
(gdb) info tracepoints
Num Type Disp Enb Address What
1 tracepoint keep y 0x080483b7 in func0
at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
collect testload
installed on target
2 tracepoint keep y 0x080483bc in func1
at ../.././../git/gdb/testsuite/gdb.trace/circ.c:32
collect testload
installed on target
(gdb) c
Continuing.
Breakpoint 3, end () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:72
72 }
(gdb) tstop
(gdb) tfind start
Found trace frame 0, tracepoint 1
#0 func0 () at ../.././../git/gdb/testsuite/gdb.trace/circ.c:28
28 }
(gdb) tfind
Found trace frame 1, tracepoint 2
32 }
(gdb)
When we don't have info about the stack available
(UNWIND_UNAVAILABLE), frames end up with outer_frame_id as frame ID.
And in the scenario above, the issue is that both frames before and
after the second tfind (the frames for func0 an func1) have the same
id (outer_frame_id), so the frame_id_eq check returns false, even
though the frames were of different functions. GDB knows that,
because the PC is inferred from the tracepoint's address, even if no
registers were collected.
To fix this, this patch adds support for frame ids with a valid code
address, but <unavailable> stack address, and then makes the unwinders
use that instead of the catch-all outer_frame_id for such frames. The
frame_id_eq check in tfind_1 then automatically does the right thing
as expected.
I tested with --directory=gdb.trace/ , before/after the patch, and
compared the resulting gdb.logs, then adjusted the tests to expect the
extra output that came out. Turns out that was only circ.exp, the
original test that actually brought this issue to light.
Tested on x86_64 Fedora 17, native and gdbserver.
gdb/
2013-12-13 Pedro Alves <palves@redhat.com>
* frame.h (enum frame_id_stack_status): New enum.
(struct frame_id) <stack_addr>: Adjust comment.
<stack_addr_p>: Delete field, replaced with ...
<stack_status>: ... this new field.
(frame_id_build_unavailable_stack): Declare.
* frame.c (frame_addr_hash, fprint_field, outer_frame_id)
(frame_id_build_special): Adjust.
(frame_id_build_unavailable_stack): New function.
(frame_id_build, frame_id_build_wild): Adjust.
(frame_id_p, frame_id_eq, frame_id_inner): Adjust to take into
account frames with unavailable stack.
* amd64-tdep.c (amd64_frame_this_id)
(amd64_sigtramp_frame_this_id, amd64_epilogue_frame_this_id): Use
frame_id_build_unavailable_stack.
* dwarf2-frame.c (dwarf2_frame_this_id): Likewise.
* i386-tdep.c (i386_frame_this_id, i386_epilogue_frame_this_id)
(i386_sigtramp_frame_this_id): Likewise.
gdb/testsuite/
2013-12-13 Pedro Alves <palves@redhat.com>
* gdb.trace/circ.exp: Expect frame info to be printed when
switching between frames with unavailable stack, but different
functions.
---
gdb/amd64-tdep.c | 32 ++++++++++++++----------
gdb/dwarf2-frame.c | 9 +++----
gdb/frame.c | 54 +++++++++++++++++++++++++++++-----------
gdb/frame.h | 30 +++++++++++++++++++---
gdb/i386-tdep.c | 33 ++++++++++++++----------
gdb/testsuite/gdb.trace/circ.exp | 4 +--
6 files changed, 112 insertions(+), 50 deletions(-)
diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 19968fc..d0dc587 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -2378,13 +2378,14 @@ amd64_frame_this_id (struct frame_info *this_frame, void **this_cache,
amd64_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- /* This marks the outermost frame. */
- if (cache->base == 0)
- return;
-
- (*this_id) = frame_id_build (cache->base + 16, cache->pc);
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else if (cache->base == 0)
+ {
+ /* This marks the outermost frame. */
+ return;
+ }
+ else
+ (*this_id) = frame_id_build (cache->base + 16, cache->pc);
}
static struct value *
@@ -2499,9 +2500,14 @@ amd64_sigtramp_frame_this_id (struct frame_info *this_frame,
amd64_sigtramp_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- (*this_id) = frame_id_build (cache->base + 16, get_frame_pc (this_frame));
+ (*this_id) = frame_id_build_unavailable_stack (get_frame_pc (this_frame));
+ else if (cache->base == 0)
+ {
+ /* This marks the outermost frame. */
+ return;
+ }
+ else
+ (*this_id) = frame_id_build (cache->base + 16, get_frame_pc (this_frame));
}
static struct value *
@@ -2670,9 +2676,9 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
this_cache);
if (!cache->base_p)
- return;
-
- (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else
+ (*this_id) = frame_id_build (cache->base + 8, cache->pc);
}
static const struct frame_unwind amd64_epilogue_frame_unwind =
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index c4f8771..91b2120 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -1275,12 +1275,11 @@ dwarf2_frame_this_id (struct frame_info *this_frame, void **this_cache,
dwarf2_frame_cache (this_frame, this_cache);
if (cache->unavailable_retaddr)
+ (*this_id) = frame_id_build_unavailable_stack (get_frame_func (this_frame));
+ else if (cache->undefined_retaddr)
return;
-
- if (cache->undefined_retaddr)
- return;
-
- (*this_id) = frame_id_build (cache->cfa, get_frame_func (this_frame));
+ else
+ (*this_id) = frame_id_build (cache->cfa, get_frame_func (this_frame));
}
static struct value *
diff --git a/gdb/frame.c b/gdb/frame.c
index 6a8b5ae..ed31c9e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -166,10 +166,11 @@ frame_addr_hash (const void *ap)
const struct frame_id f_id = frame->this_id.value;
hashval_t hash = 0;
- gdb_assert (f_id.stack_addr_p || f_id.code_addr_p
+ gdb_assert (f_id.stack_status != FID_STACK_INVALID
+ || f_id.code_addr_p
|| f_id.special_addr_p);
- if (f_id.stack_addr_p)
+ if (f_id.stack_status == FID_STACK_VALID)
hash = iterative_hash (&f_id.stack_addr,
sizeof (f_id.stack_addr), hash);
if (f_id.code_addr_p)
@@ -317,13 +318,23 @@ void
fprint_frame_id (struct ui_file *file, struct frame_id id)
{
fprintf_unfiltered (file, "{");
- fprint_field (file, "stack", id.stack_addr_p, id.stack_addr);
+
+ if (id.stack_status == FID_STACK_INVALID)
+ fprintf_unfiltered (file, "!stack");
+ else if (id.stack_status == FID_STACK_UNAVAILABLE)
+ fprintf_unfiltered (file, "stack=<unavailable>");
+ else
+ fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
fprintf_unfiltered (file, ",");
+
fprint_field (file, "code", id.code_addr_p, id.code_addr);
fprintf_unfiltered (file, ",");
+
fprint_field (file, "special", id.special_addr_p, id.special_addr);
+
if (id.artificial_depth)
fprintf_unfiltered (file, ",artificial=%d", id.artificial_depth);
+
fprintf_unfiltered (file, "}");
}
@@ -487,7 +498,7 @@ frame_unwind_caller_id (struct frame_info *next_frame)
}
const struct frame_id null_frame_id; /* All zeros. */
-const struct frame_id outer_frame_id = { 0, 0, 0, 0, 0, 1, 0 };
+const struct frame_id outer_frame_id = { 0, 0, 0, FID_STACK_INVALID, 0, 1, 0 };
struct frame_id
frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
@@ -496,7 +507,7 @@ frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
struct frame_id id = null_frame_id;
id.stack_addr = stack_addr;
- id.stack_addr_p = 1;
+ id.stack_status = FID_STACK_VALID;
id.code_addr = code_addr;
id.code_addr_p = 1;
id.special_addr = special_addr;
@@ -504,13 +515,26 @@ frame_id_build_special (CORE_ADDR stack_addr, CORE_ADDR code_addr,
return id;
}
+/* See frame.h. */
+
+struct frame_id
+frame_id_build_unavailable_stack (CORE_ADDR code_addr)
+{
+ struct frame_id id = null_frame_id;
+
+ id.stack_status = FID_STACK_UNAVAILABLE;
+ id.code_addr = code_addr;
+ id.code_addr_p = 1;
+ return id;
+}
+
struct frame_id
frame_id_build (CORE_ADDR stack_addr, CORE_ADDR code_addr)
{
struct frame_id id = null_frame_id;
id.stack_addr = stack_addr;
- id.stack_addr_p = 1;
+ id.stack_status = FID_STACK_VALID;
id.code_addr = code_addr;
id.code_addr_p = 1;
return id;
@@ -522,7 +546,7 @@ frame_id_build_wild (CORE_ADDR stack_addr)
struct frame_id id = null_frame_id;
id.stack_addr = stack_addr;
- id.stack_addr_p = 1;
+ id.stack_status = FID_STACK_VALID;
return id;
}
@@ -532,7 +556,7 @@ frame_id_p (struct frame_id l)
int p;
/* The frame is valid iff it has a valid stack address. */
- p = l.stack_addr_p;
+ p = l.stack_status != FID_STACK_INVALID;
/* outer_frame_id is also valid. */
if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
p = 1;
@@ -559,19 +583,20 @@ frame_id_eq (struct frame_id l, struct frame_id r)
{
int eq;
- if (!l.stack_addr_p && l.special_addr_p
- && !r.stack_addr_p && r.special_addr_p)
+ if (l.stack_status == FID_STACK_INVALID && l.special_addr_p
+ && r.stack_status == FID_STACK_INVALID && r.special_addr_p)
/* The outermost frame marker is equal to itself. This is the
dodgy thing about outer_frame_id, since between execution steps
we might step into another function - from which we can't
unwind either. More thought required to get rid of
outer_frame_id. */
eq = 1;
- else if (!l.stack_addr_p || !r.stack_addr_p)
+ else if (l.stack_status == FID_STACK_INVALID
+ || l.stack_status == FID_STACK_INVALID)
/* Like a NaN, if either ID is invalid, the result is false.
Note that a frame ID is invalid iff it is the null frame ID. */
eq = 0;
- else if (l.stack_addr != r.stack_addr)
+ else if (l.stack_status != r.stack_status || l.stack_addr != r.stack_addr)
/* If .stack addresses are different, the frames are different. */
eq = 0;
else if (l.code_addr_p && r.code_addr_p && l.code_addr != r.code_addr)
@@ -638,8 +663,9 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
{
int inner;
- if (!l.stack_addr_p || !r.stack_addr_p)
- /* Like NaN, any operation involving an invalid ID always fails. */
+ if (l.stack_status != FID_STACK_VALID || r.stack_status != FID_STACK_VALID)
+ /* Like NaN, any operation involving an invalid ID always fails.
+ Likewise if either ID has an unavailable stack address. */
inner = 0;
else if (l.artificial_depth > r.artificial_depth
&& l.stack_addr == r.stack_addr
diff --git a/gdb/frame.h b/gdb/frame.h
index f8d5bc1..b03f212 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -76,6 +76,23 @@ struct block;
struct gdbarch;
struct ui_file;
+/* Status of a given frame's stack. */
+
+enum frame_id_stack_status
+{
+ /* Stack address is invalid. E.g., this frame is the outermost
+ (i.e., _start), and the stack hasn't been setup yet. */
+ FID_STACK_INVALID = 0,
+
+ /* Stack address is valid, and is found in the stack_addr field. */
+ FID_STACK_VALID = 1,
+
+ /* Stack address is unavailable. I.e., there's a valid stack, but
+ we don't know where it is (because memory or registers we'd
+ compute it from were not collected). */
+ FID_STACK_UNAVAILABLE = -1
+};
+
/* The frame object. */
struct frame_info;
@@ -97,8 +114,9 @@ struct frame_id
function pointer register or stack pointer register. They are
wrong.
- This field is valid only if stack_addr_p is true. Otherwise, this
- frame represents the null frame. */
+ This field is valid only if frame_id.stack_status is
+ FID_STACK_VALID. It will be 0 for other
+ FID_STACK_... statuses. */
CORE_ADDR stack_addr;
/* The frame's code address. This shall be constant through out the
@@ -129,7 +147,7 @@ struct frame_id
CORE_ADDR special_addr;
/* Flags to indicate the above fields have valid contents. */
- unsigned int stack_addr_p : 1;
+ ENUM_BITFIELD(frame_id_stack_status) stack_status : 2;
unsigned int code_addr_p : 1;
unsigned int special_addr_p : 1;
@@ -169,6 +187,12 @@ extern struct frame_id frame_id_build_special (CORE_ADDR stack_addr,
CORE_ADDR code_addr,
CORE_ADDR special_addr);
+/* Construct a frame ID representing a frame where the stack address
+ exists, but is unavailable. CODE_ADDR is the frame's constant code
+ address (typically the entry point). The special identifier
+ address is set to indicate a wild card. */
+extern struct frame_id frame_id_build_unavailable_stack (CORE_ADDR code_addr);
+
/* Construct a wild card frame ID. The parameter is the frame's constant
stack address (typically the outer-bound). The code address as well
as the special identifier address are set to indicate wild cards. */
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index a1a4453..5b4a5a3 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1905,12 +1905,17 @@ i386_frame_this_id (struct frame_info *this_frame, void **this_cache,
{
struct i386_frame_cache *cache = i386_frame_cache (this_frame, this_cache);
- /* This marks the outermost frame. */
- if (cache->base == 0)
- return;
-
- /* See the end of i386_push_dummy_call. */
- (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ if (!cache->base_p)
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else if (cache->base == 0)
+ {
+ /* This marks the outermost frame. */
+ }
+ else
+ {
+ /* See the end of i386_push_dummy_call. */
+ (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ }
}
static enum unwind_stop_reason
@@ -2091,9 +2096,9 @@ i386_epilogue_frame_this_id (struct frame_info *this_frame,
i386_epilogue_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- (*this_id) = frame_id_build (cache->base + 8, cache->pc);
+ (*this_id) = frame_id_build_unavailable_stack (cache->pc);
+ else
+ (*this_id) = frame_id_build (cache->base + 8, cache->pc);
}
static struct value *
@@ -2284,10 +2289,12 @@ i386_sigtramp_frame_this_id (struct frame_info *this_frame, void **this_cache,
i386_sigtramp_frame_cache (this_frame, this_cache);
if (!cache->base_p)
- return;
-
- /* See the end of i386_push_dummy_call. */
- (*this_id) = frame_id_build (cache->base + 8, get_frame_pc (this_frame));
+ (*this_id) = frame_id_build_unavailable_stack (get_frame_pc (this_frame));
+ else
+ {
+ /* See the end of i386_push_dummy_call. */
+ (*this_id) = frame_id_build (cache->base + 8, get_frame_pc (this_frame));
+ }
}
static struct value *
diff --git a/gdb/testsuite/gdb.trace/circ.exp b/gdb/testsuite/gdb.trace/circ.exp
index f605bb6..6246e35 100644
--- a/gdb/testsuite/gdb.trace/circ.exp
+++ b/gdb/testsuite/gdb.trace/circ.exp
@@ -221,7 +221,7 @@ with_test_prefix "normal buffer" {
"first frame is at func0"
gdb_test "tfind pc func9" \
- ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ ".*Found trace frame $decimal, tracepoint $decimal\r\n#0 func9 .*" \
"find frame for func9"
}
@@ -282,6 +282,6 @@ with_test_prefix "circular buffer" {
gdb_test \
"tfind pc func9" \
- ".*Found trace frame $decimal, tracepoint $decimal.*" \
+ ".*Found trace frame $decimal, tracepoint $decimal\r\n#0 func9 .*" \
"find frame for func9"
}
--
1.7.11.7
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-16 16:19 ` Pedro Alves
@ 2013-12-17 9:04 ` Yao Qi
2013-12-17 10:09 ` Pedro Alves
0 siblings, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-12-17 9:04 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 12/17/2013 12:19 AM, Pedro Alves wrote:
> In case of trace frame debugging, I don't think you can ever
> have a tailcall frame on top of a frame with unavailable
> stack, because dwarf2_tailcall_sniffer_first will throw
> an error computing prev_sp. But even if that would work
Right, I find dwarf2_tailcall unwinder isn't selected for some tailcall
cases with an unavailable stack.
>> >
>> > What does the last sentence mean in the comments?
>> >
> The same comment is in frame_id_build. Re. what is means, see struct frame_id:
>
> /* The frame's special address. This shall be constant through out the
> lifetime of the frame. This is used for architectures that may have
> frames that do not change the stack but are still distinct and have
> some form of distinct identifier (e.g. the ia64 which uses a 2nd
> stack for registers). This field is treated as unordered - i.e. will
> not be used in frame ordering comparisons.
>
> This field is valid only if special_addr_p is true. Otherwise, this
> frame is considered to have a wildcard special address, i.e. one that
> ^^^^^^^^^^^^^
> matches every address value in frame comparisons. */
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> CORE_ADDR special_addr;
The last sentence, especially "wildcard" stuff, is still confusing to
me. Go through the usage of "special_addr", looks "special_addr" is
used only if "special_addr_p" is true. Comment "This field is valid
only if special_addr_p is true" is clear and sufficient. I don't see
any extra information the last sentence "Otherwise, xxxxx" delivered
except confusion.
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-17 9:04 ` Yao Qi
@ 2013-12-17 10:09 ` Pedro Alves
2013-12-17 12:39 ` Yao Qi
0 siblings, 1 reply; 23+ messages in thread
From: Pedro Alves @ 2013-12-17 10:09 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 12/17/2013 09:02 AM, Yao Qi wrote:
> On 12/17/2013 12:19 AM, Pedro Alves wrote:
>>>> What does the last sentence mean in the comments?
>>>>
>> The same comment is in frame_id_build. Re. what is means, see struct frame_id:
>>
>> /* The frame's special address. This shall be constant through out the
>> lifetime of the frame. This is used for architectures that may have
>> frames that do not change the stack but are still distinct and have
>> some form of distinct identifier (e.g. the ia64 which uses a 2nd
>> stack for registers). This field is treated as unordered - i.e. will
>> not be used in frame ordering comparisons.
>>
>> This field is valid only if special_addr_p is true. Otherwise, this
>> frame is considered to have a wildcard special address, i.e. one that
>> ^^^^^^^^^^^^^
>> matches every address value in frame comparisons. */
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> CORE_ADDR special_addr;
>
> The last sentence, especially "wildcard" stuff, is still confusing to
> me. Go through the usage of "special_addr", looks "special_addr" is
> used only if "special_addr_p" is true. Comment "This field is valid
> only if special_addr_p is true" is clear and sufficient.
It is clear, but it is not as precise or sufficient. A wild
card means that given these frame ids:
fid1: {code_p,stack_p,special_p}
fid2: {!code_p,stack_p,!special_p}
fid3: {code_p,stack_p,special_p}
fid4: {code_p,stack_p,!special_p}
{fid1, fid2} with same stack addresses,
and {fid3, fid4} with same code and stack addresses,
both:
frame_id_eq(fid1, fid2)
frame_id_eq(fid3, fid4)
return true.
> I don't see
> any extra information the last sentence "Otherwise, xxxxx" delivered
> except confusion.
The extra information indicates that e.g., frame_id_eq(fid3, fid4)
above returns true, not false, as one might at first expect.
Whether this whole wildcarding business is a good idea, is
another story.
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-17 10:09 ` Pedro Alves
@ 2013-12-17 12:39 ` Yao Qi
2013-12-17 20:55 ` Pedro Alves
0 siblings, 1 reply; 23+ messages in thread
From: Yao Qi @ 2013-12-17 12:39 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches
On 12/17/2013 06:09 PM, Pedro Alves wrote:
> It is clear, but it is not as precise or sufficient. A wild
> card means that given these frame ids:
>
> fid1: {code_p,stack_p,special_p}
> fid2: {!code_p,stack_p,!special_p}
>
> fid3: {code_p,stack_p,special_p}
> fid4: {code_p,stack_p,!special_p}
>
> {fid1, fid2} with same stack addresses,
> and {fid3, fid4} with same code and stack addresses,
> both:
>
> frame_id_eq(fid1, fid2)
> frame_id_eq(fid3, fid4)
>
> return true.
>
>> > I don't see
>> > any extra information the last sentence "Otherwise, xxxxx" delivered
>> > except confusion.
> The extra information indicates that e.g., frame_id_eq(fid3, fid4)
> above returns true, not false, as one might at first expect.
Thanks for giving these examples.
>
> Whether this whole wildcarding business is a good idea, is
> another story.
Anyway, I've got use to it now. :)
--
Yao (é½å°§)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] "tfind" across unavailable-stack frames.
2013-12-17 12:39 ` Yao Qi
@ 2013-12-17 20:55 ` Pedro Alves
0 siblings, 0 replies; 23+ messages in thread
From: Pedro Alves @ 2013-12-17 20:55 UTC (permalink / raw)
To: Yao Qi; +Cc: gdb-patches
On 12/17/2013 12:37 PM, Yao Qi wrote:
> Thanks for giving these examples.
No problem.
> On 12/17/2013 06:09 PM, Pedro Alves wrote:
>> Whether this whole wildcarding business is a good idea, is
>> another story.
>
> Anyway, I've got use to it now. :)
:-)
OK, I've pushed this in.
--
Pedro Alves
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2013-12-17 20:55 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-20 18:26 [patch] circ.exp Abid, Hafiz
2013-04-11 22:59 ` Abid, Hafiz
2013-04-16 7:52 ` Pedro Alves
2013-04-16 20:53 ` Abid, Hafiz
2013-04-17 20:33 ` Pedro Alves
2013-04-17 20:54 ` Abid, Hafiz
2013-04-18 10:30 ` Pedro Alves
2013-04-18 11:09 ` Pedro Alves
2013-12-13 17:49 ` [PATCH] "tfind" across unavailable-stack frames Pedro Alves
2013-12-14 6:23 ` Yao Qi
2013-12-16 16:19 ` Pedro Alves
2013-12-17 9:04 ` Yao Qi
2013-12-17 10:09 ` Pedro Alves
2013-12-17 12:39 ` Yao Qi
2013-12-17 20:55 ` Pedro Alves
2013-12-16 8:40 ` Metzger, Markus T
2013-12-16 16:25 ` Pedro Alves
2013-12-16 16:42 ` [PATCH v2] " Pedro Alves
2013-04-19 14:27 ` [patch] circ.exp Abid, Hafiz
2013-04-19 14:28 ` Pedro Alves
2013-05-08 10:12 ` Abid, Hafiz
2013-05-08 15:13 ` Pedro Alves
2013-05-08 16:18 ` Abid, Hafiz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).