From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11152 invoked by alias); 17 May 2013 21:01:17 -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 11140 invoked by uid 89); 17 May 2013 21:01:17 -0000 X-Spam-SWARE-Status: No, score=-7.9 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS,TW_XP autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 17 May 2013 21:01:14 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r4HL19GC023278 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 17 May 2013 17:01:09 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r4HL161H012795; Fri, 17 May 2013 17:01:07 -0400 Message-ID: <51969A92.80003@redhat.com> Date: Fri, 17 May 2013 21:01:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Hui Zhu CC: gdb-patches@sourceware.org, Keith Seitz , Tom Tromey , Yao Qi Subject: Re: [RFC] PR 15075 dprintf interferes with "next" References: <1361192891-29341-1-git-send-email-yao@codesourcery.com> <8738wpd3qe.fsf@fleche.redhat.com> <5176C14B.6010603@redhat.com> <51774714.9060306@codesourcery.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00702.txt.bz2 On 05/16/2013 08:28 AM, Hui Zhu wrote: > Hi, > > Because this patch also can handle bug 15434, I updated the test > patches to add test dprintf with non-stop mode. Looks like the test is racy. :-/ I got: At foo entry arg=1235, g=2222 iAt foo entry ntarg=1236, g=3013 errupt (gdb) FAIL: gdb.base/dprintf.exp: interrupt This will need to be sorted out. I also saw: (gdb) At foo entry arg=1235, g=2222 At foo entry arg=1236, g=3013 [Inferior 1 (process 30651) exited with code 0152] FAIL: gdb.base/dprintf.exp: shell echo foo (timeout) interrupt ../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Quit this debugging session? (y or n) FAIL: gdb.base/dprintf.exp: interrupt (GDB internal error) Resyncing due to internal error. n ../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed. A problem internal to GDB has been detected, further debugging may prove unreliable. Create a core file of GDB? (y or n) n (gdb) FAIL: gdb.base/dprintf.exp: process stopped (timeout) testcase ../../../src/gdb/testsuite/gdb.base/dprintf.exp completed in 20 seconds :-( Though that's likely unrelated. > 2013-05-16 Yao Qi > Hui Zhu > > PR breakpoints/15075 > PR breakpoints/15434 > * breakpoint.c (bpstat_stop_status): Call b->ops->after_cond. > (update_dprintf_command_list): Don't append "continue" command > to the command of dprintf breakpoint. "to the command list." > (base_breakpoint_after_cond): New. > (base_breakpoint_ops): Add base_breakpoint_after_cond. > (dprintf_after_cond): New. Say "New function." > (initialize_breakpoint_ops): Set dprintf_after_cond. > * breakpoint.h (breakpoint_ops): Add after_cond. > > 2013-05-16 Yao Qi > Hui Zhu > > PR breakpoints/15075 > PR breakpoints/15434 > * gdb.base/dprintf.exp: Don't check "continue" in the output > of "info breakpoints". > Add test dprintf with non-stop mode. Add test for dprintf in non-stop mode. > * gdb.base/dprintf.c: Add a sleep for test dprintf with non-stop for testing. > mode. > * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): > Don't check "continue" in script field. > * gdb.base/pr15075.c: New. > * gdb.base/pr15075.exp: New. > > > > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -5283,13 +5283,7 @@ bpstat_stop_status (struct address_space > b->enable_state = bp_disabled; > removed_any = 1; > } > - if (b->silent) > - bs->print = 0; > - bs->commands = b->commands; > - incref_counted_command_line (bs->commands); > - if (command_line_is_silent (bs->commands > - ? bs->commands->commands : NULL)) > - bs->print = 0; > + b->ops->after_cond (bs); > } > > } > @@ -8939,25 +8933,16 @@ update_dprintf_command_list (struct brea > _("Invalid dprintf style.")); > > gdb_assert (printf_line != NULL); > - /* Manufacture a printf/continue sequence. */ > + /* Manufacture a printf sequence. */ > { > - struct command_line *printf_cmd_line, *cont_cmd_line = NULL; > - > - if (strcmp (dprintf_style, dprintf_style_agent) != 0) > - { > - cont_cmd_line = xmalloc (sizeof (struct command_line)); > - cont_cmd_line->control_type = simple_control; > - cont_cmd_line->body_count = 0; > - cont_cmd_line->body_list = NULL; > - cont_cmd_line->next = NULL; > - cont_cmd_line->line = xstrdup ("continue"); > - } > + struct command_line *printf_cmd_line > + = xmalloc (sizeof (struct command_line)); > > printf_cmd_line = xmalloc (sizeof (struct command_line)); > printf_cmd_line->control_type = simple_control; > printf_cmd_line->body_count = 0; > printf_cmd_line->body_list = NULL; > - printf_cmd_line->next = cont_cmd_line; > + printf_cmd_line->next = NULL; > printf_cmd_line->line = printf_line; > > breakpoint_set_commands (b, printf_cmd_line); > @@ -12743,6 +12728,22 @@ base_breakpoint_explains_signal (struct > return BPSTAT_SIGNAL_HIDE; > } > > +/* The default 'after_cond' method. */ You used '' here. > + > +static void > +base_breakpoint_after_cond (struct bpstats *bs) > +{ > + struct breakpoint *b = bs->breakpoint_at; > + > + if (b->silent) > + bs->print = 0; > + bs->commands = b->commands; > + incref_counted_command_line (bs->commands); > + if (command_line_is_silent (bs->commands > + ? bs->commands->commands : NULL)) > + bs->print = 0; Indentation is wrong here. I think we can keep all these where it was. > +} > + > +/* Implement the "after_cond" breakpoint_ops method for dprintf. */ You used "" here. :-) Spurious double space before breakpoint_ops. > + > +static void > +dprintf_after_cond (struct bpstats *bs) > +{ > + bpstat tmp; > + struct breakpoint *b = bs->breakpoint_at; > + > + bs->stop = 0; > + bs->print = 0; If we make dprintf's silent (b->silent = 0), then bs->print will end up 0 for them too, given the: if (b->silent) bs->print = 0; bit. Not sure even that is necessary, given bs->stop==0 and: /* Print nothing for this entry if we don't stop or don't print. */ if (!bs->stop || !bs->print) bs->print_it = print_it_noop; > + bs->commands = b->commands; > + tmp = bs->next; > + bs->next = tmp; A = B; B = A; ? Was this supposed to be: A = B; B = NULL; ? > + incref_counted_command_line (bs->commands); If copying all this code here really is necessary, then keep the incref close to the commands copy, like in the original code. > + bpstat_do_actions_1 (&bs); > + bs->next = tmp; > +} Could you add some comments explaining what this is doing, and why? > + > /* The breakpoint_ops structure to be used on static tracepoints with > markers (`-m'). */ > > @@ -15852,6 +15872,7 @@ initialize_breakpoint_ops (void) > ops->print_it = bkpt_print_it; > ops->print_mention = bkpt_print_mention; > ops->print_recreate = dprintf_print_recreate; > + ops->after_cond = dprintf_after_cond; > } > > /* Chain containing all defined "enable breakpoint" subcommands. */ > --- a/gdb/breakpoint.h > +++ b/gdb/breakpoint.h > @@ -614,6 +614,9 @@ struct breakpoint_ops > 'catch signal' interact properly with 'handle'; see > bpstat_explains_signal. */ > enum bpstat_signal_value (*explains_signal) (struct breakpoint *); > + > + /* Do some setup after check condition is true. */ /* Called after evaluating the breakpoint's condition, and only if it evaluated true. */ > + void (*after_cond) (struct bpstats *bs); I'd rather spell out "cond". I suggest: void (*after_condition_true) (...); To be a bit more self descriptive. > }; > > /* Helper for breakpoint_ops->print_recreate implementations. Prints > > > dprintf-continue-test.txt > > > --- a/gdb/testsuite/gdb.base/dprintf.c > +++ b/gdb/testsuite/gdb.base/dprintf.c > @@ -39,6 +39,9 @@ main (int argc, char *argv[]) > foo (loc++); > foo (loc++); > foo (loc++); > + > + sleep (3); > + > return g; > } > > --- a/gdb/testsuite/gdb.base/dprintf.exp > +++ b/gdb/testsuite/gdb.base/dprintf.exp > @@ -50,10 +50,8 @@ gdb_test_sequence "info breakpoints" "dp > "\[\r\n\]2 breakpoint" > "\[\r\n\]3 dprintf" > "\[\r\n\] printf \"At foo entry\\\\n\"" > - "\[\r\n\] continue" > "\[\r\n\]4 dprintf" > "\[\r\n\] printf \"arg=%d, g=%d\\\\n\", arg, g" > - "\[\r\n\] continue" > } > > gdb_test "break $bp_location1" \ > @@ -136,3 +134,41 @@ if $target_can_dprintf { > gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \ > "Set dprintf style to an unrecognized type" > > + > +# Test dprintf with non-stop mode. > +# The testfile uses "run". The real bug happened only for ![is_remote target]. It doesn't need to use "run", AFAICS. You can run to main as usual, and then use "c&" to get the same effect, and then the test should pass with GDBserver too. There are targets that don't support non-stop, and, the call/gdb dprintf styles works on all targets. The test should be gracefully skipped on those targets. It might be better to split this to a separate test file. > +if [target_info exists use_gdb_stub] { > + unsupported "dprintf with non-stop mode" > + return 0 > +} > +gdb_test_no_output "delete 2 5" > +send_gdb "kill\n" > +gdb_expect 120 { > + -re "Kill the program being debugged. .y or n. $" { > + send_gdb "y\n" > + verbose "\t\tKilling previous program being debugged" > + exp_continue > + } > + -re "$gdb_prompt $" { > + # OK. > + } > +} Why do this manually? > +gdb_test_no_output "set dprintf-style gdb" "Set dprintf style to gdb" > +gdb_test_no_output "set target-async on" > +gdb_test_no_output "set non-stop on" > +set gdbindex_warning_re "warning: Skipping \[^\r\n\]+ \\.gdb_index section \[^\r\n\]*\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\." I don't under why do we need to explicitly check for this warning? > +gdb_test "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?" > +gdb_test "shell echo foo" "foo" What's this shell echo for? > +set test "interrupt" > +gdb_test_multiple $test $test { > + -re "interrupt\r\n$gdb_prompt " { > + pass $test > + } > +} > +set test "process stopped" > +gdb_test_multiple "" $test { > + -re "\r\n\\\[process \[0-9\]+\\\] #1 stopped\\\.\r\n" { > + pass $test > + } > +} > --- /dev/null > +++ b/gdb/testsuite/gdb.base/pr15075.c We prefer that test files are named for something that suggests what they contain, rather than bug numbers. Please call it something like dprintf-next.exp. > @@ -0,0 +1,26 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 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 > +main(void) Missing space. > + { > + int x = 5; Wrong indentation. > + > + ++x; > + ++x; /* set dprintf here */ > + return x - 7; > + } > --- /dev/null > +++ b/gdb/testsuite/gdb.base/pr15075.exp > @@ -0,0 +1,38 @@ > +# 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 . > + > +standard_testfile > + > +set executable $testfile > +set expfile $testfile.exp > + > +set dp_location [gdb_get_line_number "set dprintf here"] > + > +if [prepare_for_testing $expfile $executable $srcfile {debug}] { > + untested "failed to prepare for trace tests" This is not a trace test. Actually, prepare_for_testing already calls untested with whatever we pass it as first argument. I think it'd be the first such call in the testsuite, but I think this would be better: if [prepare_for_testing "failed to prepare for trace tests" \ $executable $srcfile {debug}] { return -1 } > + > +if ![runto_main] { > + fail "Can't run to main" > + return -1 > +} > + > +gdb_test "dprintf $dp_location, \"%d\\n\", x" \ > + "Dprintf .*" > + > +gdb_test "next" ".*" "next 1" > +gdb_test "next" ".*" "next 2" Please use more string regexes for these, that confirm the next stopped at the right lines. > +# Test inferior doesn't exit. Check that the inferior didn't exit. > +gdb_test "p x" ".* = 6" With the stricter regexes, I don't think this test would be necessary though. -- Pedro Alves