This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Reset tracepoint step_count to 0 before set it action
- From: Pedro Alves <palves at redhat dot com>
- To: Hui Zhu <hui_zhu at mentor dot com>
- Cc: gdb-patches ml <gdb-patches at sourceware dot org>, Stan Shebs <stan at codesourcery dot com>, Joel Brobecker <brobecker at adacore dot com>
- Date: Thu, 04 Apr 2013 19:30:24 +0100
- Subject: Re: [PATCH] Reset tracepoint step_count to 0 before set it action
- References: <514EE46A dot 4070808 at mentor dot com>
Hi,
Thanks for the patch.
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1296,6 +1296,16 @@ do_map_commands_command (struct breakpoi
> {
> struct commands_info *info = data;
>
> + if (b->type == bp_tracepoint)
> + {
> + struct tracepoint *t = (struct tracepoint *) b;
> +
> + /* Reset the step count to 0 because if this tracepoint has step
> + action in before, it will not reset it to 0 if new actions
> + doesn't have while-stepping. */
> + t->step_count = 0;
> + }
> +
> if (info->cmd == NULL)
> {
> struct command_line *l;
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -666,6 +666,11 @@ trace_actions_command (char *args, int f
> t->base.number);
> struct cleanup *cleanups = make_cleanup (xfree, tmpbuf);
>
> + /* Reset the step count to 0 because if this tracepoint has step
> + action in before, it will not reset it to 0 if new actions
> + doesn't have while-stepping. */
> + t->step_count = 0;
> +
> l = read_command_lines (tmpbuf, from_tty, 1,
> check_tracepoint_command, t);
> do_cleanups (cleanups);
>
>
I was about to okay this, but then I recalled that the "commands"
command actually supports setting commands to a range of
breakpoints/tracepoints at once. Looking at the code, it didn't look
like it was doing the right thing. So I hacked "maint info breakpoints"
to print t->step_count, and noticed (with or without your
patch applied):
(top-gdb) trace main
Tracepoint 5 at 0x45a2ab: file ../../src/gdb/gdb.c, line 29.
(top-gdb) trace main
Note: breakpoint 5 also set at pc 0x45a2ab.
Tracepoint 6 at 0x45a2ab: file ../../src/gdb/gdb.c, line 29.
(top-gdb) commands 5-6
Type commands for breakpoint(s) 5-6, one per line.
End with a line saying just "end".
>while-stepping 5
>end
>end
(top-gdb) maint info breakpoints 5
Num Type Disp Enb Address What
5 tracepoint keep y 0x000000000045a2ab in main at ../../src/gdb/gdb.c:29 inf 1
step_count=5
^^^^^^^^^^^^
while-stepping 5
end
not installed on target
(top-gdb) maint info breakpoints 6
Num Type Disp Enb Address What
6 tracepoint keep y 0x000000000045a2ab in main at ../../src/gdb/gdb.c:29 inf 1
step_count=0
^^^^^^^^^^^^
while-stepping 5
end
not installed on target
(top-gdb)
Tracepoint 6 didn't end up with the correct step_count.
I couldn't give a suggestion right away, and I had a hunch fixing that
would end up fixing your related issue differently, and before you know it,
I ended up hacking on it myself. The resulting patch is at the bottom.
The issue is that here:
static void
do_map_commands_command (struct breakpoint *b, void *data)
{
struct commands_info *info = data;
if (info->cmd == NULL)
{
struct command_line *l;
if (info->control != NULL)
l = copy_command_lines (info->control->body_list[0]);
else
{
struct cleanup *old_chain;
char *str;
str = xstrprintf (_("Type commands for breakpoint(s) "
"%s, one per line."),
info->arg);
old_chain = make_cleanup (xfree, str);
l = read_command_lines (str,
info->from_tty, 1,
(is_tracepoint (b)
? check_tracepoint_command : 0),
b);
do_cleanups (old_chain);
}
info->cmd = alloc_counted_command_line (l);
}
validate_actionline is never called for tracepoints other than the
first (the copy_command_lines path). Right below, we have:
/* If a breakpoint was on the list more than once, we don't need to
do anything. */
if (b->commands != info->cmd)
{
validate_commands_for_breakpoint (b, info->cmd->commands);
incref_counted_command_line (info->cmd);
decref_counted_command_line (&b->commands);
b->commands = info->cmd;
observer_notify_breakpoint_modified (b);
}
And validate_commands_for_breakpoint looks like the right place
to put a call; if we reset step_count there too, we have a nice
central fix for your issue as well, because trace_actions_command
calls breakpoint_set_commands that also calls
validate_commands_for_breakpoint.
We end up calling validate_actionline twice for the first tracepoint,
since read_command_lines calls it too, through check_tracepoint_command,
but IMO that is harmless.
> --- /dev/null
> +++ b/gdb/testsuite/gdb.trace/actions-changed.exp
> @@ -0,0 +1,121 @@
> +load_lib trace-support.exp
> +
> +standard_testfile actions-changed.c
No need to pass "actions-changed.c", it's the default.
> +if { [gdb_compile "$srcdir/$subdir/$srcfile" $binfile \
> + executable {debug nowarnings}] != "" } {
> + untested actions-changed.exp
> + return -1
> +}
Should be using prepare_for_testing. "nowarning" is not necessary.
> +
> +proc test_actions_changed { } \
> +{
It's more standard to leave the { in the previous line.
> + gdb_breakpoint "begin"
> + gdb_breakpoint "middle"
> + gdb_breakpoint "later"
> + gdb_breakpoint "endish"
> + gdb_breakpoint "end"
> +
> + gdb_test "continue" ".*Breakpoint \[0-9\]+, begin .*" \
> + "advance to tracing"
> +
> + gdb_test "trace subr" "Tracepoint .*" \
> + "tracepoint at subr"
> +
> + # First pass, define simple action
> +
> + gdb_trace_setactions "define simple action" \
> + "" \
> + "collect parm" "^$"
> +
> + gdb_test_no_output "tstart"
> +
> + gdb_test "continue" ".*Breakpoint \[0-9\]+, middle .*" \
> + "advance through tracing, 1st"
> +
> + gdb_test "tstatus" ".*Collected 1 trace frame.*" \
> + "check on first trace status"
I changed the test messages a bit.
> +
> + gdb_test_no_output "tstop"
> +
> + # Redefine action, run second trace
> +
> + gdb_trace_setactions "redefine simple action" \
> + "" \
> + "collect keeping, busy" "^$"
> +
> + gdb_test_no_output "tstart"
> +
> + gdb_test "continue" ".*Breakpoint \[0-9\]+, later .*" \
> + "advance through tracing, 2nd"
> +
> + gdb_test "tstatus" ".*Collected 1 trace frame.*" \
> + "check on redefined trace status"
> +
> + gdb_test_no_output "tstop"
> +
> + # Redefine to stepping action, run third trace
These "tstart" "tstop" make it so there are non unique messages
in the test results:
$ cat testsuite/gdb.sum | grep PASS| sort| uniq -c | sort -n | tail -n 4
1 PASS: gdb.trace/actions-changed.exp: redefine to stepping action
1 PASS: gdb.trace/actions-changed.exp: tracepoint at subr
3 PASS: gdb.trace/actions-changed.exp: tstop
4 PASS: gdb.trace/actions-changed.exp: tstart
Instead of those "1st", "2nd" hardcoded bits, we can use
with_test_prefix.
> + gdb_breakpoint "begin"
> + gdb_breakpoint "middle"
> + gdb_breakpoint "later"
> + gdb_breakpoint "endish"
> + gdb_breakpoint "end"
I was adding more tests to cover the "commands 5-6" issue,
and I found this to not be very flexible. :-) I instead made
the "end" take an argument that matches the current pass, and
got rid of begin/middle/etc.
This refactors gdb_trace_setactions a little in order to
add a variant uses the "commands" command instead of
the "actions" command.
Here's what I'm checking in.
2013-04-04 Pedro Alves <palves@redhat.com>
Hui Zhu <hui@codesourcery.com>
* breakpoint.c (validate_commands_for_breakpoint): If validating a
tracepoint, reset its STEP_COUNT and call validate_actionline.
2013-04-04 Stan Shebs <stan@codesourcery.com>
Pedro Alves <palves@redhat.com>
* gdb.trace/Makefile.in (PROGS): Add actions-changed.
* gdb.trace/actions-changed.c: New file.
* gdb.trace/actions-changed.exp: New file.
* lib/trace-support.exp (gdb_trace_setactions): Rename to ...
(gdb_trace_setactions_command): ... this. Add "actions_command"
parameter, and handle it.
(gdb_trace_setactions, gdb_trace_setcommands): New procedures.
---
gdb/testsuite/gdb.trace/Makefile.in | 6 -
gdb/testsuite/gdb.trace/actions-changed.c | 66 ++++++++++
gdb/testsuite/gdb.trace/actions-changed.exp | 174 +++++++++++++++++++++++++++
3 files changed, 243 insertions(+), 3 deletions(-)
create mode 100644 gdb/testsuite/gdb.trace/actions-changed.c
create mode 100644 gdb/testsuite/gdb.trace/actions-changed.exp
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ff161a0..5ba1f2f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1142,12 +1142,25 @@ validate_commands_for_breakpoint (struct breakpoint *b,
{
if (is_tracepoint (b))
{
- /* We need to verify that each top-level element of commands is
- valid for tracepoints, that there's at most one
- while-stepping element, and that while-stepping's body has
- valid tracing commands excluding nested while-stepping. */
+ struct tracepoint *t = (struct tracepoint *) b;
struct command_line *c;
struct command_line *while_stepping = 0;
+
+ /* Reset the while-stepping step count. The previous commands
+ might have included a while-stepping action, while the new
+ ones might not. */
+ t->step_count = 0;
+
+ /* We need to verify that each top-level element of commands is
+ valid for tracepoints, that there's at most one
+ while-stepping element, and that the while-stepping's body
+ has valid tracing commands excluding nested while-stepping.
+ We also need to validate the tracepoint action line in the
+ context of the tracepoint --- validate_actionline actually
+ has side effects, like setting the tracepoint's
+ while-stepping STEP_COUNT, in addition to checking if the
+ collect/teval actions parse and make sense in the
+ tracepoint's context. */
for (c = commands; c; c = c->next)
{
if (c->control_type == while_stepping_control)
@@ -1165,6 +1178,8 @@ validate_commands_for_breakpoint (struct breakpoint *b,
else
while_stepping = c;
}
+
+ validate_actionline (c->line, b);
}
if (while_stepping)
{
diff --git a/gdb/testsuite/gdb.trace/Makefile.in b/gdb/testsuite/gdb.trace/Makefile.in
index 8a7d523..2e23223 100644
--- a/gdb/testsuite/gdb.trace/Makefile.in
+++ b/gdb/testsuite/gdb.trace/Makefile.in
@@ -3,9 +3,9 @@ srcdir = @srcdir@
.PHONY: all clean mostlyclean distclean realclean
-PROGS = ax backtrace deltrace disconnected-tracing infotrace packetlen \
- passc-dyn passcount report save-trace tfile tfind tracecmd tsv \
- unavailable while-dyn while-stepping
+PROGS = actions-changed ax backtrace deltrace disconnected-tracing \
+ infotrace packetlen passc-dyn passcount report save-trace tfile \
+ tfind tracecmd tsv unavailable while-dyn while-stepping
all info install-info dvi install uninstall installcheck check:
@echo "Nothing to be done for $@..."
diff --git a/gdb/testsuite/gdb.trace/actions-changed.c b/gdb/testsuite/gdb.trace/actions-changed.c
new file mode 100644
index 0000000..5443e93
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/actions-changed.c
@@ -0,0 +1,66 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2013 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+int
+end (int i)
+{
+}
+
+int
+subr2 (int parm)
+{
+ int keeping, busy;
+
+ keeping = parm + parm;
+ busy = keeping * keeping;
+
+ return busy;
+}
+
+int
+subr (int parm)
+{
+ int keeping, busy;
+
+ keeping = parm + parm;
+ busy = keeping * keeping;
+
+ return busy;
+}
+
+main()
+{
+ subr (1);
+ end (1);
+
+ subr (2);
+ end (2);
+
+ subr (3);
+ end (3);
+
+ subr (4);
+ end (4);
+
+ subr (5);
+ subr2 (6);
+ end (5);
+
+ subr (6);
+ subr2 (6);
+ end (6);
+}
diff --git a/gdb/testsuite/gdb.trace/actions-changed.exp b/gdb/testsuite/gdb.trace/actions-changed.exp
new file mode 100644
index 0000000..e850da2
--- /dev/null
+++ b/gdb/testsuite/gdb.trace/actions-changed.exp
@@ -0,0 +1,174 @@
+# Copyright 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+load_lib trace-support.exp
+
+standard_testfile
+
+if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
+ return -1
+}
+
+proc test_actions_changed { } {
+ gdb_breakpoint "end"
+
+ gdb_test "trace subr" "Tracepoint .*" \
+ "tracepoint at subr"
+
+ # The first set of tests are regression tests for a GDB bug where
+ # the while-stepping count of a tracepoint would be left stale if
+ # the tracepoint's actions were redefined, and the new action list
+ # had no while-stepping action.
+
+ # First pass, define simple action.
+ with_test_prefix "1" {
+ gdb_trace_setactions "define simple action" \
+ "" \
+ "collect parm" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint \[0-9\]+, end \\(i=1\\) .*" \
+ "advance through tracing"
+
+ gdb_test "tstatus" ".*Collected 1 trace frame.*" \
+ "collected 1 trace frame"
+
+ gdb_test_no_output "tstop"
+ }
+
+ # Redefine action, run second trace.
+ with_test_prefix "2" {
+ gdb_trace_setactions "redefine simple action" \
+ "" \
+ "collect keeping, busy" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint \[0-9\]+, end \\(i=2\\) .*" \
+ "advance through tracing"
+
+ gdb_test "tstatus" ".*Collected 1 trace frame.*" \
+ "collected 1 trace frame"
+
+ gdb_test_no_output "tstop"
+ }
+
+ # Redefine to stepping action, run third trace.
+ with_test_prefix "3" {
+ gdb_trace_setactions "redefine to stepping action" \
+ "" \
+ "collect parm" "^$" \
+ "while-stepping 5" "^$" \
+ "collect parm" "^$" \
+ "end" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint \[0-9\]+, end \\(i=3\\) .*" \
+ "advance through tracing"
+
+ gdb_test "tstatus" ".*Collected 6 trace frames.*" \
+ "collected 6 trace frames"
+
+ gdb_test_no_output "tstop"
+ }
+
+ # Redefine to non-stepping, run fourth trace.
+ with_test_prefix "4" {
+ gdb_trace_setactions "redefine to non-stepping action" \
+ "" \
+ "collect parm" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint \[0-9\]+, end \\(i=4\\) .*" \
+ "advance through tracing"
+
+ gdb_test "tstatus" ".*Collected 1 trace frame.*" \
+ "collected 1 trace frame"
+
+ gdb_test_no_output "tstop"
+ }
+
+ # The following tests are related to the above, but use two
+ # tracepoints. They are regression tests for a GDB bug where only
+ # the first tracepoint would end up with the step count set.
+
+ # Store the first tracepoint's number.
+ gdb_test_no_output "set \$prev_tpnum=\$tpnum" "store previous \$tpnum"
+
+ # And here's the second tracepoint.
+ gdb_test "trace subr2" "Tracepoint .*" "tracepoint at subr2"
+
+ # Set a stepping action in both tracepoints, with the "commands"
+ # command.
+ with_test_prefix "5" {
+ gdb_trace_setcommands \
+ "redefine 2 tracepoints to stepping action, using commands" \
+ "\$prev_tpnum-\$tpnum" \
+ "collect parm" "^$" \
+ "while-stepping 5" "^$" \
+ "collect parm" "^$" \
+ "end" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint \[0-9\]+, end \\(i=5\\) .*" \
+ "advance through tracing"
+
+ gdb_test "tstatus" ".*Collected 12 trace frames.*" \
+ "collected 12 trace frames"
+
+ gdb_test_no_output "tstop"
+ }
+
+ # Redefine the actions of both tracepoints to non-stepping, also
+ # using the "commands" command.
+ with_test_prefix "6" {
+ gdb_trace_setcommands \
+ "redefine 2 tracepoints to non-stepping action, using commands" \
+ "\$prev_tpnum-\$tpnum" \
+ "collect parm" "^$"
+
+ gdb_test_no_output "tstart"
+
+ gdb_test "continue" ".*Breakpoint \[0-9\]+, end \\(i=6\\) .*" \
+ "advance through tracing"
+
+ gdb_test "tstatus" ".*Collected 2 trace frame.*" \
+ "collected 2 trace frames"
+
+ gdb_test_no_output "tstop"
+ }
+}
+
+# Check whether the target supports tracepoints.
+
+clean_restart $testfile
+
+if ![runto_main] {
+ fail "Can't run to main to check for trace support"
+ return -1
+}
+
+if ![gdb_target_supports_trace] {
+ unsupported "Current target does not support trace"
+ return -1;
+}
+
+test_actions_changed
+
+return 0
diff --git a/gdb/testsuite/lib/trace-support.exp b/gdb/testsuite/lib/trace-support.exp
index 10482b8..2601ad8 100644
--- a/gdb/testsuite/lib/trace-support.exp
+++ b/gdb/testsuite/lib/trace-support.exp
@@ -86,23 +86,23 @@ proc gdb_delete_tracepoints {} {
}
}
-#
-# Procedure: gdb_trace_setactions
# Define actions for a tracepoint.
# Arguments:
+# actions_command -- the command used to create the actions.
+# either "actions" or "commands".
# testname -- identifying string for pass/fail output
-# tracepoint -- to which tracepoint do these actions apply? (optional)
+# tracepoint -- to which tracepoint(s) do these actions apply? (optional)
# args -- list of actions to be defined.
# Returns:
# zero -- success
# non-zero -- failure
-proc gdb_trace_setactions { testname tracepoint args } {
+proc gdb_trace_setactions_command { actions_command testname tracepoint args } {
global gdb_prompt;
set state 0;
set passfail "pass";
- send_gdb "actions $tracepoint\n";
+ send_gdb "$actions_command $tracepoint\n";
set expected_result "";
gdb_expect 5 {
-re "No tracepoint number .*$gdb_prompt $" {
@@ -165,6 +165,20 @@ proc gdb_trace_setactions { testname tracepoint args } {
}
}
+# Define actions for a tracepoint, using the "actions" command. See
+# gdb_trace_setactions_command.
+#
+proc gdb_trace_setactions { testname tracepoint args } {
+ eval gdb_trace_setactions_command "actions" {$testname} {$tracepoint} $args
+}
+
+# Define actions for a tracepoint, using the "commands" command. See
+# gdb_trace_setactions_command.
+#
+proc gdb_trace_setcommands { testname tracepoint args } {
+ eval gdb_trace_setactions_command "commands" {$testname} {$tracepoint} $args
+}
+
#
# Procedure: gdb_tfind_test
# Find a specified trace frame.