This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
Hi Pedro and Tom, Thanks for your review. On Wed, May 22, 2013 at 8:46 PM, Pedro Alves <palves@redhat.com> 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 <hui@codesourcery.com> >> + >> + 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/>. */ >> + >> +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 <tromey@redhat.com> wrote: >>>>>> "Hui" == Hui Zhu <teawater@gmail.com> 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 <yao@codesourcery.com> Hui Zhu <hui@codesourcery.com> Pedro Alves <palves@redhat.com> 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 <yao@codesourcery.com> Hui Zhu <hui@codesourcery.com> 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.
Attachment:
dprintf-continue.txt
Description: Text document
Attachment:
dprintf-continue-test.txt
Description: Text document
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |