From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20505 invoked by alias); 3 Jun 2013 04:07:06 -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 20494 invoked by uid 89); 3 Jun 2013 04:07:06 -0000 X-Spam-SWARE-Status: No, score=-3.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,SPF_PASS autolearn=ham version=3.3.1 Received: from mail-ob0-f178.google.com (HELO mail-ob0-f178.google.com) (209.85.214.178) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 03 Jun 2013 04:07:04 +0000 Received: by mail-ob0-f178.google.com with SMTP id fb19so6461821obc.9 for ; Sun, 02 Jun 2013 21:07:02 -0700 (PDT) X-Received: by 10.60.65.38 with SMTP id u6mr9295256oes.52.1370232422480; Sun, 02 Jun 2013 21:07:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.60.38.193 with HTTP; Sun, 2 Jun 2013 21:06:22 -0700 (PDT) In-Reply-To: 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> <519CBE2B.7060007@redhat.com> From: Hui Zhu Date: Mon, 03 Jun 2013 04:07:00 -0000 Message-ID: Subject: Re: [RFC] PR 15075 dprintf interferes with "next" To: Pedro Alves , Tom Tromey Cc: gdb-patches@sourceware.org, Keith Seitz , Yao Qi Content-Type: text/plain; charset=ISO-8859-1 X-SW-Source: 2013-06/txt/msg00002.txt.bz2 Ping http://sourceware.org/ml/gdb-patches/2013-05/msg00958.html Thanks, Hui On Tue, May 28, 2013 at 8:01 AM, Hui Zhu wrote: > Hi Pedro and Tom, > > Thanks for your review. > > On Wed, May 22, 2013 at 8:46 PM, Pedro Alves wrote: >> 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. > > I found a issue about remote test and non-stop. Because it doesn't > have a lot of relation with dprintf, I will post a separate patch for > that later. > >> >>>> > 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. > > Added dprintf_create_breakpoints_sal. When dprintf is created, auto > set silent to 0. > >> >>>> > >>>>> >> + 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: >> > > Thanks for this patch. I merged it to dprintf-continue patch. > >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 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. > > Fixed both dprintf-next.exp and dprintf-non-stop.exp. > >> >>> 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" > > This part is fixed. > >> >> >>> --- /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. > > Changed this part to sleep(3) > >> >>> + return 0; >> >>> +} >>> \ No newline at end of file >> >> Please watch out for these, and add a newline. >> > > Fixed. > >>> +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. >> > > After change this part to: > gdb_test_multiple "" $test { > -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n$gdb_prompt" { > pass $test > } > } > I got: > Running ../../../src/gdb/testsuite/gdb.base/dprintf-non-stop.exp ... > FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout) > > I thought the reason is: > (gdb) PASS: gdb.base/dprintf-non-stop.exp: interrupt > > [process 24118] #1 stopped. > 0x00002aaaaad8f830 in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6 > FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout) > > There is not $gdb_prompt. > > > On Wed, May 22, 2013 at 10:04 PM, Tom Tromey wrote: >>>>>>> "Hui" == Hui Zhu writes: >> >> Hui> So I change this part to: >> Hui> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } { >> Hui> return -1 >> Hui> } >> >> I think it is better to use the variables created by standard_testfile. > > Fixed. > > The attachments are the new patches. > > Best, > Hui > > 2013-05-28 Yao Qi > Hui Zhu > Pedro Alves > > PR breakpoints/15075 > PR breakpoints/15434 > * breakpoint.c (bpstat_stop_status): Call > b->ops->after_condition_true. > (update_dprintf_command_list): Don't append "continue" command > to the command list of dprintf breakpoint. > (base_breakpoint_after_condition_true): New function. > (base_breakpoint_ops): Add base_breakpoint_after_condition_true. > (dprintf_create_breakpoints_sal, > dprintf_after_condition_true): New functions. > (initialize_breakpoint_ops): Set dprintf_create_breakpoints_sal > and dprintf_after_condition_true. > * breakpoint.h (breakpoint_ops): Add after_condition_true. > > 2013-05-28 Yao Qi > Hui Zhu > > PR breakpoints/15075 > PR breakpoints/15434 > * gdb.base/dprintf-next.c: New file. > * gdb.base/dprintf-next.exp: New file. > * gdb.base/dprintf-non-stop.c: New file. > * gdb.base/dprintf-non-stop.exp: New file. > * gdb.base/dprintf.exp: Don't check "continue" in the output > of "info breakpoints". > * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify): > Don't check "continue" in script field.