This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC] PR 15075 dprintf interferes with "next"
- From: Pedro Alves <palves at redhat dot com>
- To: Hui Zhu <teawater at gmail dot com>
- Cc: gdb-patches at sourceware dot org, Keith Seitz <keiths at redhat dot com>, Tom Tromey <tromey at redhat dot com>, Yao Qi <yao at codesourcery dot com>
- Date: Wed, 22 May 2013 13:46:35 +0100
- Subject: Re: [RFC] PR 15075 dprintf interferes with "next"
- References: <1361192891-29341-1-git-send-email-yao at codesourcery dot com> <8738wpd3qe dot fsf at fleche dot redhat dot com> <CANFwon3DymreN3ost7ZSrd0deb_sBC6YTzk1fpv8k+7d0ADnLg at mail dot gmail dot com> <5176C14B dot 6010603 at redhat dot com> <CANFwon1tTZTVcn7ieP5=HnPy+2hCKP1my-UE1-Xosp=Q6WrGug at mail dot gmail dot com> <51774714 dot 9060306 at codesourcery dot com> <CANFwon2Q_=++=WbZD25Ch6qnRtpmuVBOsmkqhevyMAnM3y+ZZA at mail dot gmail dot com> <CANFwon0A4wmgEuREeQHYfRWMfn_cct6dHCZ6_oFH+wVr1Wym4A at mail dot gmail dot com> <51969A92 dot 80003 at redhat dot com> <CANFwon3OaNBrvw4jwv_RJ8ws+SVx9NctfTw2FSDVowaxAmgF9Q at mail dot gmail dot com>
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 <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.
> + 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