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: Hui Zhu <teawater at gmail dot com>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Hui Zhu <hui_zhu at mentor dot com>, gdb-patches ml <gdb-patches at sourceware dot org>, Stan Shebs <stan at codesourcery dot com>, Joel Brobecker <brobecker at adacore dot com>
- Date: Sun, 7 Apr 2013 11:57:54 +0800
- Subject: Re: [PATCH] Reset tracepoint step_count to 0 before set it action
- References: <514EE46A dot 4070808 at mentor dot com> <515DC6C0 dot 7020406 at redhat dot com> <515DD41D dot 9020608 at redhat dot com>
Hi Pedro,
Thanks for your help.
Best.
Hui
On Fri, Apr 5, 2013 at 3:27 AM, Pedro Alves <palves@redhat.com> wrote:
> On 04/04/2013 07:30 PM, Pedro Alves wrote:
>
>> Here's what I'm checking in.
>
> On 03/24/2013 11:32 AM, Hui Zhu wrote:
>> And I suggest this change can be checked to 7.6 branch.
>
> I think it's safe enough, so I went ahead and put it in the branch as well.
>
> Here it is again, now with full commit log. The 7.6 version
> had a minor adjustment as validate_actionline's prototype
> is slightly different there.
>
> --------------
> tracepoint->step_count fixes
>
> If a tracepoint's actions list includes a while-stepping action, and
> then the actions are changed to a list without any while-stepping
> action, the tracepoint's step_count will be left with a stale value.
> For example:
>
> (gdb) trace subr
> Tracepoint 1 at 0x4004d9: file ../../../src/gdb/testsuite//actions-changed.c, line 31.
> (gdb) actions
> Enter actions for tracepoint 1, one per line.
> End with a line saying just "end".
> >collect $reg
> >end
> (gdb) set debug remote 1
> (gdb) tstart
> Sending packet: $QTinit#59...Packet received: OK
> Sending packet: $QTDP:1:00000000004004d9:E:0:0-#a3...Packet received: OK
> Sending packet: $QTDP:-1:00000000004004d9:R03FFFFFFFFFFFFFFFFFF#2b...Packet received: OK
> (gdb) tstop
> Sending packet: $QTStop#4b...Packet received: OK
> Sending packet: $QTNotes:#e8...Packet received: OK
> (gdb) actions
> Enter actions for tracepoint 1, one per line.
> End with a line saying just "end".
> >collect $reg
> >while-stepping 1
> >collect $reg
> >end
> >end
> (gdb) tstart
> Sending packet: $QTinit#59...Packet received: OK
> Sending packet: $QTDP:1:00000000004004d9:E:1:0-#a4...Packet received: OK
> Sending packet: $QTDP:-1:00000000004004d9:R03FFFFFFFFFFFFFFFFFF-#58...Packet received: OK
> Sending packet: $QTDP:-1:00000000004004d9:SR03FFFFFFFFFFFFFFFFFF#7e...Packet received: OK
> (gdb) tstop
> Sending packet: $QTStop#4b...Packet received: OK
> Sending packet: $QTNotes:#e8...Packet received: OK
> (gdb) actions
> Enter actions for tracepoint 1, one per line.
> End with a line saying just "end".
> >collect $regs
> >end
> (gdb) tstart
> Sending packet: $QTinit#59...Packet received: OK
> Sending packet: $QTDP:1:00000000004004d9:E:1:0-#a4...Packet received: OK
> Sending packet: $QTDP:-1:00000000004004d9:R03FFFFFFFFFFFFFFFFFF#2b...Packet received: OK
>
> The last "$QTDP:1:00000000004004d9:E:1:0-#a4" should be "$QTDP:1:00000000004004d9:E:0:0-#a3".
> In pseudo-diff:
>
> -$QTDP:1:00000000004004d9:E:1:0-#a4
> +$QTDP:1:00000000004004d9:E:0:0-#a3
>
> A related issue is that the "commands" command actually supports
> setting commands to a range of breakpoints/tracepoints at once. But,
> hacking "maint info breakpoints" to print t->step_count, reveals:
>
> (gdb) trace main
> Tracepoint 5 at 0x45a2ab: file ../../src/gdb/gdb.c, line 29.
> (gdb) trace main
> Note: breakpoint 5 also set at pc 0x45a2ab.
> Tracepoint 6 at 0x45a2ab: file ../../src/gdb/gdb.c, line 29.
> (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
> (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
> (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
> (gdb)
>
> that tracepoint 6 doesn't end up with the correct step_count.
>
> 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 the first 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 that should be harmless.
>
> 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/breakpoint.c | 23 +++-
> gdb/testsuite/gdb.trace/Makefile.in | 6 -
> gdb/testsuite/gdb.trace/actions-changed.c | 66 ++++++++++
> gdb/testsuite/gdb.trace/actions-changed.exp | 174 +++++++++++++++++++++++++++
> gdb/testsuite/lib/trace-support.exp | 24 +++-
> 5 files changed, 281 insertions(+), 12 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..bac24a7
> --- /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 (5);
> + 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.
>