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