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

[Hui Zhu <hui_zhu@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 5408 bytes --]

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  <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_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  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>
	    Pedro Alves  <palves@redhat.com>

	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 " {
>


[-- Attachment #2: dprintf-continue.txt --]
[-- Type: text/plain, Size: 4469 bytes --]

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

[-- Attachment #3: dprintf-continue-test.txt --]
[-- Type: text/plain, Size: 6539 bytes --]

--- /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,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 <http://www.gnu.org/licenses/>.  */
+
+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 <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 .*"
+
+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