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: Fri, 17 May 2013 22:01:06 +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>
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 <yao@codesourcery.com>
> Hui Zhu <hui@codesourcery.com>
>
> 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 <yao@codesourcery.com>
> Hui Zhu <hui@codesourcery.com>
>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +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