From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14256 invoked by alias); 22 May 2013 12:46:45 -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 14247 invoked by uid 89); 22 May 2013 12:46:45 -0000 X-Spam-SWARE-Status: No, score=-8.2 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 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; Wed, 22 May 2013 12:46:44 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r4MCkcAI009147 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Wed, 22 May 2013 08:46:38 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r4MCkZQg013637; Wed, 22 May 2013 08:46:36 -0400 Message-ID: <519CBE2B.7060007@redhat.com> Date: Wed, 22 May 2013 12:46: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> <51969A92.80003@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-05/txt/msg00832.txt.bz2 On 05/22/2013 11:21 AM, Hui Zhu wrote: > I tried it on fedora and looks it works OK most of times. I only got > trouble is when run this test with remote, [runto main] with > "non-stop" is open will fail sometime. Sorry, but "most of the times" is not acceptable. Can you please figure out why that is so? This should never fail. >> > 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; > I am not sure about this part. > If my understanding about Tom's review in before is right, the action > of dprintf command should be handled when GDB check the condition. > And after that, dprintf not need be handled. Yes, but I don't see how that counters my suggestion. >> > >>> >> + bpstat_do_actions_1 (&bs); >>> >> + bs->next = tmp; >>> >> +} >> > >> > Could you add some comments explaining what this is >> > doing, and why? > I update this function to following part: > /* Implement the "after_cond" breakpoint_ops method for dprintf. */ > > static void > dprintf_after_cond (struct bpstats *bs) > { > struct cleanup *old_chain; > struct breakpoint *b = bs->breakpoint_at; > > bs->commands = b->commands; > incref_counted_command_line (bs->commands); I still think there's no point in moving this to the after_condition_true hook, if both existing implementations end up doing it themselves. See below. > > /* Because function bpstat_do_actions_1 will execute all the command > list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to > NULL. */ > old_chain = make_cleanup_restore_ptr ((void **)&bs->next); That cast is invalid actually. Older gcc's will complain about the aliasing violation. I think we can get away without this, by passing a separate list with only one entry to bpstat_do_actions_1. > bs->next = NULL; > bpstat_do_actions_1 (&bs); > do_cleanups (old_chain); > > /* This dprintf need not be handled after this function > because its command list is executed by bpstat_do_actions_1. > Clear STOP and PRINT for that. */ > bs->stop = 0; > bs->print = 0; Thanks. I was more looking for general design comments on why we run the command list here. This also should explain why clear stop here, instead of on the more natural check_status. So all in all, if it works for you (seems to work fine for me), I'd rather do as this patch on top of yours does: ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ gdb/breakpoint.c | 62 ++++++++++++++++++++++++++++++++---------------------- 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 7ef5703..181aecc 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5292,6 +5292,15 @@ bpstat_stop_status (struct address_space *aspace, 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_condition_true (bs); } @@ -12742,15 +12751,7 @@ base_breakpoint_explains_signal (struct breakpoint *b) static void base_breakpoint_after_condition_true (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; + /* Nothing to do. */ } struct breakpoint_ops base_breakpoint_ops = @@ -13368,30 +13369,41 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp) } /* Implement the "after_condition_true" breakpoint_ops method for - dprintf. */ + dprintf. + + dprintf's are implemented with regular commands in their command + list, but we run the commands here instead of before presenting the + stop to the user, as dprintf's don't actually cause a stop. This + also makes it so that the commands of multiple dprintfs at the same + address are all handled. */ static void dprintf_after_condition_true (struct bpstats *bs) { struct cleanup *old_chain; - struct breakpoint *b = bs->breakpoint_at; + struct bpstats tmp_bs = { NULL }; + struct bpstats *tmp_bs_p = &tmp_bs; - bs->commands = b->commands; - incref_counted_command_line (bs->commands); + /* dprintf's never cause a stop. This wasn't set in the + check_status hook instead because that would make the dprintf's + condition not be evaluated. */ + bs->stop = 0; - /* Because function bpstat_do_actions_1 will execute all the command - list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to - NULL. */ - old_chain = make_cleanup_restore_ptr ((void **)&bs->next); - bs->next = NULL; - bpstat_do_actions_1 (&bs); - do_cleanups (old_chain); + /* Run the command list here. Take ownership of it instead of + copying. We never want these commands to run later in + bpstat_do_actions, if a breakpoint that causes a stop happens to + be set at same address as this dprintf, or even if running the + commands here throws. */ + tmp_bs.commands = bs->commands; + bs->commands = NULL; + old_chain = make_cleanup_decref_counted_command_line (&tmp_bs.commands); - /* This dprintf need not be handled after this function - because its command list is executed by bpstat_do_actions_1. - Clear STOP and PRINT for that. */ - bs->stop = 0; - bs->print = 0; + bpstat_do_actions_1 (&tmp_bs_p); + + /* 'tmp_bs.commands' will usually be NULL by now, but + bpstat_do_actions_1 may return early without processing the whole + list. */ + do_cleanups (old_chain); } /* The breakpoint_ops structure to be used on static tracepoints with ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >> +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 >> > } > After I use this part of code, I got: > ERROR: tcl error sourcing ../../../src/gdb/testsuite/gdb.base/dprintf-next.exp. > ERROR: wrong # args: should be "prepare_for_testing testname > executable ?sources? ?options?" > while executing > "prepare_for_testing "failed to prepare for dprintf next tests"" > invoked from within > "if [prepare_for_testing "failed to prepare for dprintf next tests" > $executable $srcfile {debug}] { > return -1 > }" Hmm? Oh, it looks like you forgot the '\' at the end of the line. Please try again. > So I change this part to: > if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } { 'dprintf-next.exp' really isn't magical. It's just a string. :-) > return -1 > } >>> >> +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. > Fixed. Sorry, I should have been clearer. Don't do that by checking line numbers though, as those change when more functions are added to the test. Just make each line look different instead. > + > +gdb_test "dprintf $dp_location, \"%d\\n\", x" \ > + "Dprintf .*" Please add an explicit 3rd parameter to gdb_test, so that the line number doesn't appear in gdb.sum. > + > +gdb_test "next" "23.*\\+\\+x.*" "next 1" > +gdb_test "next" "24.*\\+\\+x.*" "next 2" Do something like this: gdb_test "next" "\\+\\+x;" "next 1" gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2" You could add a comment to the first stepped line to make that clearer: gdb_test "next" "\\+\\+x; /* step here.*" "next 1" gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2" > --- /dev/null > +++ b/gdb/testsuite/gdb.base/dprintf-non-stop.c > @@ -0,0 +1,30 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright (C) 2013 Free Software Foundation, Inc. > + Contributed by Hui Zhu > + > + 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 . */ > + > +void > +foo () > +{ > +} > + > +int > +main () > +{ > + foo (); > + while (1); If something goes wrong, like GDB crashing, this could leave the test running forever, pegging the CPU. Just make it sleep for some time instead. > + return 0; > +} > \ No newline at end of file Please watch out for these, and add a newline. > +gdb_test "dprintf foo,\"At foo entry\\n\"" "Dprintf .*" > + > +send_gdb "continue &\n" > +exec sleep 1 > +set test "interrupt" > +gdb_test_multiple $test $test { > + -re "interrupt\r\n$gdb_prompt " { > + pass $test > + } > +} > + > +set test "inferior stopped" > +gdb_test_multiple "" $test { > + -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n" { > + pass $test > + } > +} This leaves the prompt in the expect buffer. I think this is likely to confuse the following test that runs. -- Pedro Alves