[RFC] PR 15075 dprintf interferes with "next"
Hui Zhu
hui_zhu@mentor.com
Mon Jun 24 08:36:00 GMT 2013
Hi Pedro,
Thanks for your review.
On 06/19/13 02:09, Pedro Alves wrote:
> On 06/07/2013 04:15 AM, Hui Zhu wrote:
>
>> 2013-06-07 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.
>
> Context is split in multiple lines with '()'s, not ','s:
Fixed.
>
> (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.
>>
>> };
>>
>> /* Default breakpoint_ops methods. */
>> @@ -13351,6 +13353,76 @@ dprintf_print_recreate (struct breakpoin
>> print_recreate_thread (tp, fp);
>> }
>>
>> +/* Implement the "create_breakpoints_sal" breakpoint_ops method for
>> + dprintf. */
>> +
>> +static void
>> +dprintf_create_breakpoints_sal (struct gdbarch *gdbarch,
>> + struct linespec_result *canonical,
>> + struct linespec_sals *lsal,
>> + char *cond_string,
>> + char *extra_string,
>> + enum bptype type_wanted,
>> + enum bpdisp disposition,
>> + int thread,
>> + int task, int ignore_count,
>> + const struct breakpoint_ops *ops,
>> + int from_tty, int enabled,
>> + int internal, unsigned flags)
>> +{
>> + struct breakpoint *b;
>> +
>> + create_breakpoints_sal_default (gdbarch, canonical, lsal,
>> + cond_string, extra_string,
>> + type_wanted,
>> + disposition, thread, task,
>> + ignore_count, ops, from_tty,
>> + enabled, internal, flags);
>> +
>> + b = get_breakpoint (breakpoint_count);
>> + gdb_assert (b != NULL);
>> +
>> + breakpoint_set_silent (b, 0);
>> +}
>
> When I tried it last, I didn't find making the dprintf
> explicitly silent necessary, given:
>
> /* 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;
>
> So did it turn out really necessary for some reason? Please
> don't leave such changes between revisions unexplained.
Sorry for my misunderstand your comments. They were removed.
>
>> +gdb_test "next" "\\+\\+x\;.*\/\* Next without dprintf.*" "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>
>
> (
> IMO, we should stop adding these (like glibc has done so too):
> http://sourceware.org/ml/gdb-patches/2013-05/msg00253.html
> )
Removed.
>
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
>> @@ -0,0 +1,54 @@
>> +# 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/>.
>> +
>> +if [is_remote target] then {
>> + unsupported "Dprintf with non-stop is not supported."
>
> That's not exactly true. It's supported, but testing is racy at the moment.
> Write instead:
>
> if [is_remote target] then {
> # Testing with remote/non-stop is racy at the moment.
> unsupported "Testing dprintf with remote/non-stop is not supported."
> return 0
> }
Fixed.
>
>> + return 0
>> +}
>> +
>> +standard_testfile
>> +
>> +if [prepare_for_testing "failed to prepare for dprintf with non-stop" \
>> + ${testfile} ${srcfile} {debug}] {
>> + return -1
>> +}
>> +
>> +gdb_test_no_output "set target-async on"
>> +gdb_test_no_output "set non-stop on"
>> +
>> +if ![runto main] {
>> + fail "Can't run to main"
>> + return -1
>> +}
>> +
>> +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
>
> Hmm, this still looks racy to me, even on native targets.
> "continue &" produces a gdb prompt. gdb_test_multiple
> inside has a match for the prompt:
>
> -re "\r\n$gdb_prompt $" {
> if ![string match "" $message] then {
> fail "$message"
> }
> set result 1
> }
>
> So if the expect happens to read
>
> continue &
> (gdb)
>
> from the buffer, we'll hit the fail. Doesn't the read1 hack catch this?
EXPECT=../contrib/expect-read1.sh make check RUNTESTFLAGS="dprintf-non-stop.exp" is OK in my part.
>
> We need to consume the prompt after that "continue&".
>
>> + }
>> +}
>> +
>> +set test "inferior stopped"
>> +gdb_test_multiple "" $test {
>> + -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n" {
>> + pass $test
>> + }
>> +}
>
> Likewise?
>
I just copy this part of code from "gdb.base/async-shell.exp".
Could you give me some help about how to consume the prompt after that "continue&"?
Thanks,
Hui
-------------- next part --------------
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5299,6 +5299,8 @@ bpstat_stop_status (struct address_space
if (command_line_is_silent (bs->commands
? bs->commands->commands : NULL))
bs->print = 0;
+
+ b->ops->after_condition_true (bs);
}
}
@@ -8934,25 +8936,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);
@@ -12738,6 +12731,14 @@ base_breakpoint_explains_signal (struct
return BPSTAT_SIGNAL_HIDE;
}
+/* The default "after_condition_true" method. */
+
+static void
+base_breakpoint_after_condition_true (struct bpstats *bs)
+{
+ /* Nothing to do. */
+}
+
struct breakpoint_ops base_breakpoint_ops =
{
base_breakpoint_dtor,
@@ -12757,7 +12758,8 @@ struct breakpoint_ops base_breakpoint_op
base_breakpoint_create_sals_from_address,
base_breakpoint_create_breakpoints_sal,
base_breakpoint_decode_linespec,
- base_breakpoint_explains_signal
+ base_breakpoint_explains_signal,
+ base_breakpoint_after_condition_true,
};
/* Default breakpoint_ops methods. */
@@ -13351,6 +13353,44 @@ dprintf_print_recreate (struct breakpoin
print_recreate_thread (tp, fp);
}
+/* Implement the "after_condition_true" breakpoint_ops method for
+ 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 bpstats tmp_bs = { NULL };
+ struct bpstats *tmp_bs_p = &tmp_bs;
+
+ /* 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;
+
+ /* 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);
+
+ 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
markers (`-m'). */
@@ -15847,6 +15887,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_condition_true = dprintf_after_condition_true;
}
/* Chain containing all defined "enable breakpoint" subcommands. */
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -614,6 +614,10 @@ struct breakpoint_ops
'catch signal' interact properly with 'handle'; see
bpstat_explains_signal. */
enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
+
+ /* Called after evaluating the breakpoint's condition,
+ and only if it evaluated true. */
+ void (*after_condition_true) (struct bpstats *bs);
};
/* Helper for breakpoint_ops->print_recreate implementations. Prints
-------------- next part --------------
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-next.c
@@ -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)
+{
+ int x = 5;
+
+ ++x; /* Next without dprintf. */
+ ++x; /* Set dprintf here. */
+ return x - 7;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-next.exp
@@ -0,0 +1,36 @@
+# 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 "failed to prepare for dprintf with next" \
+ ${testfile} ${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" "\\+\\+x\;.*\/\* Next without dprintf.*" "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,29 @@
+/* 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/>. */
+
+void
+foo ()
+{
+}
+
+int
+main ()
+{
+ foo ();
+ sleep (3);
+ return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
@@ -0,0 +1,54 @@
+# 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/>.
+
+if [is_remote target] then {
+ # Testing with remote/non-stop is racy at the moment.
+ unsupported "Testing dprintf with remote/non-stop is not supported."
+ return 0
+}
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare for dprintf with non-stop" \
+ ${testfile} ${srcfile} {debug}] {
+ return -1
+}
+
+gdb_test_no_output "set target-async on"
+gdb_test_no_output "set non-stop on"
+
+if ![runto main] {
+ fail "Can't run to main"
+ return -1
+}
+
+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
+ }
+}
--- 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" \
@@ -111,7 +109,6 @@ gdb_test_multiple "set dprintf-style age
}
if $target_can_dprintf {
-
gdb_run_cmd
gdb_test "" "Breakpoint"
@@ -135,4 +132,3 @@ if $target_can_dprintf {
gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
"Set dprintf style to an unrecognized type"
-
--- a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
+++ b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
@@ -96,7 +96,7 @@ proc test_insert_delete_modify { } {
$test
set test "dprintf marker, \"arg\" \""
mi_gdb_test $test \
- {.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*,script=\{\"printf \\\\\"arg\\\\\" \\\\\"\",\"continue\"\}.*\}\r\n\^done} \
+ {.*=breakpoint-created,bkpt=\{number="6",type="dprintf".*,script=\{\"printf \\\\\"arg\\\\\" \\\\\"\"\}.*\}\r\n\^done} \
$test
# 2. when modifying condition
More information about the Gdb-patches
mailing list