Re: [RFC] PR 15075 dprintf interferes with "next"

[Pedro Alves <palves@redhat.com>]

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:

 	(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.

> +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
)

> --- /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
}

> +    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?

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?

-- 
Pedro Alves