From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23438 invoked by alias); 25 Jun 2013 08:53:39 -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 23428 invoked by uid 89); 25 Jun 2013 08:53:38 -0000 X-Spam-SWARE-Status: No, score=-4.4 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_XP autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 25 Jun 2013 08:53:36 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1UrP0H-0002Er-OO from Hui_Zhu@mentor.com ; Tue, 25 Jun 2013 01:53:33 -0700 Received: from SVR-ORW-FEM-05.mgc.mentorg.com ([147.34.97.43]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Tue, 25 Jun 2013 01:53:34 -0700 Received: from [127.0.0.1] (147.34.91.1) by svr-orw-fem-05.mgc.mentorg.com (147.34.97.43) with Microsoft SMTP Server id 14.2.247.3; Tue, 25 Jun 2013 01:53:32 -0700 Message-ID: <51C95A89.2030809@mentor.com> Date: Tue, 25 Jun 2013 09:14:00 -0000 From: Hui Zhu User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Thunderbird/22.0 MIME-Version: 1.0 To: Pedro Alves CC: Hui Zhu , Tom Tromey , , Keith Seitz , Yao Qi Subject: Re: [RFC] PR 15075 dprintf interferes with "next" 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> <51ACD6ED.7040604@redhat.com> <51C0A274.9010808@redhat.com> <51C7CA0C.5020809@mentor.com> <51C8A9FA.1050001@redhat.com> In-Reply-To: <51C8A9FA.1050001@redhat.com> Content-Type: multipart/mixed; boundary="------------030101000408010605060304" X-Virus-Found: No X-SW-Source: 2013-06/txt/msg00691.txt.bz2 --------------030101000408010605060304 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 5408 On 06/25/13 04:20, Pedro Alves wrote: > On 06/24/2013 05:24 AM, Hui Zhu wrote: > >>> 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. > > I suspect the "exec sleep 1" is masking it. If you remove the sleep, > then the test fails. > >> >>> >>> 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&"? > > Just doing: > > -send_gdb "continue &\n" > +gdb_test "continue &" "Continuing\\." > > will consume the prompt. Otherwise, we'd do like > e.g., completion.exp does: > > # Eat the prompt > gdb_expect { > -re "$gdb_prompt " { > pass "$test (eat prompt)" > } > timeout { > fail "(timeout) $test (eat prompt)" > } > } > > We still need the sleep, but for different a reason -- be sure > to wait for the dprintf to trigger -- we need GDB to print the > prompt before the program reaches the dprintf. I think it's slightly > better to do the sleep on the inferior program side. We can expect > the "At foo entry" output from the dprintf, as the default "set dprintf-style" > is "gdb". > > I see now that the > > set test "inferior stopped" > gdb_test_multiple "" $test { > -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n" { > pass $test > } > } > > case doesn't need to eat the prompt, as that output appears > after the prompt: > > (gdb) PASS: gdb.base/dprintf-non-stop.exp: continue & > At foo entry > PASS: gdb.base/dprintf-non-stop.exp: dprintf triggered > interrupt > (gdb) > [process 6106] #1 stopped. > PASS: gdb.base/dprintf-non-stop.exp: interrupt > PASS: gdb.base/dprintf-non-stop.exp: inferior stopped > > and the "interrupt" test already eats it. > > See patchlet on top of yours below. Please merge it with > yours, and push the result in (and post it, for the archives). Hi Pedro, Thanks for your patch. I merged it with old patch together. Please help me review it. Thanks, Hui 2013-06-25 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_after_condition_true): New function. (initialize_breakpoint_ops): Set dprintf_after_condition_true. * breakpoint.h (breakpoint_ops): Add after_condition_true. 2013-06-25 Yao Qi Hui Zhu Pedro Alves 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. > > Thanks, > Pedro Alves > > --- > gdb/testsuite/gdb.base/dprintf-non-stop.c | 1 + > gdb/testsuite/gdb.base/dprintf-non-stop.exp | 17 +++++++++++++++-- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/gdb/testsuite/gdb.base/dprintf-non-stop.c b/gdb/testsuite/gdb.base/dprintf-non-stop.c > index 2198848..2d25d9e 100644 > --- a/gdb/testsuite/gdb.base/dprintf-non-stop.c > +++ b/gdb/testsuite/gdb.base/dprintf-non-stop.c > @@ -23,6 +23,7 @@ foo () > int > main () > { > + sleep (1); > foo (); > sleep (3); > return 0; > diff --git a/gdb/testsuite/gdb.base/dprintf-non-stop.exp b/gdb/testsuite/gdb.base/dprintf-non-stop.exp > index c3fb85e..707f913 100644 > --- a/gdb/testsuite/gdb.base/dprintf-non-stop.exp > +++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp > @@ -36,9 +36,22 @@ if ![runto main] { > > gdb_test "dprintf foo,\"At foo entry\\n\"" "Dprintf .*" > > -send_gdb "continue &\n" > -exec sleep 1 > +gdb_test "continue &" "Continuing\\." > > +# Wait for the dprintf to trigger. > +set test "dprintf triggered" > +gdb_expect { > + -re "At foo entry" { > + pass "$test" > + } > + timeout { > + fail "$test (timeout)" > + } > +} > + > +# Now test that we're still able to issue commands. GDB used to > +# implement re-resuming from dprintfs with a synchronous "continue" in > +# the dprintf's command list, which stole the prompt from the user. > set test "interrupt" > gdb_test_multiple $test $test { > -re "interrupt\r\n$gdb_prompt " { > --------------030101000408010605060304 Content-Type: text/plain; charset="us-ascii"; name="dprintf-continue.txt" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="dprintf-continue.txt" Content-length: 4469 --- 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 --------------030101000408010605060304 Content-Type: text/plain; charset="us-ascii"; name="dprintf-continue-test.txt" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="dprintf-continue-test.txt" Content-length: 6539 --- /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 . */ + +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 . + +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,30 @@ +/* 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 . */ + +void +foo () +{ +} + +int +main () +{ + sleep (1); + foo (); + sleep (3); + return 0; +} --- /dev/null +++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp @@ -0,0 +1,67 @@ +# 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 . + +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 .*" + +gdb_test "continue &" "Continuing\\." + +# Wait for the dprintf to trigger. +set test "dprintf triggered" +gdb_expect { + -re "At foo entry" { + pass "$test" + } + timeout { + fail "$test (timeout)" + } +} + +# Now test that we're still able to issue commands. GDB used to +# implement re-resuming from dprintfs with a synchronous "continue" in +# the dprintf's command list, which stole the prompt from the user. +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 --------------030101000408010605060304--