From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12745 invoked by alias); 7 Apr 2013 03:58:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 12733 invoked by uid 89); 7 Apr 2013 03:58:42 -0000 X-Spam-SWARE-Status: No, score=-4.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE autolearn=ham version=3.3.1 Received: from mail-bk0-f54.google.com (HELO mail-bk0-f54.google.com) (209.85.214.54) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Sun, 07 Apr 2013 03:58:37 +0000 Received: by mail-bk0-f54.google.com with SMTP id q16so2648038bkw.27 for ; Sat, 06 Apr 2013 20:58:35 -0700 (PDT) X-Received: by 10.204.135.12 with SMTP id l12mr1656298bkt.101.1365307115202; Sat, 06 Apr 2013 20:58:35 -0700 (PDT) MIME-Version: 1.0 Received: by 10.204.201.3 with HTTP; Sat, 6 Apr 2013 20:57:54 -0700 (PDT) In-Reply-To: <515DD41D.9020608@redhat.com> References: <514EE46A.4070808@mentor.com> <515DC6C0.7020406@redhat.com> <515DD41D.9020608@redhat.com> From: Hui Zhu Date: Mon, 08 Apr 2013 07:58:00 -0000 Message-ID: Subject: Re: [PATCH] Reset tracepoint step_count to 0 before set it action To: Pedro Alves Cc: Hui Zhu , gdb-patches ml , Stan Shebs , Joel Brobecker Content-Type: text/plain; charset=ISO-8859-1 X-SW-Source: 2013-04/txt/msg00169.txt.bz2 Hi Pedro, Thanks for your help. Best. Hui On Fri, Apr 5, 2013 at 3:27 AM, Pedro Alves 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 > Hui Zhu > > * breakpoint.c (validate_commands_for_breakpoint): If validating a > tracepoint, reset its STEP_COUNT and call validate_actionline. > > 2013-04-04 Stan Shebs > Pedro Alves > > * 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 . */ > + > +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 . > + > +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. >