[RFC] PR 15075 dprintf interferes with "next"

[RFC] PR 15075 dprintf interferes with "next"

[Yao Qi]

Hi,
I'd like to get your comments on this patch, as I think my patch is
hacky but I am unable to figure out a correct one.  This patch fixes more
problems than PR 15075 mentioned, they are,

  - When step a source line by "next" command, and there is a dprintf
    on that line, "next" command will behave like "continue".
  - Two dprintf are set on the same address.  Only one printf is
    executed.
  - dprintf and regular breakpoint are set on the same address,
    inferior doesn't stop.

In order to fix these problems, I don't append "continue" command to
dprintf breakpoint, and after all commands of all breakpoints (caused
the stop) are executed, execute "continue" command if
    1) the inferior is not proceeded by these commands
    2) and the stop is only caused by dprintf (no other types of
breakpoint)
    3) the inferior is not in the state of stepping a source line or
an instruction (to fix PR 15075).

My patch fixes all the three known problems, but I think it is still
hacky.

The printf stuff is implemented as command, it is convenient to move
them to the target side.  I tried to change the bpstat saying that
"don't stop", but the command can't be executed.  We have to keep
bpstat saying that "stop" for dprintf.  I also thought about Tom's
suggestion about Python "Breakpoint.stop" API, but "Breakpoint.stop"
acts as condition instead of command, so I don't go that way.

Known issue: from the user's perspective, inferior stops at the dprintf
breakpoint if dprintf breakpoint and regular breakpoint are set at
the same address.  I'll fix it if my patch is on the right track.

gdb:

2013-02-18  Yao Qi  <yao@codesourcery.com>

	* breakpoint.c (bpstat_do_actions_1): Change argument from
	"bpstat *" to "struc thread_info *".
	(bpstat_do_actions_1): Execute "continue" command if BS has
	dprintf breakpoint only.
	(bpstat_do_actions): Caller update.
	(update_dprintf_command_list): Don't append "continue" command
	to the command of dprintf breakpoint.
---
 gdb/breakpoint.c |   68 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fb57a57..878ab08 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -4263,20 +4263,26 @@ command_line_is_silent (struct command_line *cmd)
 }
 
 /* Execute all the commands associated with all the breakpoints at
-   this location.  Any of these commands could cause the process to
-   proceed beyond this point, etc.  We look out for such changes by
-   checking the global "breakpoint_proceeded" after each command.
+   the location where THREAD stops.  Any of these commands could cause
+   the process to proceed beyond this point, etc.  We look out for
+   such changes by checking the global "breakpoint_proceeded" after
+   each command.
 
    Returns true if a breakpoint command resumed the inferior.  In that
    case, it is the caller's responsibility to recall it again with the
    bpstat of the current thread.  */
 
 static int
-bpstat_do_actions_1 (bpstat *bsp)
+bpstat_do_actions_1 (struct thread_info *thread)
 {
+  bpstat *bsp = &thread->control.stop_bpstat;
   bpstat bs;
   struct cleanup *old_chain;
   int again = 0;
+  /* Breakpoint dprintf in list BS.  */
+  int bkpt_dprintf = 0;
+  /* Breakpoint other than dprintf in list BS.  */
+  int bkpt_other_than_dprintf = 0;
 
   /* Avoid endless recursion if a `source' command is contained
      in bs->commands.  */
@@ -4298,6 +4304,13 @@ bpstat_do_actions_1 (bpstat *bsp)
       struct command_line *cmd;
       struct cleanup *this_cmd_tree_chain;
 
+      if (bs->breakpoint_at != NULL)
+	{
+	  if (bs->breakpoint_at->type == bp_dprintf)
+	    bkpt_dprintf = 1;
+	  else
+	    bkpt_other_than_dprintf = 1;
+	}
       /* Take ownership of the BSP's command tree, if it has one.
 
          The command tree could legitimately contain commands like
@@ -4356,6 +4369,33 @@ bpstat_do_actions_1 (bpstat *bsp)
 	  break;
 	}
     }
+
+  /* Inferior is not proceed by commands.  */
+  if (!breakpoint_proceeded
+      /* No breakpoint of type other than dprintf.  If there is
+	 breakpoint of other types in BS, the inferior really needs
+	 stopped.  Don't execute command "continue".  */
+      && !bkpt_other_than_dprintf && bkpt_dprintf
+      && (0 != strcmp (dprintf_style, dprintf_style_agent))
+      /* GDB is not stepping a line.  When GDB is stepping a source
+	 line, don't execute command "continue", otherwise "next" will
+	 behave like "continue", which is not expected.  */
+      && !(thread->control.step_range_start >= 1
+	   && thread->control.step_range_end >= 1))
+    {
+      struct command_line *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");;
+
+      execute_control_command (cont_cmd_line);
+      if (!target_can_async_p ())
+	again = 1;
+    }
   do_cleanups (old_chain);
   return again;
 }
@@ -4374,7 +4414,7 @@ bpstat_do_actions (void)
        and only return when it is stopped at the next breakpoint, we
        keep doing breakpoint actions until it returns false to
        indicate the inferior was not resumed.  */
-    if (!bpstat_do_actions_1 (&inferior_thread ()->control.stop_bpstat))
+    if (!bpstat_do_actions_1 (inferior_thread ()))
       break;
 
   discard_cleanups (cleanup_if_error);
@@ -8947,25 +8987,15 @@ update_dprintf_command_list (struct breakpoint *b)
 		    _("Invalid dprintf style."));
 
   gdb_assert (printf_line != NULL);
-  /* Manufacture a printf/continue sequence.  */
+  /* Manufacture a printf command.  */
   {
-    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);
-- 
1.7.7.6

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

[Joel Brobecker]

> I'd like to get your comments on this patch, as I think my patch is
> hacky but I am unable to figure out a correct one.  This patch fixes more
> problems than PR 15075 mentioned, they are,
> 
>   - When step a source line by "next" command, and there is a dprintf
>     on that line, "next" command will behave like "continue".
>   - Two dprintf are set on the same address.  Only one printf is
>     executed.
>   - dprintf and regular breakpoint are set on the same address,
>     inferior doesn't stop.
> 
> In order to fix these problems, I don't append "continue" command to
> dprintf breakpoint, and after all commands of all breakpoints (caused
> the stop) are executed, execute "continue" command if
>     1) the inferior is not proceeded by these commands
>     2) and the stop is only caused by dprintf (no other types of
> breakpoint)
>     3) the inferior is not in the state of stepping a source line or
> an instruction (to fix PR 15075).
> 
> My patch fixes all the three known problems, but I think it is still
> hacky.

My first reaction is that we'd better just byte the bullet, and
implement this properly. In my opinion, something at the infrun/
handle_inferior_event level... Pedro is a specialist of the dreaded
handle_inferior_event function, so he might have a more precise
suggestion.

Interestingly enough, I did point out these limitations, but we still
went ahead, as we thought that the feature would be useful...

-- 
Joel

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

[Tom Tromey]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Yao> The printf stuff is implemented as command, it is convenient to move
Yao> them to the target side.  I tried to change the bpstat saying that
Yao> "don't stop", but the command can't be executed.  We have to keep
Yao> bpstat saying that "stop" for dprintf.  I also thought about Tom's
Yao> suggestion about Python "Breakpoint.stop" API, but "Breakpoint.stop"
Yao> acts as condition instead of command, so I don't go that way.

Pedro pointed out that 'stop' runs too early -- it makes "cond" not work
on a dprintf.  (BTW if there is no test for this, there should be... )

But, why not just do the work just after the condition is evaluated?
For example, it could be done with special handling in
bpstat_check_breakpoint_conditions, or by introducing a new
check_condition breakpoint_ops method, with the dprintf method calling
the super method before proceeding to "printf" and then returning 0.

I tend to think that the "commands" approach is just not a good one.

Tom

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

[Hui Zhu]

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

On Fri, Feb 22, 2013 at 12:35 AM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:
>
> Yao> The printf stuff is implemented as command, it is convenient to move
> Yao> them to the target side.  I tried to change the bpstat saying that
> Yao> "don't stop", but the command can't be executed.  We have to keep
> Yao> bpstat saying that "stop" for dprintf.  I also thought about Tom's
> Yao> suggestion about Python "Breakpoint.stop" API, but "Breakpoint.stop"
> Yao> acts as condition instead of command, so I don't go that way.
>
> Pedro pointed out that 'stop' runs too early -- it makes "cond" not work
> on a dprintf.  (BTW if there is no test for this, there should be... )
>
> But, why not just do the work just after the condition is evaluated?
> For example, it could be done with special handling in
> bpstat_check_breakpoint_conditions, or by introducing a new
> check_condition breakpoint_ops method, with the dprintf method calling
> the super method before proceeding to "printf" and then returning 0.
>
> I tend to think that the "commands" approach is just not a good one.
>
> Tom

Hi Tom,

I update this patch according to your comments:
Add after_cond to breakpoint_ops and let bpstat_stop_status call it
after cond check.

Then add normal code to base_breakpoint_after_cond handle setup of
other breakpoints and set dprintf special code to dprintf_after_cond
that do print work.

And I also updated test for this change.

Please help me review it.

Thanks,
Hui

2013-04-23  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR gdb/15075

	* breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
	(update_dprintf_command_list): Don't append "continue" command
	to the command of dprintf breakpoint.
	(base_breakpoint_after_cond): New.
	(base_breakpoint_ops): Add base_breakpoint_after_cond.
	(dprintf_after_cond): New.
	(initialize_breakpoint_ops): Set dprintf_after_cond.
	* breakpoint.h (breakpoint_ops): Add after_cond.

2013-04-23  Hui Zhu  <hui@codesourcery.com>

	PR gdb/15075

	* gdb.base/dprintf.exp: Remove "continue" from "info breakpoint"
	test.

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

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5296,13 +5296,7 @@ bpstat_stop_status (struct address_space
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
-	      if (b->silent)
-		bs->print = 0;
-	      bs->commands = b->commands;
-	      incref_counted_command_line (bs->commands);
-	      if (command_line_is_silent (bs->commands
-					  ? bs->commands->commands : NULL))
-		bs->print = 0;
+	      b->ops->after_cond (bs);
 	    }
 
 	}
@@ -8960,25 +8954,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);
@@ -12764,6 +12749,22 @@ base_breakpoint_explains_signal (struct
   return BPSTAT_SIGNAL_HIDE;
 }
 
+/* The default 'after_cond' method.  */
+
+static void
+base_breakpoint_after_cond (struct bpstats *bs)
+{
+  struct breakpoint *b = bs->breakpoint_at;
+
+  if (b->silent)
+    bs->print = 0;
+  bs->commands = b->commands;
+  incref_counted_command_line (bs->commands);
+  if (command_line_is_silent (bs->commands
+			      ? bs->commands->commands : NULL))
+  bs->print = 0;
+}
+
 struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -12783,7 +12784,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_cond,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -13377,6 +13379,23 @@ dprintf_print_recreate (struct breakpoin
   print_recreate_thread (tp, fp);
 }
 
+/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
+
+static void
+dprintf_after_cond (struct bpstats *bs)
+{
+  bpstat tmp;
+
+  bs->stop = 0;
+  bs->print = 0;
+  bs->commands = b->commands;
+  tmp = bs->next;
+  bs->next = tmp;
+  incref_counted_command_line (bs->commands);
+  bpstat_do_actions_1 (&bs);
+  bs->next = tmp;
+}
+
 /* The breakpoint_ops structure to be used on static tracepoints with
    markers (`-m').  */
 
@@ -15873,6 +15892,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_cond = dprintf_after_cond;
 }
 
 /* Chain containing all defined "enable breakpoint" subcommands.  */
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -614,6 +614,9 @@ struct breakpoint_ops
      'catch signal' interact properly with 'handle'; see
      bpstat_explains_signal.  */
   enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
+
+  /* Do some setup after check condition is true.  */
+  void (*after_cond) (struct bpstats *bs);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

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

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

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

[Keith Seitz]

On 04/23/2013 08:44 AM, Hui Zhu wrote:
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> +/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
> +
> +static void
> +dprintf_after_cond (struct bpstats *bs)
> +{
> +  bpstat tmp;
> +
> +  bs->stop = 0;
> +  bs->print = 0;
> +  bs->commands = b->commands;
> +  tmp = bs->next;
> +  bs->next = tmp;
> +  incref_counted_command_line (bs->commands);
> +  bpstat_do_actions_1 (&bs);
> +  bs->next = tmp;
> +}

This function must be incomplete. What is 'tmp' for? It is superfluous 
as written. 'b' is undefined, too.

Keith

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

[Hui Zhu]

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

On Wed, Apr 24, 2013 at 1:13 AM, Keith Seitz <keiths@redhat.com> wrote:
> On 04/23/2013 08:44 AM, Hui Zhu wrote:
>>
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> +/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
>> +
>> +static void
>> +dprintf_after_cond (struct bpstats *bs)
>> +{
>> +  bpstat tmp;
>> +
>> +  bs->stop = 0;
>> +  bs->print = 0;
>> +  bs->commands = b->commands;
>> +  tmp = bs->next;
>> +  bs->next = tmp;
>> +  incref_counted_command_line (bs->commands);
>> +  bpstat_do_actions_1 (&bs);
>> +  bs->next = tmp;
>> +}
>
>
> This function must be incomplete. What is 'tmp' for? It is superfluous as
> written. 'b' is undefined, too.
>
> Keith

Oops, I post a wrong patch.  This is the new one.
Sorry for that.

Thanks,
Hui

2013-04-23  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR gdb/15075

	* breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
	(update_dprintf_command_list): Don't append "continue" command
	to the command of dprintf breakpoint.
	(base_breakpoint_after_cond): New.
	(base_breakpoint_ops): Add base_breakpoint_after_cond.
	(dprintf_after_cond): New.
	(initialize_breakpoint_ops): Set dprintf_after_cond.
	* breakpoint.h (breakpoint_ops): Add after_cond.

2013-04-23  Hui Zhu  <hui@codesourcery.com>

	PR gdb/15075

	* gdb.base/dprintf.exp: Remove "continue" from "info breakpoint"
	test.

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

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5296,13 +5296,7 @@ bpstat_stop_status (struct address_space
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
-	      if (b->silent)
-		bs->print = 0;
-	      bs->commands = b->commands;
-	      incref_counted_command_line (bs->commands);
-	      if (command_line_is_silent (bs->commands
-					  ? bs->commands->commands : NULL))
-		bs->print = 0;
+	      b->ops->after_cond (bs);
 	    }
 
 	}
@@ -8960,25 +8954,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);
@@ -12764,6 +12749,22 @@ base_breakpoint_explains_signal (struct
   return BPSTAT_SIGNAL_HIDE;
 }
 
+/* The default 'after_cond' method.  */
+
+static void
+base_breakpoint_after_cond (struct bpstats *bs)
+{
+  struct breakpoint *b = bs->breakpoint_at;
+
+  if (b->silent)
+    bs->print = 0;
+  bs->commands = b->commands;
+  incref_counted_command_line (bs->commands);
+  if (command_line_is_silent (bs->commands
+			      ? bs->commands->commands : NULL))
+  bs->print = 0;
+}
+
 struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -12783,7 +12784,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_cond,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -13377,6 +13379,24 @@ dprintf_print_recreate (struct breakpoin
   print_recreate_thread (tp, fp);
 }
 
+/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
+
+static void
+dprintf_after_cond (struct bpstats *bs)
+{
+  bpstat tmp;
+  struct breakpoint *b = bs->breakpoint_at;
+
+  bs->stop = 0;
+  bs->print = 0;
+  bs->commands = b->commands;
+  tmp = bs->next;
+  bs->next = tmp;
+  incref_counted_command_line (bs->commands);
+  bpstat_do_actions_1 (&bs);
+  bs->next = tmp;
+}
+
 /* The breakpoint_ops structure to be used on static tracepoints with
    markers (`-m').  */
 
@@ -15873,6 +15893,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_cond = dprintf_after_cond;
 }
 
 /* Chain containing all defined "enable breakpoint" subcommands.  */
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -614,6 +614,9 @@ struct breakpoint_ops
      'catch signal' interact properly with 'handle'; see
      bpstat_explains_signal.  */
   enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
+
+  /* Do some setup after check condition is true.  */
+  void (*after_cond) (struct bpstats *bs);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

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

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

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

[Yao Qi]

On 04/24/2013 07:59 AM, Hui Zhu wrote:
> 2013-04-23  Yao Qi<yao@codesourcery.com>
> 	    Hui Zhu<hui@codesourcery.com>
>
> 	PR gdb/15075
         ^^^^^^^^^^^^ "PR breakpoints/15075"

>
> 	* breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
> 	(update_dprintf_command_list): Don't append "continue" command
> 	to the command of dprintf breakpoint.
> 	(base_breakpoint_after_cond): New.
> 	(base_breakpoint_ops): Add base_breakpoint_after_cond.
> 	(dprintf_after_cond): New.
> 	(initialize_breakpoint_ops): Set dprintf_after_cond.
> 	* breakpoint.h (breakpoint_ops): Add after_cond.
>
> 2013-04-23  Hui Zhu<hui@codesourcery.com>
>
> 	PR gdb/15075
>
> 	* gdb.base/dprintf.exp: Remove "continue" from "info breakpoint"
> 	test.
We also need to stop checking "continue" in 
gdb.mi/mi-breakpoint-changed.exp also, as what patch [1] does.  B.T.W, 
why don't you add gdb.base/pr15075.[c,exp] from patch [1]?

-- 
Yao (齐尧)

[1] http://sourceware.org/ml/gdb-patches/2013-02/msg00736.html

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

[Hui Zhu]

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

On Wed, Apr 24, 2013 at 10:44 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 04/24/2013 07:59 AM, Hui Zhu wrote:
>>
>> 2013-04-23  Yao Qi<yao@codesourcery.com>
>>             Hui Zhu<hui@codesourcery.com>
>>
>>         PR gdb/15075
>
>         ^^^^^^^^^^^^ "PR breakpoints/15075"
>
>
>>
>>         * breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
>>         (update_dprintf_command_list): Don't append "continue" command
>>         to the command of dprintf breakpoint.
>>         (base_breakpoint_after_cond): New.
>>         (base_breakpoint_ops): Add base_breakpoint_after_cond.
>>         (dprintf_after_cond): New.
>>         (initialize_breakpoint_ops): Set dprintf_after_cond.
>>         * breakpoint.h (breakpoint_ops): Add after_cond.
>>
>> 2013-04-23  Hui Zhu<hui@codesourcery.com>
>>
>>         PR gdb/15075
>>
>>         * gdb.base/dprintf.exp: Remove "continue" from "info breakpoint"
>>         test.
>
> We also need to stop checking "continue" in gdb.mi/mi-breakpoint-changed.exp
> also, as what patch [1] does.  B.T.W, why don't you add
> gdb.base/pr15075.[c,exp] from patch [1]?
>
> --
> Yao (齐尧)
>
> [1] http://sourceware.org/ml/gdb-patches/2013-02/msg00736.html

Oops, I am sorry that the discussion threads confused me.  Post a new
version for it.

And I didn't include the "disallows setting commands for dprintf"
patch because looks it is still in discussion.

Thanks,
Hui

2013-04-23  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR breakpoints/15075

	* breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
	(update_dprintf_command_list): Don't append "continue" command
	to the command of dprintf breakpoint.
	(base_breakpoint_after_cond): New.
	(base_breakpoint_ops): Add base_breakpoint_after_cond.
	(dprintf_after_cond): New.
	(initialize_breakpoint_ops): Set dprintf_after_cond.
	* breakpoint.h (breakpoint_ops): Add after_cond.

2013-04-23  Yao Qi  <yao@codesourcery.com>

	PR breakpoints/15075

	* 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.
	* gdb.base/pr15075.c: New.
	* gdb.base/pr15075.exp: New.

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

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5296,13 +5296,7 @@ bpstat_stop_status (struct address_space
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
-	      if (b->silent)
-		bs->print = 0;
-	      bs->commands = b->commands;
-	      incref_counted_command_line (bs->commands);
-	      if (command_line_is_silent (bs->commands
-					  ? bs->commands->commands : NULL))
-		bs->print = 0;
+	      b->ops->after_cond (bs);
 	    }
 
 	}
@@ -8960,25 +8954,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);
@@ -12764,6 +12749,22 @@ base_breakpoint_explains_signal (struct
   return BPSTAT_SIGNAL_HIDE;
 }
 
+/* The default 'after_cond' method.  */
+
+static void
+base_breakpoint_after_cond (struct bpstats *bs)
+{
+  struct breakpoint *b = bs->breakpoint_at;
+
+  if (b->silent)
+    bs->print = 0;
+  bs->commands = b->commands;
+  incref_counted_command_line (bs->commands);
+  if (command_line_is_silent (bs->commands
+			      ? bs->commands->commands : NULL))
+  bs->print = 0;
+}
+
 struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -12783,7 +12784,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_cond,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -13377,6 +13379,24 @@ dprintf_print_recreate (struct breakpoin
   print_recreate_thread (tp, fp);
 }
 
+/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
+
+static void
+dprintf_after_cond (struct bpstats *bs)
+{
+  bpstat tmp;
+  struct breakpoint *b = bs->breakpoint_at;
+
+  bs->stop = 0;
+  bs->print = 0;
+  bs->commands = b->commands;
+  tmp = bs->next;
+  bs->next = tmp;
+  incref_counted_command_line (bs->commands);
+  bpstat_do_actions_1 (&bs);
+  bs->next = tmp;
+}
+
 /* The breakpoint_ops structure to be used on static tracepoints with
    markers (`-m').  */
 
@@ -15873,6 +15893,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_cond = dprintf_after_cond;
 }
 
 /* Chain containing all defined "enable breakpoint" subcommands.  */
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -614,6 +614,9 @@ struct breakpoint_ops
      'catch signal' interact properly with 'handle'; see
      bpstat_explains_signal.  */
   enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
+
+  /* Do some setup after check condition is true.  */
+  void (*after_cond) (struct bpstats *bs);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

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

--- 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" \
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr15075.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;
+   ++x; /* set dprintf here */
+   return x - 7;
+ }
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr15075.exp
@@ -0,0 +1,38 @@
+# 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 $expfile $executable $srcfile {debug}] {
+    untested "failed to prepare for trace tests"
+    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" ".*" "next 1"
+gdb_test "next" ".*" "next 2"
+# Test inferior doesn't exit.
+gdb_test "p x" ".* = 6"
--- 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

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

[Hui Zhu]

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

Hi,

Because this patch also can handle bug 15434, I updated the test
patches to add test dprintf with non-stop mode.

Thanks,
Hui

2013-05-16  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR breakpoints/15075
	PR breakpoints/15434
	* breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
	(update_dprintf_command_list): Don't append "continue" command
	to the command of dprintf breakpoint.
	(base_breakpoint_after_cond): New.
	(base_breakpoint_ops): Add base_breakpoint_after_cond.
	(dprintf_after_cond): New.
	(initialize_breakpoint_ops): Set dprintf_after_cond.
	* breakpoint.h (breakpoint_ops): Add after_cond.

2013-05-16  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR breakpoints/15075
	PR breakpoints/15434
	* gdb.base/dprintf.exp: Don't check "continue" in the output
	of "info breakpoints".
	Add test dprintf with non-stop mode.
	* gdb.base/dprintf.c: Add a sleep for test dprintf with non-stop
	mode.
	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
	Don't check "continue" in script field.
	* gdb.base/pr15075.c: New.
	* gdb.base/pr15075.exp: New.

On Wed, Apr 24, 2013 at 11:18 AM, Hui Zhu <teawater@gmail.com> wrote:
> On Wed, Apr 24, 2013 at 10:44 AM, Yao Qi <yao@codesourcery.com> wrote:
>> On 04/24/2013 07:59 AM, Hui Zhu wrote:
>>>
>>> 2013-04-23  Yao Qi<yao@codesourcery.com>
>>>             Hui Zhu<hui@codesourcery.com>
>>>
>>>         PR gdb/15075
>>
>>         ^^^^^^^^^^^^ "PR breakpoints/15075"
>>
>>
>>>
>>>         * breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
>>>         (update_dprintf_command_list): Don't append "continue" command
>>>         to the command of dprintf breakpoint.
>>>         (base_breakpoint_after_cond): New.
>>>         (base_breakpoint_ops): Add base_breakpoint_after_cond.
>>>         (dprintf_after_cond): New.
>>>         (initialize_breakpoint_ops): Set dprintf_after_cond.
>>>         * breakpoint.h (breakpoint_ops): Add after_cond.
>>>
>>> 2013-04-23  Hui Zhu<hui@codesourcery.com>
>>>
>>>         PR gdb/15075
>>>
>>>         * gdb.base/dprintf.exp: Remove "continue" from "info breakpoint"
>>>         test.
>>
>> We also need to stop checking "continue" in gdb.mi/mi-breakpoint-changed.exp
>> also, as what patch [1] does.  B.T.W, why don't you add
>> gdb.base/pr15075.[c,exp] from patch [1]?
>>
>> --
>> Yao (齐尧)
>>
>> [1] http://sourceware.org/ml/gdb-patches/2013-02/msg00736.html
>
> Oops, I am sorry that the discussion threads confused me.  Post a new
> version for it.
>
> And I didn't include the "disallows setting commands for dprintf"
> patch because looks it is still in discussion.
>
> Thanks,
> Hui
>
> 2013-04-23  Yao Qi  <yao@codesourcery.com>
>             Hui Zhu  <hui@codesourcery.com>
>
>         PR breakpoints/15075
>
>         * breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
>         (update_dprintf_command_list): Don't append "continue" command
>         to the command of dprintf breakpoint.
>         (base_breakpoint_after_cond): New.
>         (base_breakpoint_ops): Add base_breakpoint_after_cond.
>         (dprintf_after_cond): New.
>         (initialize_breakpoint_ops): Set dprintf_after_cond.
>         * breakpoint.h (breakpoint_ops): Add after_cond.
>
> 2013-04-23  Yao Qi  <yao@codesourcery.com>
>
>         PR breakpoints/15075
>
>         * 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.
>         * gdb.base/pr15075.c: New.
>         * gdb.base/pr15075.exp: New.

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

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5283,13 +5283,7 @@ bpstat_stop_status (struct address_space
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
-	      if (b->silent)
-		bs->print = 0;
-	      bs->commands = b->commands;
-	      incref_counted_command_line (bs->commands);
-	      if (command_line_is_silent (bs->commands
-					  ? bs->commands->commands : NULL))
-		bs->print = 0;
+	      b->ops->after_cond (bs);
 	    }
 
 	}
@@ -8939,25 +8933,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);
@@ -12743,6 +12728,22 @@ base_breakpoint_explains_signal (struct
   return BPSTAT_SIGNAL_HIDE;
 }
 
+/* The default 'after_cond' method.  */
+
+static void
+base_breakpoint_after_cond (struct bpstats *bs)
+{
+  struct breakpoint *b = bs->breakpoint_at;
+
+  if (b->silent)
+    bs->print = 0;
+  bs->commands = b->commands;
+  incref_counted_command_line (bs->commands);
+  if (command_line_is_silent (bs->commands
+			      ? bs->commands->commands : NULL))
+  bs->print = 0;
+}
+
 struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -12762,7 +12763,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_cond,
 };
 
 /* Default breakpoint_ops methods.  */
@@ -13356,6 +13358,24 @@ dprintf_print_recreate (struct breakpoin
   print_recreate_thread (tp, fp);
 }
 
+/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
+
+static void
+dprintf_after_cond (struct bpstats *bs)
+{
+  bpstat tmp;
+  struct breakpoint *b = bs->breakpoint_at;
+
+  bs->stop = 0;
+  bs->print = 0;
+  bs->commands = b->commands;
+  tmp = bs->next;
+  bs->next = tmp;
+  incref_counted_command_line (bs->commands);
+  bpstat_do_actions_1 (&bs);
+  bs->next = tmp;
+}
+
 /* The breakpoint_ops structure to be used on static tracepoints with
    markers (`-m').  */
 
@@ -15852,6 +15872,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_cond = dprintf_after_cond;
 }
 
 /* Chain containing all defined "enable breakpoint" subcommands.  */
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -614,6 +614,9 @@ struct breakpoint_ops
      'catch signal' interact properly with 'handle'; see
      bpstat_explains_signal.  */
   enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
+
+  /* Do some setup after check condition is true.  */
+  void (*after_cond) (struct bpstats *bs);
 };
 
 /* Helper for breakpoint_ops->print_recreate implementations.  Prints

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

--- a/gdb/testsuite/gdb.base/dprintf.c
+++ b/gdb/testsuite/gdb.base/dprintf.c
@@ -39,6 +39,9 @@ main (int argc, char *argv[])
   foo (loc++);
   foo (loc++);
   foo (loc++);
+
+  sleep (3);
+
   return g;
 }
 
--- 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" \
@@ -136,3 +134,41 @@ if $target_can_dprintf {
 gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
     "Set dprintf style to an unrecognized type"
 
+
+# Test dprintf with non-stop mode.
+# The testfile uses "run".  The real bug happened only for ![is_remote target].
+if [target_info exists use_gdb_stub] {
+    unsupported "dprintf with non-stop mode"
+    return 0
+}
+gdb_test_no_output "delete 2 5"
+send_gdb "kill\n"
+gdb_expect 120 {
+    -re "Kill the program being debugged. .y or n. $" {
+	send_gdb "y\n"
+	verbose "\t\tKilling previous program being debugged"
+	exp_continue
+    }
+    -re "$gdb_prompt $" {
+	# OK.
+    }
+}
+gdb_test_no_output "set dprintf-style gdb" "Set dprintf style to gdb"
+gdb_test_no_output "set target-async on"
+gdb_test_no_output "set non-stop on"
+set gdbindex_warning_re "warning: Skipping \[^\r\n\]+ \\.gdb_index section \[^\r\n\]*\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\."
+
+gdb_test "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?"
+gdb_test "shell echo foo" "foo"
+set test "interrupt"
+gdb_test_multiple $test $test {
+    -re "interrupt\r\n$gdb_prompt " {
+	pass $test
+    }
+}
+set test "process stopped"
+gdb_test_multiple "" $test {
+    -re "\r\n\\\[process \[0-9\]+\\\] #1 stopped\\\.\r\n" {
+	pass $test
+    }
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr15075.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;
+   ++x; /* set dprintf here */
+   return x - 7;
+ }
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr15075.exp
@@ -0,0 +1,38 @@
+# 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 $expfile $executable $srcfile {debug}] {
+    untested "failed to prepare for trace tests"
+    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" ".*" "next 1"
+gdb_test "next" ".*" "next 2"
+# Test inferior doesn't exit.
+gdb_test "p x" ".* = 6"
--- 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

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

[Pedro Alves]

On 05/16/2013 08:28 AM, Hui Zhu wrote:
> Hi,
> 
> Because this patch also can handle bug 15434, I updated the test
> patches to add test dprintf with non-stop mode.

Looks like the test is racy.  :-/  I got:

At foo entry
arg=1235, g=2222
iAt foo entry
ntarg=1236, g=3013
errupt
(gdb) FAIL: gdb.base/dprintf.exp: interrupt

This will need to be sorted out.

I also saw:

(gdb) At foo entry
arg=1235, g=2222
At foo entry
arg=1236, g=3013
[Inferior 1 (process 30651) exited with code 0152]
FAIL: gdb.base/dprintf.exp: shell echo foo (timeout)
interrupt
../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) FAIL: gdb.base/dprintf.exp: interrupt (GDB internal error)
Resyncing due to internal error.
n
../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n
(gdb) FAIL: gdb.base/dprintf.exp: process stopped (timeout)
testcase ../../../src/gdb/testsuite/gdb.base/dprintf.exp completed in 20 seconds

:-(  Though that's likely unrelated.


> 2013-05-16  Yao Qi  <yao@codesourcery.com>
> 	    Hui Zhu  <hui@codesourcery.com>
> 
> 	PR breakpoints/15075
> 	PR breakpoints/15434
> 	* breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
> 	(update_dprintf_command_list): Don't append "continue" command
> 	to the command of dprintf breakpoint.

"to the command list."

> 	(base_breakpoint_after_cond): New.
> 	(base_breakpoint_ops): Add base_breakpoint_after_cond.
> 	(dprintf_after_cond): New.

Say "New function."

> 	(initialize_breakpoint_ops): Set dprintf_after_cond.
> 	* breakpoint.h (breakpoint_ops): Add after_cond.
> 
> 2013-05-16  Yao Qi  <yao@codesourcery.com>
> 	    Hui Zhu  <hui@codesourcery.com>
> 
> 	PR breakpoints/15075
> 	PR breakpoints/15434
> 	* gdb.base/dprintf.exp: Don't check "continue" in the output
> 	of "info breakpoints".
> 	Add test dprintf with non-stop mode.

 	Add test for dprintf in non-stop mode.

> 	* gdb.base/dprintf.c: Add a sleep for test dprintf with non-stop

for testing.

> 	mode.
> 	* gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
> 	Don't check "continue" in script field.
> 	* gdb.base/pr15075.c: New.
> 	* gdb.base/pr15075.exp: New.
> 



> 
> 
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5283,13 +5283,7 @@ bpstat_stop_status (struct address_space
>  		    b->enable_state = bp_disabled;
>  		  removed_any = 1;
>  		}
> -	      if (b->silent)
> -		bs->print = 0;
> -	      bs->commands = b->commands;
> -	      incref_counted_command_line (bs->commands);
> -	      if (command_line_is_silent (bs->commands
> -					  ? bs->commands->commands : NULL))
> -		bs->print = 0;
> +	      b->ops->after_cond (bs);
>  	    }
>  
>  	}
> @@ -8939,25 +8933,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);
> @@ -12743,6 +12728,22 @@ base_breakpoint_explains_signal (struct
>    return BPSTAT_SIGNAL_HIDE;
>  }
>  



> +/* The default 'after_cond' method.  */

You used '' here.

> +
> +static void
> +base_breakpoint_after_cond (struct bpstats *bs)
> +{
> +  struct breakpoint *b = bs->breakpoint_at;
> +
> +  if (b->silent)
> +    bs->print = 0;
> +  bs->commands = b->commands;
> +  incref_counted_command_line (bs->commands);
> +  if (command_line_is_silent (bs->commands
> +			      ? bs->commands->commands : NULL))
> +  bs->print = 0;

Indentation is wrong here.

I think we can keep all these where it was.

> +}
> +


> +/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */

You used "" here.  :-)  Spurious double space before breakpoint_ops.

> +
> +static void
> +dprintf_after_cond (struct bpstats *bs)
> +{
> +  bpstat tmp;
> +  struct breakpoint *b = bs->breakpoint_at;
> +
> +  bs->stop = 0;
> +  bs->print = 0;

If we make dprintf's silent (b->silent = 0), then bs->print will
end up 0 for them too, given the:

  if (b->silent)
    bs->print = 0;

bit.

Not sure even that is necessary, given bs->stop==0 and:

      /* 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;

> +  bs->commands = b->commands;
> +  tmp = bs->next;
> +  bs->next = tmp;

A = B;
B = A;

?

Was this supposed to be:

A = B;
B = NULL;

?

> +  incref_counted_command_line (bs->commands);

If copying all this code here really is necessary, then keep the
incref close to the commands copy, like in the original code.

> +  bpstat_do_actions_1 (&bs);
> +  bs->next = tmp;
> +}

Could you add some comments explaining what this is
doing, and why?

> +
>  /* The breakpoint_ops structure to be used on static tracepoints with
>     markers (`-m').  */
>  
> @@ -15852,6 +15872,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_cond = dprintf_after_cond;
>  }
>  
>  /* Chain containing all defined "enable breakpoint" subcommands.  */
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -614,6 +614,9 @@ struct breakpoint_ops
>       'catch signal' interact properly with 'handle'; see
>       bpstat_explains_signal.  */
>    enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
> +
> +  /* Do some setup after check condition is true.  */

   /* Called after evaluating the breakpoint's condition,
      and only if it evaluated true.  */

> +  void (*after_cond) (struct bpstats *bs);

I'd rather spell out "cond".  I suggest:

  void (*after_condition_true) (...);

To be a bit more self descriptive.

>  };
>  
>  /* Helper for breakpoint_ops->print_recreate implementations.  Prints
> 
> 
> dprintf-continue-test.txt
> 
> 
> --- a/gdb/testsuite/gdb.base/dprintf.c
> +++ b/gdb/testsuite/gdb.base/dprintf.c
> @@ -39,6 +39,9 @@ main (int argc, char *argv[])
>    foo (loc++);
>    foo (loc++);
>    foo (loc++);
> +
> +  sleep (3);
> +
>    return g;
>  }
>  
> --- 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" \
> @@ -136,3 +134,41 @@ if $target_can_dprintf {
>  gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
>      "Set dprintf style to an unrecognized type"
>  
> +
> +# Test dprintf with non-stop mode.
> +# The testfile uses "run".  The real bug happened only for ![is_remote target].

It doesn't need to use "run", AFAICS.  You can run to
main as usual, and then use "c&" to get the same effect,
and then the test should pass with GDBserver too.

There are targets that don't support non-stop,
and, the call/gdb dprintf styles works on all targets.
The test should be gracefully skipped on those targets.
It might be better to split this to a separate test file.

> +if [target_info exists use_gdb_stub] {
> +    unsupported "dprintf with non-stop mode"
> +    return 0
> +}
> +gdb_test_no_output "delete 2 5"
> +send_gdb "kill\n"
> +gdb_expect 120 {
> +    -re "Kill the program being debugged. .y or n. $" {
> +	send_gdb "y\n"
> +	verbose "\t\tKilling previous program being debugged"
> +	exp_continue
> +    }
> +    -re "$gdb_prompt $" {
> +	# OK.
> +    }
> +}

Why do this manually?

> +gdb_test_no_output "set dprintf-style gdb" "Set dprintf style to gdb"
> +gdb_test_no_output "set target-async on"
> +gdb_test_no_output "set non-stop on"
> +set gdbindex_warning_re "warning: Skipping \[^\r\n\]+ \\.gdb_index section \[^\r\n\]*\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\."

I don't under why do we need to explicitly check for this warning?

> +gdb_test "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?"
> +gdb_test "shell echo foo" "foo"

What's this shell echo for?

> +set test "interrupt"
> +gdb_test_multiple $test $test {
> +    -re "interrupt\r\n$gdb_prompt " {
> +	pass $test
> +    }
> +}
> +set test "process stopped"
> +gdb_test_multiple "" $test {
> +    -re "\r\n\\\[process \[0-9\]+\\\] #1 stopped\\\.\r\n" {
> +	pass $test
> +    }
> +}
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr15075.c

We prefer that test files are named for something that suggests what they
contain, rather than bug numbers.  Please call it something
like dprintf-next.exp.

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

Missing space.

> + {
> +   int x = 5;

Wrong indentation.

> +
> +   ++x;
> +   ++x; /* set dprintf here */
> +   return x - 7;
> + }
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/pr15075.exp
> @@ -0,0 +1,38 @@
> +# 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 $expfile $executable $srcfile {debug}] {
> +    untested "failed to prepare for trace tests"

This is not a trace test.  Actually, prepare_for_testing
already calls untested with whatever we pass it as first argument.
I think it'd be the first such call in the testsuite, but I
think this would be better:

if [prepare_for_testing "failed to prepare for trace tests" \
    $executable $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" ".*" "next 1"
> +gdb_test "next" ".*" "next 2"

Please use more string regexes for these, that confirm the next stopped
at the right lines.

> +# Test inferior doesn't exit.

Check that the inferior didn't exit.

> +gdb_test "p x" ".* = 6"

With the stricter regexes, I don't think this test would be
necessary though.

-- 
Pedro Alves

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

[Hui Zhu]

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

Hi Pedro,

Thanks for your review.

On Sat, May 18, 2013 at 5:01 AM, Pedro Alves <palves@redhat.com> wrote:
> On 05/16/2013 08:28 AM, Hui Zhu wrote:
>> Hi,
>>
>> Because this patch also can handle bug 15434, I updated the test
>> patches to add test dprintf with non-stop mode.
>
> Looks like the test is racy.  :-/  I got:
>
> At foo entry
> arg=1235, g=2222
> iAt foo entry
> ntarg=1236, g=3013
> errupt
> (gdb) FAIL: gdb.base/dprintf.exp: interrupt
>
> This will need to be sorted out.
>
> I also saw:
>
> (gdb) At foo entry
> arg=1235, g=2222
> At foo entry
> arg=1236, g=3013
> [Inferior 1 (process 30651) exited with code 0152]
> FAIL: gdb.base/dprintf.exp: shell echo foo (timeout)
> interrupt
> ../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) FAIL: gdb.base/dprintf.exp: interrupt (GDB internal error)
> Resyncing due to internal error.
> n
> ../../src/gdb/thread.c:694: internal-error: set_stop_requested: Assertion `tp' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> (gdb) FAIL: gdb.base/dprintf.exp: process stopped (timeout)
> testcase ../../../src/gdb/testsuite/gdb.base/dprintf.exp completed in 20 seconds
>
> :-(  Though that's likely unrelated.

I updated this part and put it to dpintf-non-stop.exp.
I tried it on fedora and looks it works OK most of times.  I only got
trouble is when run this test with remote, [runto main] with
"non-stop" is open will fail sometime.

>
>
>> 2013-05-16  Yao Qi  <yao@codesourcery.com>
>>           Hui Zhu  <hui@codesourcery.com>
>>
>>       PR breakpoints/15075
>>       PR breakpoints/15434
>>       * breakpoint.c (bpstat_stop_status): Call b->ops->after_cond.
>>       (update_dprintf_command_list): Don't append "continue" command
>>       to the command of dprintf breakpoint.
>
> "to the command list."

Fixed.

>
>>       (base_breakpoint_after_cond): New.
>>       (base_breakpoint_ops): Add base_breakpoint_after_cond.
>>       (dprintf_after_cond): New.
>
> Say "New function."

Fixed.

>
>>       (initialize_breakpoint_ops): Set dprintf_after_cond.
>>       * breakpoint.h (breakpoint_ops): Add after_cond.
>>
>> 2013-05-16  Yao Qi  <yao@codesourcery.com>
>>           Hui Zhu  <hui@codesourcery.com>
>>
>>       PR breakpoints/15075
>>       PR breakpoints/15434
>>       * gdb.base/dprintf.exp: Don't check "continue" in the output
>>       of "info breakpoints".
>>       Add test dprintf with non-stop mode.
>
>         Add test for dprintf in non-stop mode.
>
>>       * gdb.base/dprintf.c: Add a sleep for test dprintf with non-stop
>
> for testing.

Fixed.

>
>>       mode.
>>       * gdb.mi/mi-breakpoint-changed.exp (test_insert_delete_modify):
>>       Don't check "continue" in script field.
>>       * gdb.base/pr15075.c: New.
>>       * gdb.base/pr15075.exp: New.
>>
>
>
>
>>
>>
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5283,13 +5283,7 @@ bpstat_stop_status (struct address_space
>>                   b->enable_state = bp_disabled;
>>                 removed_any = 1;
>>               }
>> -           if (b->silent)
>> -             bs->print = 0;
>> -           bs->commands = b->commands;
>> -           incref_counted_command_line (bs->commands);
>> -           if (command_line_is_silent (bs->commands
>> -                                       ? bs->commands->commands : NULL))
>> -             bs->print = 0;
>> +           b->ops->after_cond (bs);
>>           }
>>
>>       }
>> @@ -8939,25 +8933,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);
>> @@ -12743,6 +12728,22 @@ base_breakpoint_explains_signal (struct
>>    return BPSTAT_SIGNAL_HIDE;
>>  }
>>
>
>
>
>> +/* The default 'after_cond' method.  */
>
> You used '' here.

Fixed.

>
>> +
>> +static void
>> +base_breakpoint_after_cond (struct bpstats *bs)
>> +{
>> +  struct breakpoint *b = bs->breakpoint_at;
>> +
>> +  if (b->silent)
>> +    bs->print = 0;
>> +  bs->commands = b->commands;
>> +  incref_counted_command_line (bs->commands);
>> +  if (command_line_is_silent (bs->commands
>> +                           ? bs->commands->commands : NULL))
>> +  bs->print = 0;
>
> Indentation is wrong here.
>
> I think we can keep all these where it was.

Fixed.

>
>> +}
>> +
>
>
>> +/* Implement the "after_cond"  breakpoint_ops method for dprintf.  */
>
> You used "" here.  :-)  Spurious double space before breakpoint_ops.

Fixed.  :)

>
>> +
>> +static void
>> +dprintf_after_cond (struct bpstats *bs)
>> +{
>> +  bpstat tmp;
>> +  struct breakpoint *b = bs->breakpoint_at;
>> +
>> +  bs->stop = 0;
>> +  bs->print = 0;
>
> If we make dprintf's silent (b->silent = 0), then bs->print will
> end up 0 for them too, given the:
>
>   if (b->silent)
>     bs->print = 0;
>
> bit.
>
> Not sure even that is necessary, given bs->stop==0 and:
>
>       /* 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;

I am not sure about this part.
If my understanding about Tom's review in before is right, the action
of dprintf command should be handled when GDB check the condition.
And after that, dprintf not need be handled.

So dprintf_after_cond just clear the bs->stop and bs->print because
dprintf not need be handled after this function.
And call command of this dprintf because the its condition is true.

I added some comments for that.

>
>> +  bs->commands = b->commands;
>> +  tmp = bs->next;
>> +  bs->next = tmp;
>
> A = B;
> B = A;
>
> ?
>
> Was this supposed to be:
>
> A = B;
> B = NULL;
>
> ?

I think this "bs->next = tmp;" is duplicate line after
bpstat_do_actions_1.  Fixed.

>
>> +  incref_counted_command_line (bs->commands);
>
> If copying all this code here really is necessary, then keep the
> incref close to the commands copy, like in the original code.

Fixed.

>
>> +  bpstat_do_actions_1 (&bs);
>> +  bs->next = tmp;
>> +}
>
> Could you add some comments explaining what this is
> doing, and why?

I update this function to following part:
/* Implement the "after_cond" breakpoint_ops method for dprintf.  */

static void
dprintf_after_cond (struct bpstats *bs)
{
  struct cleanup *old_chain;
  struct breakpoint *b = bs->breakpoint_at;

  bs->commands = b->commands;
  incref_counted_command_line (bs->commands);

  /* Because function bpstat_do_actions_1 will execute all the command
     list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
     NULL.  */
  old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
  bs->next = NULL;
  bpstat_do_actions_1 (&bs);
  do_cleanups (old_chain);

  /* This dprintf need not be handled after this function
     because its command list is executed by bpstat_do_actions_1.
     Clear STOP and PRINT for that.  */
  bs->stop = 0;
  bs->print = 0;
}

>
>> +
>>  /* The breakpoint_ops structure to be used on static tracepoints with
>>     markers (`-m').  */
>>
>> @@ -15852,6 +15872,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_cond = dprintf_after_cond;
>>  }
>>
>>  /* Chain containing all defined "enable breakpoint" subcommands.  */
>> --- a/gdb/breakpoint.h
>> +++ b/gdb/breakpoint.h
>> @@ -614,6 +614,9 @@ struct breakpoint_ops
>>       'catch signal' interact properly with 'handle'; see
>>       bpstat_explains_signal.  */
>>    enum bpstat_signal_value (*explains_signal) (struct breakpoint *);
>> +
>> +  /* Do some setup after check condition is true.  */
>
>    /* Called after evaluating the breakpoint's condition,
>       and only if it evaluated true.  */

Fixed.

>
>> +  void (*after_cond) (struct bpstats *bs);
>
> I'd rather spell out "cond".  I suggest:
>
>   void (*after_condition_true) (...);
>
> To be a bit more self descriptive.
>

Fixed.

>>  };
>>
>>  /* Helper for breakpoint_ops->print_recreate implementations.  Prints
>>
>>
>> dprintf-continue-test.txt
>>
>>
>> --- a/gdb/testsuite/gdb.base/dprintf.c
>> +++ b/gdb/testsuite/gdb.base/dprintf.c
>> @@ -39,6 +39,9 @@ main (int argc, char *argv[])
>>    foo (loc++);
>>    foo (loc++);
>>    foo (loc++);
>> +
>> +  sleep (3);
>> +
>>    return g;
>>  }
>>
>> --- 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" \
>> @@ -136,3 +134,41 @@ if $target_can_dprintf {
>>  gdb_test "set dprintf-style foobar" "Undefined item: \"foobar\"." \
>>      "Set dprintf style to an unrecognized type"
>>
>> +
>> +# Test dprintf with non-stop mode.
>> +# The testfile uses "run".  The real bug happened only for ![is_remote target].
>
> It doesn't need to use "run", AFAICS.  You can run to
> main as usual, and then use "c&" to get the same effect,
> and then the test should pass with GDBserver too.
>
> There are targets that don't support non-stop,
> and, the call/gdb dprintf styles works on all targets.
> The test should be gracefully skipped on those targets.
> It might be better to split this to a separate test file.

Add dprintf-non-stop.exp and dprintf-non-stop.c for it.

>
>> +if [target_info exists use_gdb_stub] {
>> +    unsupported "dprintf with non-stop mode"
>> +    return 0
>> +}
>> +gdb_test_no_output "delete 2 5"
>> +send_gdb "kill\n"
>> +gdb_expect 120 {
>> +    -re "Kill the program being debugged. .y or n. $" {
>> +     send_gdb "y\n"
>> +     verbose "\t\tKilling previous program being debugged"
>> +     exp_continue
>> +    }
>> +    -re "$gdb_prompt $" {
>> +     # OK.
>> +    }
>> +}
>
> Why do this manually?

This part was removed.

>
>> +gdb_test_no_output "set dprintf-style gdb" "Set dprintf style to gdb"
>> +gdb_test_no_output "set target-async on"
>> +gdb_test_no_output "set non-stop on"
>> +set gdbindex_warning_re "warning: Skipping \[^\r\n\]+ \\.gdb_index section \[^\r\n\]*\r\nDo \"set use-deprecated-index-sections on\" before the file is read\r\nto use the section anyway\\."
>
> I don't under why do we need to explicitly check for this warning?

Removed too.

>
>> +gdb_test "run &" "Starting program: \[^\r\n\]*(\r\n$gdbindex_warning_re)?"
>> +gdb_test "shell echo foo" "foo"
>
> What's this shell echo for?

This part copy from other test of non-stop.  It let test wait some
time then dprintf execution can complete.  Now I changed it to exec
sleep 1.

>
>> +set test "interrupt"
>> +gdb_test_multiple $test $test {
>> +    -re "interrupt\r\n$gdb_prompt " {
>> +     pass $test
>> +    }
>> +}
>> +set test "process stopped"
>> +gdb_test_multiple "" $test {
>> +    -re "\r\n\\\[process \[0-9\]+\\\] #1 stopped\\\.\r\n" {
>> +     pass $test
>> +    }
>> +}
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/pr15075.c
>
> We prefer that test files are named for something that suggests what they
> contain, rather than bug numbers.  Please call it something
> like dprintf-next.exp.

Fixed.

>
>> @@ -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)
>
> Missing space.

Fixed.

>
>> + {
>> +   int x = 5;
>
> Wrong indentation.

Fixed.

>
>> +
>> +   ++x;
>> +   ++x; /* set dprintf here */
>> +   return x - 7;
>> + }
>> --- /dev/null
>> +++ b/gdb/testsuite/gdb.base/pr15075.exp
>> @@ -0,0 +1,38 @@
>> +# 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 $expfile $executable $srcfile {debug}] {
>> +    untested "failed to prepare for trace tests"
>
> This is not a trace test.  Actually, prepare_for_testing
> already calls untested with whatever we pass it as first argument.
> I think it'd be the first such call in the testsuite, but I
> think this would be better:
>
> if [prepare_for_testing "failed to prepare for trace tests" \
>     $executable $srcfile {debug}] {
>    return -1
> }

After I use this part of code, I got:
ERROR: tcl error sourcing ../../../src/gdb/testsuite/gdb.base/dprintf-next.exp.
ERROR: wrong # args: should be "prepare_for_testing testname
executable ?sources? ?options?"
    while executing
"prepare_for_testing "failed to prepare for dprintf next tests""
    invoked from within
"if [prepare_for_testing "failed to prepare for dprintf next tests"
    $executable $srcfile {debug}] {
   return -1
}"


So I change this part to:
if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {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" ".*" "next 1"
>> +gdb_test "next" ".*" "next 2"
>
> Please use more string regexes for these, that confirm the next stopped
> at the right lines.

Fixed.

>
>> +# Test inferior doesn't exit.
>
> Check that the inferior didn't exit.
>
>> +gdb_test "p x" ".* = 6"
>
> With the stricter regexes, I don't think this test would be
> necessary though.

Removed.

Post new patches according to your comments.

Best,
Hui

2013-05-22  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.com>

	PR breakpoints/15075
	PR breakpoints/15434
	* utils.c (restore_ptr_closure): New struct.
	(restore_ptr, make_cleanup_restore_ptr): New functions.
	* utils.h (make_cleanup_restore_ptr): New extern.
	* 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-05-22  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.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.

[-- Attachment #2: clean_ptr.txt --]
[-- Type: text/plain, Size: 1217 bytes --]

--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -385,6 +385,35 @@ make_cleanup_restore_uinteger (unsigned
   return make_cleanup_restore_integer ((int *) variable);
 }
 
+struct restore_ptr_closure
+{
+  void **variable;
+  void *value;
+};
+
+static void
+restore_ptr (void *p)
+{
+  struct restore_ptr_closure *closure = p;
+
+  *(closure->variable) = closure->value;
+}
+
+/* Remember the current value of *VARIABLE and make it restored when
+   the cleanup is run.  */
+
+struct cleanup *
+make_cleanup_restore_ptr (void **variable)
+{
+  struct restore_ptr_closure *c =
+    xmalloc (sizeof (struct restore_ptr_closure));
+
+  c->variable = variable;
+  c->value = *variable;
+
+  return make_cleanup_dtor (restore_ptr, (void *) c, xfree);
+}
+
 /* Helper for make_cleanup_unpush_target.  */
 
 static void
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -97,6 +97,7 @@ extern struct cleanup *make_cleanup_obst
 
 extern struct cleanup *make_cleanup_restore_integer (int *variable);
 extern struct cleanup *make_cleanup_restore_uinteger (unsigned int *variable);
+extern struct cleanup *make_cleanup_restore_ptr (void **variable);
 
 struct target_ops;
 extern struct cleanup *make_cleanup_unpush_target (struct target_ops *ops);

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

--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5283,13 +5283,7 @@ bpstat_stop_status (struct address_space
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
-	      if (b->silent)
-		bs->print = 0;
-	      bs->commands = b->commands;
-	      incref_counted_command_line (bs->commands);
-	      if (command_line_is_silent (bs->commands
-					  ? bs->commands->commands : NULL))
-		bs->print = 0;
+	      b->ops->after_condition_true (bs);
 	    }
 
 	}
@@ -8939,25 +8933,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);
@@ -12743,6 +12728,22 @@ 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)
+{
+  struct breakpoint *b = bs->breakpoint_at;
+
+  if (b->silent)
+    bs->print = 0;
+  bs->commands = b->commands;
+  incref_counted_command_line (bs->commands);
+  if (command_line_is_silent (bs->commands
+			      ? bs->commands->commands : NULL))
+    bs->print = 0;
+}
+
 struct breakpoint_ops base_breakpoint_ops =
 {
   base_breakpoint_dtor,
@@ -12762,7 +12763,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.  */
@@ -13356,6 +13358,33 @@ dprintf_print_recreate (struct breakpoin
   print_recreate_thread (tp, fp);
 }
 
+/* Implement the "after_condition_true" breakpoint_ops method for
+   dprintf.  */
+
+static void
+dprintf_after_condition_true (struct bpstats *bs)
+{
+  struct cleanup *old_chain;
+  struct breakpoint *b = bs->breakpoint_at;
+
+  bs->commands = b->commands;
+  incref_counted_command_line (bs->commands);
+
+  /* Because function bpstat_do_actions_1 will execute all the command
+     list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
+     NULL.  */
+  old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
+  bs->next = NULL;
+  bpstat_do_actions_1 (&bs);
+  do_cleanups (old_chain);
+
+  /* This dprintf need not be handled after this function
+     because its command list is executed by bpstat_do_actions_1.
+     Clear STOP and PRINT for that.  */
+  bs->stop = 0;
+  bs->print = 0;
+}
+
 /* The breakpoint_ops structure to be used on static tracepoints with
    markers (`-m').  */
 
@@ -15852,6 +15881,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 #4: dprintf-continue-test.txt --]
[-- Type: text/plain, Size: 5784 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;
+  ++x; /* set dprintf here */
+  return x - 7;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-next.exp
@@ -0,0 +1,35 @@
+# 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 dprintf-next.exp "dprintf-next" {} {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" "23.*\\+\\+x.*" "next 1"
+gdb_test "next" "24.*\\+\\+x.*" "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>
+
+   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 ();
+  while (1);
+  return 0;
+}
\ No newline at end of file
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
@@ -0,0 +1,45 @@
+#   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 { [prepare_for_testing dprintf-non-stop.exp "dprintf-non-stop" {} {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" \
@@ -135,4 +133,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

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

[Pedro Alves]

On 05/22/2013 11:21 AM, Hui Zhu wrote:

> I tried it on fedora and looks it works OK most of times.  I only got
> trouble is when run this test with remote, [runto main] with
> "non-stop" is open will fail sometime.

Sorry, but "most of the times" is not acceptable.  Can you please
figure out why that is so?  This should never fail.

>> > Not sure even that is necessary, given bs->stop==0 and:
>> >
>> >       /* 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;
> I am not sure about this part.
> If my understanding about Tom's review in before is right, the action
> of dprintf command should be handled when GDB check the condition.
> And after that, dprintf not need be handled.

Yes, but I don't see how that counters my suggestion.

>> >
>>> >> +  bpstat_do_actions_1 (&bs);
>>> >> +  bs->next = tmp;
>>> >> +}
>> >
>> > Could you add some comments explaining what this is
>> > doing, and why?
> I update this function to following part:
> /* Implement the "after_cond" breakpoint_ops method for dprintf.  */
> 
> static void
> dprintf_after_cond (struct bpstats *bs)


> {
>   struct cleanup *old_chain;
>   struct breakpoint *b = bs->breakpoint_at;
> 
>   bs->commands = b->commands;
>   incref_counted_command_line (bs->commands);

I still think there's no point in moving this to the
after_condition_true hook, if both existing implementations
end up doing it themselves.  See below.

> 
>   /* Because function bpstat_do_actions_1 will execute all the command
>      list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
>      NULL.  */
>   old_chain = make_cleanup_restore_ptr ((void **)&bs->next);

That cast is invalid actually.  Older gcc's will complain about
the aliasing violation.  I think we can get away without this, by
passing a separate list with only one entry to bpstat_do_actions_1.

>   bs->next = NULL;
>   bpstat_do_actions_1 (&bs);
>   do_cleanups (old_chain);
> 
>   /* This dprintf need not be handled after this function
>      because its command list is executed by bpstat_do_actions_1.
>      Clear STOP and PRINT for that.  */
>   bs->stop = 0;
>   bs->print = 0;

Thanks.  I was more looking for general design comments on why we
run the command list here.  This also should explain why clear stop
here, instead of on the more natural check_status.

So all in all, if it works for you (seems to work fine for me), I'd
rather do as this patch on top of yours does:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 gdb/breakpoint.c |   62 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 37 insertions(+), 25 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 7ef5703..181aecc 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5292,6 +5292,15 @@ bpstat_stop_status (struct address_space *aspace,
 		    b->enable_state = bp_disabled;
 		  removed_any = 1;
 		}
+
+	      if (b->silent)
+		bs->print = 0;
+	      bs->commands = b->commands;
+	      incref_counted_command_line (bs->commands);
+	      if (command_line_is_silent (bs->commands
+					  ? bs->commands->commands : NULL))
+		bs->print = 0;
+
 	      b->ops->after_condition_true (bs);
 	    }
 
@@ -12742,15 +12751,7 @@ base_breakpoint_explains_signal (struct breakpoint *b)
 static void
 base_breakpoint_after_condition_true (struct bpstats *bs)
 {
-  struct breakpoint *b = bs->breakpoint_at;
-
-  if (b->silent)
-    bs->print = 0;
-  bs->commands = b->commands;
-  incref_counted_command_line (bs->commands);
-  if (command_line_is_silent (bs->commands
-			      ? bs->commands->commands : NULL))
-    bs->print = 0;
+  /* Nothing to do.   */
 }
 
 struct breakpoint_ops base_breakpoint_ops =
@@ -13368,30 +13369,41 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp)
 }
 
 /* Implement the "after_condition_true" breakpoint_ops method for
-   dprintf.  */
+   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 breakpoint *b = bs->breakpoint_at;
+  struct bpstats tmp_bs = { NULL };
+  struct bpstats *tmp_bs_p = &tmp_bs;
 
-  bs->commands = b->commands;
-  incref_counted_command_line (bs->commands);
+  /* 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;
 
-  /* Because function bpstat_do_actions_1 will execute all the command
-     list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
-     NULL.  */
-  old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
-  bs->next = NULL;
-  bpstat_do_actions_1 (&bs);
-  do_cleanups (old_chain);
+  /* 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);
 
-  /* This dprintf need not be handled after this function
-     because its command list is executed by bpstat_do_actions_1.
-     Clear STOP and PRINT for that.  */
-  bs->stop = 0;
-  bs->print = 0;
+  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
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

>>> >> +if [prepare_for_testing $expfile $executable $srcfile {debug}] {
>>> >> +    untested "failed to prepare for trace tests"
>> >
>> > This is not a trace test.  Actually, prepare_for_testing
>> > already calls untested with whatever we pass it as first argument.
>> > I think it'd be the first such call in the testsuite, but I
>> > think this would be better:
>> >
>> > if [prepare_for_testing "failed to prepare for trace tests" \
>> >     $executable $srcfile {debug}] {
>> >    return -1
>> > }
> After I use this part of code, I got:
> ERROR: tcl error sourcing ../../../src/gdb/testsuite/gdb.base/dprintf-next.exp.
> ERROR: wrong # args: should be "prepare_for_testing testname
> executable ?sources? ?options?"
>     while executing
> "prepare_for_testing "failed to prepare for dprintf next tests""
>     invoked from within
> "if [prepare_for_testing "failed to prepare for dprintf next tests"
>     $executable $srcfile {debug}] {
>    return -1
> }"

Hmm?  Oh, it looks like you forgot the '\' at the end of the line.
Please try again.

> So I change this part to:
> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {

'dprintf-next.exp' really isn't magical.  It's just a string.  :-)

>     return -1
> }

>>> >> +gdb_test "next" ".*" "next 1"
>>> >> +gdb_test "next" ".*" "next 2"
>> >
>> > Please use more string regexes for these, that confirm the next stopped
>> > at the right lines.
> Fixed.

Sorry, I should have been clearer.  Don't do that by checking
line numbers though, as those change when more functions are
added to the test.  Just make each line look different instead.

> +
> +gdb_test "dprintf $dp_location, \"%d\\n\", x" \
> +    "Dprintf .*"

Please add an explicit 3rd parameter to gdb_test, so that
the line number doesn't appear in gdb.sum.

> +
> +gdb_test "next" "23.*\\+\\+x.*" "next 1"
> +gdb_test "next" "24.*\\+\\+x.*" "next 2"

Do something like this:

gdb_test "next" "\\+\\+x;" "next 1"
gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"

You could add a comment to the first stepped line to
make that clearer:

gdb_test "next" "\\+\\+x; /* step here.*" "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>
> +
> +   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 ();
> +  while (1);

If something goes wrong, like GDB crashing, this could leave the test
running forever, pegging the CPU.  Just make it sleep for some 
time instead.

> +  return 0;

> +}
> \ No newline at end of file

Please watch out for these, and add a newline.

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

This leaves the prompt in the expect buffer.  I think
this is likely to confuse the following test that runs.

-- 
Pedro Alves

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

[Hui Zhu]

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

Hi Pedro and Tom,

Thanks for your review.

On Wed, May 22, 2013 at 8:46 PM, Pedro Alves <palves@redhat.com> wrote:
> On 05/22/2013 11:21 AM, Hui Zhu wrote:
>
>> I tried it on fedora and looks it works OK most of times.  I only got
>> trouble is when run this test with remote, [runto main] with
>> "non-stop" is open will fail sometime.
>
> Sorry, but "most of the times" is not acceptable.  Can you please
> figure out why that is so?  This should never fail.

I found a issue about remote test and non-stop.  Because it doesn't
have a lot of relation with dprintf, I will post a separate patch for
that later.

>
>>> > Not sure even that is necessary, given bs->stop==0 and:
>>> >
>>> >       /* 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;
>> I am not sure about this part.
>> If my understanding about Tom's review in before is right, the action
>> of dprintf command should be handled when GDB check the condition.
>> And after that, dprintf not need be handled.
>
> Yes, but I don't see how that counters my suggestion.

Added dprintf_create_breakpoints_sal. When dprintf is created, auto
set silent to 0.

>
>>> >
>>>> >> +  bpstat_do_actions_1 (&bs);
>>>> >> +  bs->next = tmp;
>>>> >> +}
>>> >
>>> > Could you add some comments explaining what this is
>>> > doing, and why?
>> I update this function to following part:
>> /* Implement the "after_cond" breakpoint_ops method for dprintf.  */
>>
>> static void
>> dprintf_after_cond (struct bpstats *bs)
>
>
>> {
>>   struct cleanup *old_chain;
>>   struct breakpoint *b = bs->breakpoint_at;
>>
>>   bs->commands = b->commands;
>>   incref_counted_command_line (bs->commands);
>
> I still think there's no point in moving this to the
> after_condition_true hook, if both existing implementations
> end up doing it themselves.  See below.
>
>>
>>   /* Because function bpstat_do_actions_1 will execute all the command
>>      list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
>>      NULL.  */
>>   old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
>
> That cast is invalid actually.  Older gcc's will complain about
> the aliasing violation.  I think we can get away without this, by
> passing a separate list with only one entry to bpstat_do_actions_1.
>
>>   bs->next = NULL;
>>   bpstat_do_actions_1 (&bs);
>>   do_cleanups (old_chain);
>>
>>   /* This dprintf need not be handled after this function
>>      because its command list is executed by bpstat_do_actions_1.
>>      Clear STOP and PRINT for that.  */
>>   bs->stop = 0;
>>   bs->print = 0;
>
> Thanks.  I was more looking for general design comments on why we
> run the command list here.  This also should explain why clear stop
> here, instead of on the more natural check_status.
>
> So all in all, if it works for you (seems to work fine for me), I'd
> rather do as this patch on top of yours does:
>

Thanks for this patch.  I merged it to dprintf-continue patch.

> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  gdb/breakpoint.c |   62 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 37 insertions(+), 25 deletions(-)
>
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 7ef5703..181aecc 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5292,6 +5292,15 @@ bpstat_stop_status (struct address_space *aspace,
>                     b->enable_state = bp_disabled;
>                   removed_any = 1;
>                 }
> +
> +             if (b->silent)
> +               bs->print = 0;
> +             bs->commands = b->commands;
> +             incref_counted_command_line (bs->commands);
> +             if (command_line_is_silent (bs->commands
> +                                         ? bs->commands->commands : NULL))
> +               bs->print = 0;
> +
>               b->ops->after_condition_true (bs);
>             }
>
> @@ -12742,15 +12751,7 @@ base_breakpoint_explains_signal (struct breakpoint *b)
>  static void
>  base_breakpoint_after_condition_true (struct bpstats *bs)
>  {
> -  struct breakpoint *b = bs->breakpoint_at;
> -
> -  if (b->silent)
> -    bs->print = 0;
> -  bs->commands = b->commands;
> -  incref_counted_command_line (bs->commands);
> -  if (command_line_is_silent (bs->commands
> -                             ? bs->commands->commands : NULL))
> -    bs->print = 0;
> +  /* Nothing to do.   */
>  }
>
>  struct breakpoint_ops base_breakpoint_ops =
> @@ -13368,30 +13369,41 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp)
>  }
>
>  /* Implement the "after_condition_true" breakpoint_ops method for
> -   dprintf.  */
> +   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 breakpoint *b = bs->breakpoint_at;
> +  struct bpstats tmp_bs = { NULL };
> +  struct bpstats *tmp_bs_p = &tmp_bs;
>
> -  bs->commands = b->commands;
> -  incref_counted_command_line (bs->commands);
> +  /* 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;
>
> -  /* Because function bpstat_do_actions_1 will execute all the command
> -     list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
> -     NULL.  */
> -  old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
> -  bs->next = NULL;
> -  bpstat_do_actions_1 (&bs);
> -  do_cleanups (old_chain);
> +  /* 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);
>
> -  /* This dprintf need not be handled after this function
> -     because its command list is executed by bpstat_do_actions_1.
> -     Clear STOP and PRINT for that.  */
> -  bs->stop = 0;
> -  bs->print = 0;
> +  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
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>>>> >> +if [prepare_for_testing $expfile $executable $srcfile {debug}] {
>>>> >> +    untested "failed to prepare for trace tests"
>>> >
>>> > This is not a trace test.  Actually, prepare_for_testing
>>> > already calls untested with whatever we pass it as first argument.
>>> > I think it'd be the first such call in the testsuite, but I
>>> > think this would be better:
>>> >
>>> > if [prepare_for_testing "failed to prepare for trace tests" \
>>> >     $executable $srcfile {debug}] {
>>> >    return -1
>>> > }
>> After I use this part of code, I got:
>> ERROR: tcl error sourcing ../../../src/gdb/testsuite/gdb.base/dprintf-next.exp.
>> ERROR: wrong # args: should be "prepare_for_testing testname
>> executable ?sources? ?options?"
>>     while executing
>> "prepare_for_testing "failed to prepare for dprintf next tests""
>>     invoked from within
>> "if [prepare_for_testing "failed to prepare for dprintf next tests"
>>     $executable $srcfile {debug}] {
>>    return -1
>> }"
>
> Hmm?  Oh, it looks like you forgot the '\' at the end of the line.
> Please try again.

Fixed both dprintf-next.exp and dprintf-non-stop.exp.

>
>> So I change this part to:
>> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
>
> 'dprintf-next.exp' really isn't magical.  It's just a string.  :-)
>
>>     return -1
>> }
>
>>>> >> +gdb_test "next" ".*" "next 1"
>>>> >> +gdb_test "next" ".*" "next 2"
>>> >
>>> > Please use more string regexes for these, that confirm the next stopped
>>> > at the right lines.
>> Fixed.
>
> Sorry, I should have been clearer.  Don't do that by checking
> line numbers though, as those change when more functions are
> added to the test.  Just make each line look different instead.
>
>> +
>> +gdb_test "dprintf $dp_location, \"%d\\n\", x" \
>> +    "Dprintf .*"
>
> Please add an explicit 3rd parameter to gdb_test, so that
> the line number doesn't appear in gdb.sum.
>
>> +
>> +gdb_test "next" "23.*\\+\\+x.*" "next 1"
>> +gdb_test "next" "24.*\\+\\+x.*" "next 2"
>
> Do something like this:
>
> gdb_test "next" "\\+\\+x;" "next 1"
> gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"
>
> You could add a comment to the first stepped line to
> make that clearer:
>
> gdb_test "next" "\\+\\+x; /* step here.*" "next 1"
> gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"

This part is fixed.

>
>
>> --- /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>
>> +
>> +   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 ();
>> +  while (1);
>
> If something goes wrong, like GDB crashing, this could leave the test
> running forever, pegging the CPU.  Just make it sleep for some
> time instead.

Changed this part to sleep(3)

>
>> +  return 0;
>
>> +}
>> \ No newline at end of file
>
> Please watch out for these, and add a newline.
>

Fixed.

>> +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
>> +    }
>> +}
>
> This leaves the prompt in the expect buffer.  I think
> this is likely to confuse the following test that runs.
>

After change this part to:
gdb_test_multiple "" $test {
    -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n$gdb_prompt" {
	pass $test
    }
}
I got:
Running ../../../src/gdb/testsuite/gdb.base/dprintf-non-stop.exp ...
FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)

I thought the reason is:
(gdb) PASS: gdb.base/dprintf-non-stop.exp: interrupt

[process 24118] #1 stopped.
0x00002aaaaad8f830 in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6
FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)

There is not $gdb_prompt.


On Wed, May 22, 2013 at 10:04 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>
> Hui> So I change this part to:
> Hui> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
> Hui>     return -1
> Hui> }
>
> I think it is better to use the variables created by standard_testfile.

Fixed.

The attachments are the new patches.

Best,
Hui

2013-05-28  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.
	(initialize_breakpoint_ops): Set dprintf_create_breakpoints_sal
	and dprintf_after_condition_true.
	* breakpoint.h (breakpoint_ops): Add after_condition_true.

2013-05-28  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.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.

[-- Attachment #2: dprintf-continue.txt --]
[-- Type: text/plain, Size: 5391 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);
 	    }
 
 	}
@@ -8948,25 +8950,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);
@@ -12752,6 +12745,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,
@@ -12771,7 +12772,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.  */
@@ -13365,6 +13367,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);
+}
+
+/* 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').  */
 
@@ -15861,6 +15933,8 @@ initialize_breakpoint_ops (void)
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;
   ops->print_recreate = dprintf_print_recreate;
+  ops->create_breakpoints_sal = dprintf_create_breakpoints_sal;
+  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: 5914 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.
+   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/>.  */
+
+void
+foo ()
+{
+}
+
+int
+main ()
+{
+  foo ();
+  sleep (3);
+  return 0;
+}
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-non-stop.exp
@@ -0,0 +1,48 @@
+#   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/>.
+
+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" \
@@ -135,4 +133,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

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

[Yao Qi]

On 05/28/2013 08:01 AM, Hui Zhu wrote:
>>> +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
>>> >>+    }
>>> >>+}
>> >
>> >This leaves the prompt in the expect buffer.  I think
>> >this is likely to confuse the following test that runs.
>> >
> After change this part to:
> gdb_test_multiple "" $test {
>      -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n$gdb_prompt" {
> 	pass $test
>      }
> }
> I got:
> Running ../../../src/gdb/testsuite/gdb.base/dprintf-non-stop.exp ...
> FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)
>

because the gdb_prompt has been consumed by the previous matching:

gdb_test_multiple $test $test {
     -re "interrupt\r\n$gdb_prompt " {
                       ^^^^^^^^^^^^
      pass $test
     }
}

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

I raise a question here that no one asked before, why don't combine 
these two gdb_test_multiple into one? like:

set test "interrupt inferior"
gdb_test_multiple "interrupt" $test {
     -re "interrupt\r\n.*\\\[.* \[0-9\]+\\\] #1 stopped\\\." {
	pass $test
     }
}
-- 
Yao (齐尧)

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

[Hui Zhu]

On Tue, May 28, 2013 at 11:36 AM, Yao Qi <yao@codesourcery.com> wrote:
> On 05/28/2013 08:01 AM, Hui Zhu wrote:
>>>>
>>>> +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
>>>> >>+    }
>>>> >>+}
>>>
>>> >
>>> >This leaves the prompt in the expect buffer.  I think
>>> >this is likely to confuse the following test that runs.
>>> >
>>
>> After change this part to:
>> gdb_test_multiple "" $test {
>>      -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n$gdb_prompt" {
>>         pass $test
>>      }
>> }
>> I got:
>> Running ../../../src/gdb/testsuite/gdb.base/dprintf-non-stop.exp ...
>> FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)
>>
>
> because the gdb_prompt has been consumed by the previous matching:
>
> gdb_test_multiple $test $test {
>     -re "interrupt\r\n$gdb_prompt " {
>                       ^^^^^^^^^^^^
>      pass $test
>
>     }
> }

I tried move $gdb_prompt to other part but still not work.

>
>> +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
>> +    }
>> +}
>
>
> I raise a question here that no one asked before, why don't combine these
> two gdb_test_multiple into one? like:
>
> set test "interrupt inferior"
> gdb_test_multiple "interrupt" $test {
>     -re "interrupt\r\n.*\\\[.* \[0-9\]+\\\] #1 stopped\\\." {
>         pass $test
>     }
> }

This cannot pass test with gdbserver.  I think that is why
async-shell.exp do something like it.

Thanks,
Hui

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

[Hui Zhu]

Ping http://sourceware.org/ml/gdb-patches/2013-05/msg00958.html

Thanks,
Hui

On Tue, May 28, 2013 at 8:01 AM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Pedro and Tom,
>
> Thanks for your review.
>
> On Wed, May 22, 2013 at 8:46 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 05/22/2013 11:21 AM, Hui Zhu wrote:
>>
>>> I tried it on fedora and looks it works OK most of times.  I only got
>>> trouble is when run this test with remote, [runto main] with
>>> "non-stop" is open will fail sometime.
>>
>> Sorry, but "most of the times" is not acceptable.  Can you please
>> figure out why that is so?  This should never fail.
>
> I found a issue about remote test and non-stop.  Because it doesn't
> have a lot of relation with dprintf, I will post a separate patch for
> that later.
>
>>
>>>> > Not sure even that is necessary, given bs->stop==0 and:
>>>> >
>>>> >       /* 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;
>>> I am not sure about this part.
>>> If my understanding about Tom's review in before is right, the action
>>> of dprintf command should be handled when GDB check the condition.
>>> And after that, dprintf not need be handled.
>>
>> Yes, but I don't see how that counters my suggestion.
>
> Added dprintf_create_breakpoints_sal. When dprintf is created, auto
> set silent to 0.
>
>>
>>>> >
>>>>> >> +  bpstat_do_actions_1 (&bs);
>>>>> >> +  bs->next = tmp;
>>>>> >> +}
>>>> >
>>>> > Could you add some comments explaining what this is
>>>> > doing, and why?
>>> I update this function to following part:
>>> /* Implement the "after_cond" breakpoint_ops method for dprintf.  */
>>>
>>> static void
>>> dprintf_after_cond (struct bpstats *bs)
>>
>>
>>> {
>>>   struct cleanup *old_chain;
>>>   struct breakpoint *b = bs->breakpoint_at;
>>>
>>>   bs->commands = b->commands;
>>>   incref_counted_command_line (bs->commands);
>>
>> I still think there's no point in moving this to the
>> after_condition_true hook, if both existing implementations
>> end up doing it themselves.  See below.
>>
>>>
>>>   /* Because function bpstat_do_actions_1 will execute all the command
>>>      list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
>>>      NULL.  */
>>>   old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
>>
>> That cast is invalid actually.  Older gcc's will complain about
>> the aliasing violation.  I think we can get away without this, by
>> passing a separate list with only one entry to bpstat_do_actions_1.
>>
>>>   bs->next = NULL;
>>>   bpstat_do_actions_1 (&bs);
>>>   do_cleanups (old_chain);
>>>
>>>   /* This dprintf need not be handled after this function
>>>      because its command list is executed by bpstat_do_actions_1.
>>>      Clear STOP and PRINT for that.  */
>>>   bs->stop = 0;
>>>   bs->print = 0;
>>
>> Thanks.  I was more looking for general design comments on why we
>> run the command list here.  This also should explain why clear stop
>> here, instead of on the more natural check_status.
>>
>> So all in all, if it works for you (seems to work fine for me), I'd
>> rather do as this patch on top of yours does:
>>
>
> Thanks for this patch.  I merged it to dprintf-continue patch.
>
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>  gdb/breakpoint.c |   62 ++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 37 insertions(+), 25 deletions(-)
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 7ef5703..181aecc 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -5292,6 +5292,15 @@ bpstat_stop_status (struct address_space *aspace,
>>                     b->enable_state = bp_disabled;
>>                   removed_any = 1;
>>                 }
>> +
>> +             if (b->silent)
>> +               bs->print = 0;
>> +             bs->commands = b->commands;
>> +             incref_counted_command_line (bs->commands);
>> +             if (command_line_is_silent (bs->commands
>> +                                         ? bs->commands->commands : NULL))
>> +               bs->print = 0;
>> +
>>               b->ops->after_condition_true (bs);
>>             }
>>
>> @@ -12742,15 +12751,7 @@ base_breakpoint_explains_signal (struct breakpoint *b)
>>  static void
>>  base_breakpoint_after_condition_true (struct bpstats *bs)
>>  {
>> -  struct breakpoint *b = bs->breakpoint_at;
>> -
>> -  if (b->silent)
>> -    bs->print = 0;
>> -  bs->commands = b->commands;
>> -  incref_counted_command_line (bs->commands);
>> -  if (command_line_is_silent (bs->commands
>> -                             ? bs->commands->commands : NULL))
>> -    bs->print = 0;
>> +  /* Nothing to do.   */
>>  }
>>
>>  struct breakpoint_ops base_breakpoint_ops =
>> @@ -13368,30 +13369,41 @@ dprintf_print_recreate (struct breakpoint *tp, struct ui_file *fp)
>>  }
>>
>>  /* Implement the "after_condition_true" breakpoint_ops method for
>> -   dprintf.  */
>> +   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 breakpoint *b = bs->breakpoint_at;
>> +  struct bpstats tmp_bs = { NULL };
>> +  struct bpstats *tmp_bs_p = &tmp_bs;
>>
>> -  bs->commands = b->commands;
>> -  incref_counted_command_line (bs->commands);
>> +  /* 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;
>>
>> -  /* Because function bpstat_do_actions_1 will execute all the command
>> -     list of BS and follow it by BS->NEXT, temporarily set BS->NEXT to
>> -     NULL.  */
>> -  old_chain = make_cleanup_restore_ptr ((void **)&bs->next);
>> -  bs->next = NULL;
>> -  bpstat_do_actions_1 (&bs);
>> -  do_cleanups (old_chain);
>> +  /* 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);
>>
>> -  /* This dprintf need not be handled after this function
>> -     because its command list is executed by bpstat_do_actions_1.
>> -     Clear STOP and PRINT for that.  */
>> -  bs->stop = 0;
>> -  bs->print = 0;
>> +  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
>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>>>> >> +if [prepare_for_testing $expfile $executable $srcfile {debug}] {
>>>>> >> +    untested "failed to prepare for trace tests"
>>>> >
>>>> > This is not a trace test.  Actually, prepare_for_testing
>>>> > already calls untested with whatever we pass it as first argument.
>>>> > I think it'd be the first such call in the testsuite, but I
>>>> > think this would be better:
>>>> >
>>>> > if [prepare_for_testing "failed to prepare for trace tests" \
>>>> >     $executable $srcfile {debug}] {
>>>> >    return -1
>>>> > }
>>> After I use this part of code, I got:
>>> ERROR: tcl error sourcing ../../../src/gdb/testsuite/gdb.base/dprintf-next.exp.
>>> ERROR: wrong # args: should be "prepare_for_testing testname
>>> executable ?sources? ?options?"
>>>     while executing
>>> "prepare_for_testing "failed to prepare for dprintf next tests""
>>>     invoked from within
>>> "if [prepare_for_testing "failed to prepare for dprintf next tests"
>>>     $executable $srcfile {debug}] {
>>>    return -1
>>> }"
>>
>> Hmm?  Oh, it looks like you forgot the '\' at the end of the line.
>> Please try again.
>
> Fixed both dprintf-next.exp and dprintf-non-stop.exp.
>
>>
>>> So I change this part to:
>>> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
>>
>> 'dprintf-next.exp' really isn't magical.  It's just a string.  :-)
>>
>>>     return -1
>>> }
>>
>>>>> >> +gdb_test "next" ".*" "next 1"
>>>>> >> +gdb_test "next" ".*" "next 2"
>>>> >
>>>> > Please use more string regexes for these, that confirm the next stopped
>>>> > at the right lines.
>>> Fixed.
>>
>> Sorry, I should have been clearer.  Don't do that by checking
>> line numbers though, as those change when more functions are
>> added to the test.  Just make each line look different instead.
>>
>>> +
>>> +gdb_test "dprintf $dp_location, \"%d\\n\", x" \
>>> +    "Dprintf .*"
>>
>> Please add an explicit 3rd parameter to gdb_test, so that
>> the line number doesn't appear in gdb.sum.
>>
>>> +
>>> +gdb_test "next" "23.*\\+\\+x.*" "next 1"
>>> +gdb_test "next" "24.*\\+\\+x.*" "next 2"
>>
>> Do something like this:
>>
>> gdb_test "next" "\\+\\+x;" "next 1"
>> gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"
>>
>> You could add a comment to the first stepped line to
>> make that clearer:
>>
>> gdb_test "next" "\\+\\+x; /* step here.*" "next 1"
>> gdb_test "next" "\\+\\+x; /* set dprintf here" "next 2"
>
> This part is fixed.
>
>>
>>
>>> --- /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>
>>> +
>>> +   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 ();
>>> +  while (1);
>>
>> If something goes wrong, like GDB crashing, this could leave the test
>> running forever, pegging the CPU.  Just make it sleep for some
>> time instead.
>
> Changed this part to sleep(3)
>
>>
>>> +  return 0;
>>
>>> +}
>>> \ No newline at end of file
>>
>> Please watch out for these, and add a newline.
>>
>
> Fixed.
>
>>> +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
>>> +    }
>>> +}
>>
>> This leaves the prompt in the expect buffer.  I think
>> this is likely to confuse the following test that runs.
>>
>
> After change this part to:
> gdb_test_multiple "" $test {
>     -re "\r\n\\\[.* \[0-9\]+\\\] #1 stopped\\\.\r\n$gdb_prompt" {
>         pass $test
>     }
> }
> I got:
> Running ../../../src/gdb/testsuite/gdb.base/dprintf-non-stop.exp ...
> FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)
>
> I thought the reason is:
> (gdb) PASS: gdb.base/dprintf-non-stop.exp: interrupt
>
> [process 24118] #1 stopped.
> 0x00002aaaaad8f830 in nanosleep () from /lib/x86_64-linux-gnu/libc.so.6
> FAIL: gdb.base/dprintf-non-stop.exp: inferior stopped (timeout)
>
> There is not $gdb_prompt.
>
>
> On Wed, May 22, 2013 at 10:04 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:
>>
>> Hui> So I change this part to:
>> Hui> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
>> Hui>     return -1
>> Hui> }
>>
>> I think it is better to use the variables created by standard_testfile.
>
> Fixed.
>
> The attachments are the new patches.
>
> Best,
> Hui
>
> 2013-05-28  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.
>         (initialize_breakpoint_ops): Set dprintf_create_breakpoints_sal
>         and dprintf_after_condition_true.
>         * breakpoint.h (breakpoint_ops): Add after_condition_true.
>
> 2013-05-28  Yao Qi  <yao@codesourcery.com>
>             Hui Zhu  <hui@codesourcery.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.

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

[Pedro Alves]

On 06/03/2013 05:06 AM, Hui Zhu wrote:
> Ping http://sourceware.org/ml/gdb-patches/2013-05/msg00958.html
> 

As this exposes the non-stop racy failure, we should fix it that one
first.  Failing that, we should kfail or skip the test with
remote targets.  Let's consider the latter option later if we don't
manage to address the race timely.  As I said on:

 http://sourceware.org/ml/gdb-patches/2013-05/msg01111.html

I'm investigating this.  I have a prototype patch, but I need
a bit more to handle some details, like what to do with
signal catchpoints when we find threads had been stopped with
a signal (I'm currently thinking of skipping the catchpoints).
I'm composing a test to exercise/expose this kind of stuff,
for a better RFC.

-- 
Pedro Alves

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

[Hui Zhu]

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

Hi Pedro,

Thanks for your review.

On Tue, Jun 4, 2013 at 1:48 AM, Pedro Alves <palves@redhat.com> wrote:
> On 06/03/2013 05:06 AM, Hui Zhu wrote:
>> Ping http://sourceware.org/ml/gdb-patches/2013-05/msg00958.html
>>
>
> As this exposes the non-stop racy failure, we should fix it that one
> first.  Failing that, we should kfail or skip the test with
> remote targets.  Let's consider the latter option later if we don't
> manage to address the race timely.  As I said on:
>
>  http://sourceware.org/ml/gdb-patches/2013-05/msg01111.html
>
> I'm investigating this.  I have a prototype patch, but I need
> a bit more to handle some details, like what to do with
> signal catchpoints when we find threads had been stopped with
> a signal (I'm currently thinking of skipping the catchpoints).
> I'm composing a test to exercise/expose this kind of stuff,
> for a better RFC.

I add a check in the begin of dprintf-non-stop.exp.  Then if the
target in remote, this test will not support.

Please help me review it.

Thanks,
Hui

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.
	(initialize_breakpoint_ops): Set dprintf_create_breakpoints_sal
	and dprintf_after_condition_true.
	* breakpoint.h (breakpoint_ops): Add after_condition_true.

2013-06-07  Yao Qi  <yao@codesourcery.com>
	    Hui Zhu  <hui@codesourcery.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.

[-- Attachment #2: dprintf-continue.txt --]
[-- Type: text/plain, Size: 5391 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,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);
+}
+
+/* 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 +15919,8 @@ initialize_breakpoint_ops (void)
   ops->print_it = bkpt_print_it;
   ops->print_mention = bkpt_print_mention;
   ops->print_recreate = dprintf_print_recreate;
+  ops->create_breakpoints_sal = dprintf_create_breakpoints_sal;
+  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: 6024 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.
+   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/>.  */
+
+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.
+#   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."
+    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" \
@@ -135,4 +133,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

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

[Hui Zhu]

Ping http://sourceware.org/ml/gdb-patches/2013-06/msg00139.html

Thanks,
Hui

On Fri, Jun 7, 2013 at 11:15 AM, Hui Zhu <teawater@gmail.com> wrote:
> Hi Pedro,
>
> Thanks for your review.
>
> On Tue, Jun 4, 2013 at 1:48 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 06/03/2013 05:06 AM, Hui Zhu wrote:
>>> Ping http://sourceware.org/ml/gdb-patches/2013-05/msg00958.html
>>>
>>
>> As this exposes the non-stop racy failure, we should fix it that one
>> first.  Failing that, we should kfail or skip the test with
>> remote targets.  Let's consider the latter option later if we don't
>> manage to address the race timely.  As I said on:
>>
>>  http://sourceware.org/ml/gdb-patches/2013-05/msg01111.html
>>
>> I'm investigating this.  I have a prototype patch, but I need
>> a bit more to handle some details, like what to do with
>> signal catchpoints when we find threads had been stopped with
>> a signal (I'm currently thinking of skipping the catchpoints).
>> I'm composing a test to exercise/expose this kind of stuff,
>> for a better RFC.
>
> I add a check in the begin of dprintf-non-stop.exp.  Then if the
> target in remote, this test will not support.
>
> Please help me review it.
>
> Thanks,
> Hui
>
> 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.
>         (initialize_breakpoint_ops): Set dprintf_create_breakpoints_sal
>         and dprintf_after_condition_true.
>         * breakpoint.h (breakpoint_ops): Add after_condition_true.
>
> 2013-06-07  Yao Qi  <yao@codesourcery.com>
>             Hui Zhu  <hui@codesourcery.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.

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

[Pedro Alves]

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

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

[Hui Zhu]

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

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



[-- 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: 6138 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,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

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

[Pedro Alves]

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

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

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

[Hui Zhu]

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

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

[Pedro Alves]

On 06/25/2013 09:53 AM, Hui Zhu wrote:
> On 06/25/13 04:20, Pedro Alves wrote:

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

OK.

Thanks,
-- 
Pedro Alves

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

[Hui Zhu]

On 06/25/13 17:57, Pedro Alves wrote:
> On 06/25/2013 09:53 AM, Hui Zhu wrote:
>> On 06/25/13 04:20, Pedro Alves wrote:
>
>>> 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.
>
> OK.
>
> Thanks,
>

Thanks for your help.  Checked in to http://sourceware.org/ml/gdb-cvs/2013-06/msg00156.html

Can I commit the patch to 7.6 branch?

Thanks,
Hui

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

[Pedro Alves]

On 06/25/2013 12:47 PM, Hui Zhu wrote:

> Thanks for your help.  Checked in to http://sourceware.org/ml/gdb-cvs/2013-06/msg00156.html
> 
> Can I commit the patch to 7.6 branch?

Fine with me.  This is pretty isolated to dprintf, so should
be safe enough.  Please add a reference to the PRs at:
<http://sourceware.org/gdb/wiki/GDB_7.6_Release>.

-- 
Pedro Alves

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

[Hui Zhu]

On Tue, Jun 25, 2013 at 9:56 PM, Pedro Alves <palves@redhat.com> wrote:
> On 06/25/2013 12:47 PM, Hui Zhu wrote:
>
>> Thanks for your help.  Checked in to http://sourceware.org/ml/gdb-cvs/2013-06/msg00156.html
>>
>> Can I commit the patch to 7.6 branch?
>
> Fine with me.  This is pretty isolated to dprintf, so should
> be safe enough.  Please add a reference to the PRs at:
> <http://sourceware.org/gdb/wiki/GDB_7.6_Release>.
>

Thanks.  Checked in to 7.6 branch.
http://sourceware.org/ml/gdb-cvs/2013-06/msg00161.html
http://sourceware.org/ml/gdb-cvs/2013-06/msg00162.html
And I updated  <http://sourceware.org/gdb/wiki/GDB_7.6_Release> for this fix.

Best,
Hui

> --
> Pedro Alves
>

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

[Tom Tromey]

>>>>> "Hui" == Hui Zhu <teawater@gmail.com> writes:

Hui> So I change this part to:
Hui> if { [prepare_for_testing dprintf-next.exp "dprintf-next" {} {debug}] } {
Hui>     return -1
Hui> }

I think it is better to use the variables created by standard_testfile.

Tom

DPrintf feedback (was: [RFC] PR 15075 dprintf interferes with "next")

[Marc Khouzam]

Hi,

Yao posting a patch for dprintf was good timing.
I've been working on integrating the dprintf feature
into Eclipse and I ran into some issues.

I think the main point to discuss is the fact that
dprintf use breakpoint commands.  This has been put in question,
a couple of times on the mailing list I believe.  An impact
of that approach is that although in CLI, dprintf are silent 
breakpoints, they don't seem to be in MI.  When I put a dprintf 
in a loop and run over it, I get a bunch of *stopped/*running 
events and =breakpoint-modified events.

I'm not sure how I feel about the =break-modified event just yet
and your expert opinion would be appreciated.  However, the 
*stopped/*running events seem like they shouldn't be there.
They cause a lot of un-needed communication between Eclipse and GDB.

This gives me the feeling that it would be better to have dprintf
behave like an active tracepoint instead of a breakpoint.  I believe
Joel had suggested that at one point.

Such an approach may also help fix the "continue" problem
that Yao is trying to address.  I seem to recall that Hui had posted 
patches to add some kind of 'printf' tracepoint command.  Maybe that 
would be something that can help?

Also, as pointed out by Pedro, the user is able to modify the 
dprintf breakpoint commands, which can suddenly turn a dprintf
into something else, although GDB still has it tagged as 'dprintf'.
The fact that GDB has a new type 'dprintf', implies that dprintf
should keep their 'dprintf' properties, and not be turned into
a standard bp.

Other issues I ran across are below.  I wasn't sure opening PRs was
the way to do since those issues may disappear if the current 
implementation is amended:

- If I set a condition on a dprintf, things don't behave right.
  When the condition is false, the printout is not shown, but the 
  dprintf causes the inferior to stop and not continue.

- Setting the dprintf-style to 'agent' for a local session defaults
  to 'gdb' style, but the 'continue' command is not added in that 
  case (easy to fix)

- Pagination is triggered for dprintf in CLI mode, at least when
  using dprintf-style 'gdb'.  I think this is a problem because:
   a) when pagination is triggered, inferior execution will be 
      interrupted until the user answers the pagination prompt
   b) pagination is triggered by the dprintf but not by real 
      inferior output. So, as dprintf and inferior printouts 
      appear interleaved on the screen, the pagination prompt 
      will be triggered when the dprintfs add up to too many, 
      which will seem random to the user, since the other 
      printouts are also visible.

- In Eclipse, the dprintf-style that makes sense is either 
  'call' or 'agent'.  'call' will call 'printf' in the inferior.
  With GDB 7.5, GDB cannot call an inferior method on Ubuntu, 
  which is a serious limitation for eclipse/dprintf.
  http://sourceware.org/ml/gdb-patches/2012-07/msg00037.html

- Updating the dprintf-style to 'agent' for existing dprintfs 
  will cause: "May only run agent-printf on the target" when 
  it is time to print.

- The fact that there is no expression checking on the validity 
  of the dprintf string can be a user-friendliness problem

- Output buffering is not behaving as a real printf.  For 
  example, if my program does
     printf("hello");
     printf("friend\n");
and I put a dprintf " my " on the second line, 
I would expect to see
   "hello my friend"
but instead I see
   " my hellofriend"
which shows that the dprintf string does not go to the same 
buffer as the real printfs, and is flushed earlier.
This also happens with the dprintf-style "agent"

Thanks 

Marc

Re: DPrintf feedback

[Tom Tromey]

>>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:

Marc> Such an approach may also help fix the "continue" problem
Marc> that Yao is trying to address.  I seem to recall that Hui had posted 
Marc> patches to add some kind of 'printf' tracepoint command.  Maybe that 
Marc> would be something that can help?

I thought this was subsumed by "set dprintf-style agent"?

Marc> Other issues I ran across are below.  I wasn't sure opening PRs was
Marc> the way to do since those issues may disappear if the current 
Marc> implementation is amended:

I think it is best to file PRs.  If a patch fixes them all, then that is
fine.  But if not, then this the only way to avoid losing track.

Marc> - Pagination is triggered for dprintf in CLI mode, at least when
Marc> using dprintf-style 'gdb'.

Maybe we need a new "gdb-unfiltered" setting?  Or we could disable
pagination entirely here, but that maybe leads to other issues.

Marc> - In Eclipse, the dprintf-style that makes sense is either 
Marc>   'call' or 'agent'.

I was surprised to hear this.  Could you say why?

Marc> - Output buffering is not behaving as a real printf.  For 
Marc>   example, if my program does
Marc>      printf("hello");
Marc>      printf("friend\n");
Marc> and I put a dprintf " my " on the second line, 
Marc> I would expect to see
Marc>    "hello my friend"
Marc> but instead I see
Marc>    " my hellofriend"
Marc> which shows that the dprintf string does not go to the same 
Marc> buffer as the real printfs, and is flushed earlier.
Marc> This also happens with the dprintf-style "agent"

Which other setting does this happen with?
I would expect it to work with "call" but not with "gdb".

You could hack around this by disabling buffering for stdout.
I'm not totally sure it is a bug for it to work the current way by
default.

Tom

RE: DPrintf feedback

[Marc Khouzam]

> -----Original Message-----
> From: Tom Tromey [mailto:tromey@redhat.com] 
> Sent: Friday, February 22, 2013 2:32 PM
> To: Marc Khouzam
> Cc: 'Yao Qi'; 'gdb-patches@sourceware.org'; 'Pedro Alves'; 
> 'Joel Brobecker'; 'Stan Shebs'
> Subject: Re: DPrintf feedback
> 
> >>>>> "Marc" == Marc Khouzam <marc.khouzam@ericsson.com> writes:
> 
> Marc> Such an approach may also help fix the "continue" problem
> Marc> that Yao is trying to address.  I seem to recall that 
> Hui had posted 
> Marc> patches to add some kind of 'printf' tracepoint 
> command.  Maybe that 
> Marc> would be something that can help?
> 
> I thought this was subsumed by "set dprintf-style agent"?

I haven't followed the details, so I can't be sure.
"set dprintf-style agent" makes the dprintf breakpoint 
command be "agent-printf <string>".
I didn't expect that to be something that could be re-used
by a tracepoint command.  But that makes me realize the 
bigger limitation of using tracepoints; tracepoints are
only supported with gdbserver.  I thought a tracepoint
doing the printf would be nice for each case, not just
with gdbserver.  I guess there's a lot more work
needed to first support tracepoints in GDB alone, and
then use them for dprintf.  I missed that detail.

I wonder if you guys feel tracepoints would be a nicer
way to handle dprintf?

> Marc> Other issues I ran across are below.  I wasn't sure 
> opening PRs was
> Marc> the way to do since those issues may disappear if the current 
> Marc> implementation is amended:
> 
> I think it is best to file PRs.  If a patch fixes them all, 
> then that is
> fine.  But if not, then this the only way to avoid losing track.

Will do.

> Marc> - Pagination is triggered for dprintf in CLI mode, at least when
> Marc> using dprintf-style 'gdb'.
> 
> Maybe we need a new "gdb-unfiltered" setting?  Or we could disable
> pagination entirely here, but that maybe leads to other issues.
> 
> Marc> - In Eclipse, the dprintf-style that makes sense is either 
> Marc>   'call' or 'agent'.
> 
> I was surprised to hear this.  Could you say why?

On Linux, Eclipse uses the -inferior-tty-set command to tell GDB
to send the inferior's output to a different console.  So we have
a gdb console and an inferior console.  If we want dprintf to look 
like a real printf, its output needs to be shown in the inferior
console, so it requires GDB to send it to the same tty.  That is
not what "dprintf-style gdb" does, as it sends it to the gdb console.

> Marc> - Output buffering is not behaving as a real printf.  For 
> Marc>   example, if my program does
> Marc>      printf("hello");
> Marc>      printf("friend\n");
> Marc> and I put a dprintf " my " on the second line, 
> Marc> I would expect to see
> Marc>    "hello my friend"
> Marc> but instead I see
> Marc>    " my hellofriend"
> Marc> which shows that the dprintf string does not go to the same 
> Marc> buffer as the real printfs, and is flushed earlier.
> Marc> This also happens with the dprintf-style "agent"
> 
> Which other setting does this happen with?
> I would expect it to work with "call" but not with "gdb".
>
> You could hack around this by disabling buffering for stdout.
> I'm not totally sure it is a bug for it to work the current way by
> default.

I hadn't tried it with "call" because my machine is an Ubuntu,
so the "call" setting does not work.  I managed to get my hands
on a Suse machine and tried it; you are right, "call" prints
things nicely.  I guess the problem is only that "agent" style
does not do that properly.  I'll specify this important point
in the PR.

Thanks for the quick answer.

Marc

RE: DPrintf feedback

[Marc Khouzam]

> > I think it is best to file PRs.  If a patch fixes them all, 
> > then that is
> > fine.  But if not, then this the only way to avoid losing track.
> 
> Will do.

I've opened the following 8 PRs for tracking:

http://sourceware.org/bugzilla/show_bug.cgi?id=15181	
Missing continue on dprintf that defaults to "gdb" style from "agent"
http://sourceware.org/bugzilla/show_bug.cgi?id=15186 
Agent style dprintf does not buffer output properly
http://sourceware.org/bugzilla/show_bug.cgi?id=15182 
Pagination is triggered for dprintf
http://sourceware.org/bugzilla/show_bug.cgi?id=15187 
Dprintf does not work for Eclipse on Ubuntu
http://sourceware.org/bugzilla/show_bug.cgi?id=15179 
Dprintf interrupts execution when condition is false
http://sourceware.org/bugzilla/show_bug.cgi?id=15184 
Cannot change dprintf style to agent for existing dprintfs
http://sourceware.org/bugzilla/show_bug.cgi?id=15180	
Agent style dprintf does not respect conditions
http://sourceware.org/bugzilla/show_bug.cgi?id=15185 
Dprintf should parse its string for validity right away

BR,

Marc

[PATCH 1/4] Fix dprintf bugs

[Yao Qi]

Hi,
This patch fixes PR15075.  Besides this bug, this patch also fixes
some other problems:

  - Two dprintf are set on the same address.  Only one printf is
    executed.
  - dprintf and regular breakpoint are set on the same address,
    inferior doesn't stop.

In order to fix these problems, I don't append "continue" command to
dprintf breakpoint, and execute commands of dprintf after
bpstat_check_breakpoint_conditions if condition is true.  The reason I
choose this place (after bpstat_check_breakpoint_conditions instead of
in bpstat_check_breakpoint_conditions) to handle dprintf is that we
have to set BS->stop to zero, but have to update hit count if
condition is true, so we have to do two things together.

With this patch, GDB executes dprintf commands after
bpstat_check_breakpoint_conditions, the different place where
breakpoint commands are executed.  I propose to disable setting
command for dprintf, or disallow user setting commands for dprintf.
If we believe it is important to allow user setting commands for
dprintf (a good use case?), we need a new design like creating struct
dprintf as a 'sub-class' of breakpoint and adding its own field for
printf commands.  The code on sending commands to the remote should be
adjusted as well.

Running gdb.base/pr15075.exp on mainline GDB (unpatched) will get the
following FAIL,

  FAIL: gdb.base/pr15075.exp: p x

and this patch fixes this FAIL.

gdb:

2013-02-28  Yao Qi  <yao@codesourcery.com>

	PR breakpoints/15075
	* breakpoint.c (bpstat_stop_status): Execute commands of
	dprintf if BS->stop is true.  Update hit count and set
	BS->stop to zero.
	(bpstat_what): Set 'this_action' to BPSTAT_WHAT_SINGLE if
	breakpoint is dprintf.
	(update_dprintf_command_list): Don't append "continue" command
	to the commands of dprintf.
	(do_map_commands_command): If breakpoint is dprintf, throw an
	error.

	* NEWS: Mention that GDB disallows setting commands for dprintf.

gdb/testsuite:

2013-02-28  Yao Qi  <yao@codesourcery.com>

	* 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.
	* gdb.base/pr15075.c: New.
	* gdb.base/pr15075.exp: New.
---
 gdb/NEWS                                       |    2 +
 gdb/breakpoint.c                               |   43 +++++++++++++++--------
 gdb/testsuite/gdb.base/dprintf.exp             |    2 -
 gdb/testsuite/gdb.base/pr15075.c               |   26 ++++++++++++++
 gdb/testsuite/gdb.base/pr15075.exp             |   38 +++++++++++++++++++++
 gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp |    2 +-
 6 files changed, 95 insertions(+), 18 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/pr15075.c
 create mode 100644 gdb/testsuite/gdb.base/pr15075.exp

diff --git a/gdb/NEWS b/gdb/NEWS
index 0877aa2..2f0a157 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -70,6 +70,8 @@ Lynx 178 PowerPC		powerpc-*-lynx*178
 * The command 'info tracepoints' can now display 'installed on target'
   or 'not installed on target' for each non-pending location of tracepoint.
 
+* GDB now disallows setting commands for dynamic printf breakpoint.
+
 * New configure options
 
 --enable-libmcheck/--disable-libmcheck
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index fb57a57..d4c3690 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1295,6 +1295,10 @@ do_map_commands_command (struct breakpoint *b, void *data)
 {
   struct commands_info *info = data;
 
+  if (b->type == bp_dprintf)
+    error (_("Setting commands for dprintf breakpoint is not "
+	     "allowed"));
+
   if (info->cmd == NULL)
     {
       struct command_line *l;
@@ -5272,6 +5276,25 @@ bpstat_stop_status (struct address_space *aspace,
 	{
 	  bpstat_check_breakpoint_conditions (bs, ptid);
 
+	  if (bs->stop && b->type == bp_dprintf)
+	    {
+	      struct counted_command_line * ccmd = b->commands;
+	      struct command_line *cmd = ccmd ? ccmd->commands : NULL;
+
+	      while (cmd != NULL)
+		{
+		  execute_control_command (cmd);
+		  cmd = cmd->next;
+		}
+
+	      /* Update the hit count of dprintf breakpoint, although
+		 we set BS->stop zero.  */
+	      ++(b->hit_count);
+	      observer_notify_breakpoint_modified (b);
+
+	      bs->stop = 0;
+	    }
+
 	  if (bs->stop)
 	    {
 	      ++(b->hit_count);
@@ -5519,7 +5542,7 @@ bpstat_what (bpstat bs_head)
 	  break;
 
 	case bp_dprintf:
-	  this_action = BPSTAT_WHAT_STOP_SILENT;
+	  this_action = BPSTAT_WHAT_SINGLE;
 	  break;
 
 	default:
@@ -8947,25 +8970,15 @@ update_dprintf_command_list (struct breakpoint *b)
 		    _("Invalid dprintf style."));
 
   gdb_assert (printf_line != NULL);
-  /* Manufacture a printf/continue sequence.  */
+  /* Manufacture a printf command.  */
   {
-    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);
diff --git a/gdb/testsuite/gdb.base/dprintf.exp b/gdb/testsuite/gdb.base/dprintf.exp
index 2b00c24..ffbd3f2 100644
--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -48,10 +48,8 @@ gdb_test_sequence "info breakpoints" "dprintf info 1" {
     "\[\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" \
diff --git a/gdb/testsuite/gdb.base/pr15075.c b/gdb/testsuite/gdb.base/pr15075.c
new file mode 100644
index 0000000..60fe8cf
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr15075.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;
+   ++x; /* set dprintf here */
+   return x - 7;
+ }
diff --git a/gdb/testsuite/gdb.base/pr15075.exp b/gdb/testsuite/gdb.base/pr15075.exp
new file mode 100644
index 0000000..86b24cc
--- /dev/null
+++ b/gdb/testsuite/gdb.base/pr15075.exp
@@ -0,0 +1,38 @@
+# 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 $expfile $executable $srcfile {debug}] {
+    untested "failed to prepare for trace tests"
+    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" ".*" "next 1"
+gdb_test "next" ".*" "next 2"
+# Test inferior doesn't exit.
+gdb_test "p x" ".* = 6"
diff --git a/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp b/gdb/testsuite/gdb.mi/mi-breakpoint-changed.exp
index fd32698..24ff55b 100644
--- 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
-- 
1.7.7.6

[PATCH 2/4] Test case of conditional dprintf

[Yao Qi]

This is to test that dprintf works well with condition.  Since print
stuff in dprintf is implemented as commands, so there should be no
FAIL without patch 1/5 applied.

Note that this case is similar to

  PR 15179 - Dprintf interrupts execution when condition is false

but I am still unable to reproduce this PR.  dprintf-cond.exp can easily
extended to reproduce PR 15180 in next step.

gdb/testsuite:

2013-02-28  Yao Qi  <yao@codesourcery.com>

     * gdb.base/dprintf-cond.exp: New.
---
 gdb/testsuite/gdb.base/dprintf-cond.exp |   75 +++++++++++++++++++++++++++++++
 1 files changed, 75 insertions(+), 0 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/dprintf-cond.exp

diff --git a/gdb/testsuite/gdb.base/dprintf-cond.exp b/gdb/testsuite/gdb.base/dprintf-cond.exp
new file mode 100644
index 0000000..b692124
--- /dev/null
+++ b/gdb/testsuite/gdb.base/dprintf-cond.exp
@@ -0,0 +1,75 @@
+# 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 pr15075.c
+
+set executable $testfile
+set expfile $testfile.exp
+
+set dp_location [gdb_get_line_number "set dprintf here"]
+set bp_location [expr $dp_location + 1]
+
+if [prepare_for_testing $expfile $executable $srcfile {debug}] {
+    untested "failed to prepare for trace tests"
+    return -1
+}
+
+proc test_dprintf { style } { with_test_prefix "$style" {
+
+    global executable
+    global dp_location bp_location
+
+    # Start with a fresh gdb.
+    clean_restart ${executable}
+    if ![runto_main] {
+	fail "Can't run to main"
+	return -1
+    }
+
+    gdb_test_no_output "set dprintf-style ${style}" \
+	"Set dprintf style"
+    gdb_test "dprintf $dp_location, \"false, %d\\n\", x" \
+	"Dprintf 2 at .*" "dprintf 1"
+    gdb_test "cond 2 x == 5"
+
+    gdb_test "dprintf $dp_location, \"true, %d\\n\", x" \
+	"Dprintf 3 at .*" "dprintf 2"
+    gdb_test "cond 3 x > 5"
+
+    gdb_test "break $bp_location" "Breakpoint .*"
+
+    gdb_test "continue" "true, 6\\r\\n\\r\\n.*" "continue"
+
+    # Test program stops on the right breakpoint.
+    gdb_test "p x" ".* = 7"
+
+    # Test the hit count of dprintf breakpoints.
+    gdb_test_sequence "info breakpoints" "dprintf info" {
+	"\[\r\n\]Num     Type           Disp Enb Address +What"
+	"\[\r\n\]1       breakpoint"
+	"\[\r\n\]2       dprintf"
+	"\[\r\n\]\tstop only if x == 5"
+	"\[\r\n\]"
+	"\[\r\n\]3       dprintf"
+	"\[\r\n\]\tstop only if x > 5"
+	"\[\r\n\]\tbreakpoint already hit 1 time"
+	"\[\r\n\]"
+    }
+}}
+
+test_dprintf "gdb"
+
+if ![target_info exists gdb,noinferiorio] {
+    test_dprintf "call"
+}
-- 
1.7.7.6

[PATCH 3/4] Test dprintf breakpoint works correctly with other breakpoints

[Yao Qi]

gdb/testsuite:

2013-02-28  Yao Qi  <yao@codesourcery.com>

	* gdb.base/dprintf.exp: Test dprintf breakpoints works correctly
	with other breakpoints setting on the same address.
---
 gdb/testsuite/gdb.base/dprintf.exp |   49 ++++++++++++++++++++++++++++++-----
 1 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/gdb.base/dprintf.exp b/gdb/testsuite/gdb.base/dprintf.exp
index ffbd3f2..22c740a 100644
--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -40,8 +40,11 @@ gdb_breakpoint "main"
 gdb_test "dprintf foo,\"At foo entry\\n\"" \
   "Dprintf .*"
 
+# Set two dprintf breakpoints at the same location.
 gdb_test "dprintf $dp_location1,\"arg=%d, g=%d\\n\", arg, g" \
-  "Dprintf .*"
+    "Dprintf .*"
+gdb_test "dprintf $dp_location1,\"g=%d, arg=%d\\n\", g, arg" \
+    "Dprintf .*"
 
 gdb_test_sequence "info breakpoints" "dprintf info 1" {
     "\[\r\n\]Num     Type           Disp Enb Address +What"
@@ -50,6 +53,8 @@ gdb_test_sequence "info breakpoints" "dprintf info 1" {
     "\[\r\n\]        printf \"At foo entry\\\\n\""
     "\[\r\n\]4       dprintf"
     "\[\r\n\]        printf \"arg=%d, g=%d\\\\n\", arg, g"
+    "\[\r\n\]5       dprintf"
+    "\[\r\n\]        printf \"g=%d, arg=%d\\\\n\", g, arg"
 }
 
 gdb_test "break $bp_location1" \
@@ -76,22 +81,52 @@ if ![target_info exists gdb,noinferiorio] {
 
     gdb_test "" "Breakpoint"
 
-    gdb_test "continue" "At foo entry.*arg=1234, g=1234.*" "1st dprintf, call"
+    # Test two dprintf breakpoints on the same location are executed.
+    gdb_test "continue" \
+	"At foo entry.*arg=1234, g=1234\\r\\n.*g=1234, arg=1234\\r\\n.*" \
+	"1st dprintf, call"
 
-    gdb_test "continue" "At foo entry.*arg=1235, g=2222.*" "2nd dprintf, call"
+    gdb_test "continue" \
+	"At foo entry.*arg=1235, g=2222\\r\\n.*g=2222, arg=1235\\r\\n.*" \
+	"2nd dprintf, call"
 
     gdb_test_no_output "set dprintf-function fprintf" "Set dprintf function"
     gdb_test_no_output "set dprintf-channel stderr" "Set dprintf channel"
 
+    # Set breakpoint on foo, and test dprintf set at the same address
+    # behaves correctly.
+    gdb_test "break foo" "Note: breakpoint ${decimal} also set at pc ${hex}.*"
+    gdb_test_no_output "set \$foo_breakpoint_number = \$bpnum"
+    # Hit the breakpoint on foo, and the command of dprintf
+    # breakpoint on foo is executed as well.
+    gdb_test "continue" \
+	"Continuing\.\\r\\nAt foo entry\\r.*Breakpoint .*" \
+	"continue"
+    gdb_test "delete \$foo_breakpoint_number"
+
     gdb_run_cmd
 
     gdb_test "" "Breakpoint"
 
-    gdb_test "continue" "At foo entry.*arg=1234, g=1234.*" \
+    gdb_test "continue" \
+	"At foo entry.*arg=1234, g=1234\\r\\n.*g=1234, arg=1234\\r\\n.*" \
 	"1st dprintf, fprintf"
 
-    gdb_test "continue" "At foo entry.*arg=1235, g=2222.*" \
+    gdb_test "continue" \
+	"At foo entry.*arg=1235, g=2222\\r\\n.*g=2222, arg=1235\\r\\n.*" \
 	"2nd dprintf, fprintf"
+
+    gdb_test_sequence "info breakpoints" "dprintf info 2" {
+	"\[\r\n\]Num     Type           Disp Enb Address +What"
+	"\[\r\n\]2       breakpoint"
+	"\[\r\n\]\tbreakpoint already hit 1 time"
+	"\[\r\n\]3       dprintf"
+	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]        call \\(void\\\) fprintf \\(stderr,\"At foo entry\\\\n\"\\)"
+	"\[\r\n\]4       dprintf"
+	"\[\r\n\]\tbreakpoint already hit 2 times"
+	"\[\r\n\]        call \\(void\\) fprintf \\(stderr,\"arg=%d, g=%d\\\\n\", arg, g\\)"
+    }
 }
 
 set target_can_dprintf 1
@@ -116,8 +151,8 @@ if $target_can_dprintf {
 
     gdb_test "continue" "Breakpoint \[0-9\]+, foo .*" "2nd dprintf, agent"
 
-    gdb_test_sequence "info breakpoints" "dprintf info 2" {
-	"\[\r\n\]Num     Type           Disp Enb Address    What"
+    gdb_test_sequence "info breakpoints" "dprintf info 3" {
+	"\[\r\n\]Num     Type           Disp Enb Address +What"
 	"\[\r\n\]2       breakpoint"
 	"\[\r\n\]\tbreakpoint already hit 2 times"
 	"\[\r\n\]3       dprintf"
-- 
1.7.7.6

[PATCH 4/4] Test case on setting dprintf commands.

[Yao Qi]

Test that GDB doesn't allow setting commands for dprintf breakpoint.

gdb/testsuite:

2013-02-28  Yao Qi  <yao@codesourcery.com>

	* gdb.base/dprintf.exp: Test commands are not allowed to set
	for dprintf breakpoint.
---
 gdb/testsuite/gdb.base/dprintf.exp |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/gdb/testsuite/gdb.base/dprintf.exp b/gdb/testsuite/gdb.base/dprintf.exp
index 22c740a..aa0108d 100644
--- a/gdb/testsuite/gdb.base/dprintf.exp
+++ b/gdb/testsuite/gdb.base/dprintf.exp
@@ -40,6 +40,20 @@ gdb_breakpoint "main"
 gdb_test "dprintf foo,\"At foo entry\\n\"" \
   "Dprintf .*"
 
+# Test that GDB doesn't allow setting commands for dprintf.
+set test "set commands for dprintf"
+gdb_test_multiple "commands \$bpnum" "${test}" {
+    -re "End with a line saying just \"end\".*>" {
+	send_gdb "silent\n"
+	send_gdb "end\n"
+
+	fail $test
+    }
+    -re "Setting commands for dprintf breakpoint is not allowed.*\r\n$gdb_prompt $" {
+	pass $test
+    }
+}
+
 # Set two dprintf breakpoints at the same location.
 gdb_test "dprintf $dp_location1,\"arg=%d, g=%d\\n\", arg, g" \
     "Dprintf .*"
-- 
1.7.7.6

Re: [PATCH 1/4] Fix dprintf bugs

[Eli Zaretskii]

> From: Yao Qi <yao@codesourcery.com>
> Date: Thu, 28 Feb 2013 21:15:59 +0800
> 
> This patch fixes PR15075.  Besides this bug, this patch also fixes
> some other problems:
> 
>   - Two dprintf are set on the same address.  Only one printf is
>     executed.
>   - dprintf and regular breakpoint are set on the same address,
>     inferior doesn't stop.
> 
> In order to fix these problems, I don't append "continue" command to
> dprintf breakpoint, and execute commands of dprintf after
> bpstat_check_breakpoint_conditions if condition is true.  The reason I
> choose this place (after bpstat_check_breakpoint_conditions instead of
> in bpstat_check_breakpoint_conditions) to handle dprintf is that we
> have to set BS->stop to zero, but have to update hit count if
> condition is true, so we have to do two things together.
> 
> With this patch, GDB executes dprintf commands after
> bpstat_check_breakpoint_conditions, the different place where
> breakpoint commands are executed.  I propose to disable setting
> command for dprintf, or disallow user setting commands for dprintf.

I don't understand: if you are disallowing commands for dprintf, then
why do you also fix the handling of these commands?

Re: [PATCH 1/4] Fix dprintf bugs

[Yao Qi]

On 02/28/2013 11:54 PM, Eli Zaretskii wrote:
> I don't understand: if you are disallowing commands for dprintf, then
> why do you also fix the handling of these commands?

Internally, dprintf breakpoint still uses 'print' command, but we 
disallow users setting commands for it in CLI and MI.

-- 
Yao (齐尧)

RE: [PATCH 1/4] Fix dprintf bugs

[Marc Khouzam]

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org 
> [mailto:gdb-patches-owner@sourceware.org] On Behalf Of Yao Qi
> Sent: Thursday, February 28, 2013 8:16 AM
> To: gdb-patches@sourceware.org
> Subject: [PATCH 1/4] Fix dprintf bugs
> 
> Hi,
> This patch fixes PR15075.  Besides this bug, this patch also fixes
> some other problems:
> 
>   - Two dprintf are set on the same address.  Only one printf is
>     executed.
>   - dprintf and regular breakpoint are set on the same address,
>     inferior doesn't stop.
> 
> In order to fix these problems, I don't append "continue" command to
> dprintf breakpoint, and execute commands of dprintf after
> bpstat_check_breakpoint_conditions if condition is true.  The reason I
> choose this place (after bpstat_check_breakpoint_conditions instead of
> in bpstat_check_breakpoint_conditions) to handle dprintf is that we
> have to set BS->stop to zero, but have to update hit count if
> condition is true, so we have to do two things together.
> 
> With this patch, GDB executes dprintf commands after
> bpstat_check_breakpoint_conditions, the different place where
> breakpoint commands are executed.  I propose to disable setting
> command for dprintf, or disallow user setting commands for dprintf.
> If we believe it is important to allow user setting commands for
> dprintf (a good use case?), we need a new design like creating struct
> dprintf as a 'sub-class' of breakpoint and adding its own field for
> printf commands.  The code on sending commands to the remote should be
> adjusted as well.

I've played around with this patch and I like the solution a lot!

I like the fact that a user cannot modify the list of commands for dprintf.
If they want to use commands, they should use a breakpoint, not
a dprintf.  It is no longer possible to update the dprintf string
without having to create a new dprintf though; but if this is to be
supported, I think it should be a specific dprintf operation.

I no longer see *stopped/*running events in MI, which is great.

I'm still hesitant about the -break-modified event in that case
though.  I believe the event is triggered because the hit count 
has changed.  For a normal bp, it makes sense to have this event
in this case, since execution has stopped and only a single 
event will be seen (not exactly true for non-stop, but still
makes sense, I think).  However, for dprintf which is meant to 
let the inferior continue to run, there could be quite many 
hit events very quickly.  Since we already have some feeback
that the dprintf has hit through the actual printf string, I'm
leaning towards not having that event for dprintf hits.
Furthermore, this event is not being sent when using dprintf-style
"agent" anyway.

I also saw that conditions are now properly respected for dprintf-style
"gdb" and "call".  That is great.  Conditions are still not respected for 
style "agent" but that is a separate issue I believe (PR 15180).

I did notice that although commands cannot be set for dprintf from
the CLI they are not blocked for MI:

(gdb) interpreter-exec mi "-break-commands 1 hello"
^done
(gdb) info b
Num     Type           Disp Enb Address            What
1       dprintf        keep y   0x0000000000400570 in main() at loopfirst.cc:8
        hello

Finally, it would be nice to have MI commands to deal with dprintf :)
Re-using the breakpoint MI commands sound best, although I wonder about
two cases:
a) Would we be able to specify the printf string in the -break-insert command?
b) How could we change the printf command after a dprintf is created?  Will we
   need to delete it and create a new one, or would we be able to update it?

All that being said, I'm impressed at how many improvements this
relatively small patch provides.  Nice work.

Marc

Re: [PATCH 1/4] Fix dprintf bugs

[Yao Qi]

Marc, thanks for playing with dprintf and giving a lot useful comments 
and suggestions.


On 03/03/2013 10:21 AM, Marc Khouzam wrote:
> I'm still hesitant about the -break-modified event in that case
> though.  I believe the event is triggered because the hit count
> has changed.  For a normal bp, it makes sense to have this event
> in this case, since execution has stopped and only a single
> event will be seen (not exactly true for non-stop, but still
> makes sense, I think).  However, for dprintf which is meant to
> let the inferior continue to run, there could be quite many
> hit events very quickly.  Since we already have some feeback
> that the dprintf has hit through the actual printf string, I'm
> leaning towards not having that event for dprintf hits.

Right, the "hit count" is not very meaningful to dprintf.  I am fine not 
to update hit count for dprintf.

> Furthermore, this event is not being sent when using dprintf-style
> "agent" anyway.
>
> I also saw that conditions are now properly respected for dprintf-style
> "gdb" and "call".  That is great.  Conditions are still not respected for
> style "agent" but that is a separate issue I believe (PR 15180).
>

Right, PR 15180 is a separate problem.  It is not dprintf specific, in 
fact.  It is about target side commands execution respected for the 
target side conditions.

> I did notice that although commands cannot be set for dprintf from
> the CLI they are not blocked for MI:
>
> (gdb) interpreter-exec mi "-break-commands 1 hello"
> ^done
> (gdb) info b
> Num     Type           Disp Enb Address            What
> 1       dprintf        keep y   0x0000000000400570 in main() at loopfirst.cc:8
>          hello
>

I'll handle this in the new version.

-- 
Yao (齐尧)

RE: [PATCH 1/4] Fix dprintf bugs

[Marc Khouzam]

From: Yao Qi [yao@codesourcery.com]
Sent: March 7, 2013 1:46 AM
> 
> On 03/03/2013 10:21 AM, Marc Khouzam wrote:
> > I'm still hesitant about the -break-modified event in that case
> > though.  I believe the event is triggered because the hit count
> > has changed.  For a normal bp, it makes sense to have this event
> > in this case, since execution has stopped and only a single
> > event will be seen (not exactly true for non-stop, but still
> > makes sense, I think).  However, for dprintf which is meant to
> > let the inferior continue to run, there could be quite many
> > hit events very quickly.  Since we already have some feeback
> > that the dprintf has hit through the actual printf string, I'm
> > leaning towards not having that event for dprintf hits.
> 
> Right, the "hit count" is not very meaningful to dprintf.  I am fine not
> to update hit count for dprintf.

I was actually thinking of keeping the hit count being updated but
simply not sending the MI event for it.  I'm under the impression
that updating the hit count is not expensive, so having that information
available can be useful.  What bothers me is the very many MI
events being sent to the frontend.  If the hit count is not reported
by an MI event for dprintf, its value can still be obtained by
the frontend through -break-list for example.

For the agent-style dprintf though, I'm guessing that the hit count
is not being updated to avoid communication with GDB?  This
scenario already had to be handled for normal breakpoints and 
I suggest using whatever scheme is already being used.

Thanks for the nice improvements

Marc

Re: [PATCH 1/4] Fix dprintf bugs

[Yao Qi]

On 03/07/2013 10:05 PM, Marc Khouzam wrote:
> I was actually thinking of keeping the hit count being updated but
> simply not sending the MI event for it.  I'm under the impression
> that updating the hit count is not expensive, so having that information
> available can be useful.  What bothers me is the very many MI
> events being sent to the frontend.  If the hit count is not reported
> by an MI event for dprintf, its value can still be obtained by
> the frontend through -break-list for example.

FAOD, are you suggesting that we still update hit count, but don't have 
to notify breakpoint-modified observer?

>
> For the agent-style dprintf though, I'm guessing that the hit count
> is not being updated to avoid communication with GDB?  This
> scenario already had to be handled for normal breakpoints and
> I suggest using whatever scheme is already being used.

We can use async remote notification here, to notify GDB that the hit 
count or other attributes of breakpoint/tracepoint are modified.  I've 
had some local patches to report to GDB that the hit count and 
traceframe usage of tracepoint is updated.  After pending async remote 
notification are patches approved, I'll post them out.

-- 
Yao (齐尧)

RE: [PATCH 1/4] Fix dprintf bugs

[Marc Khouzam]

> On 03/07/2013 10:05 PM, Marc Khouzam wrote:
> > I was actually thinking of keeping the hit count being updated but
> > simply not sending the MI event for it.  I'm under the impression
> > that updating the hit count is not expensive, so having that information
> > available can be useful.  What bothers me is the very many MI
> > events being sent to the frontend.  If the hit count is not reported
> > by an MI event for dprintf, its value can still be obtained by
> > the frontend through -break-list for example.
> 
> FAOD, are you suggesting that we still update hit count, but don't have
> to notify breakpoint-modified observer?

That is right.  Of course, I'm just making a suggestion.  I don't know
GDB well enough to know all the implications, but from a frontend 
point-of-view I believe this solution is best.

> > For the agent-style dprintf though, I'm guessing that the hit count
> > is not being updated to avoid communication with GDB?  This
> > scenario already had to be handled for normal breakpoints and
> > I suggest using whatever scheme is already being used.
>
> We can use async remote notification here, to notify GDB that the hit
> count or other attributes of breakpoint/tracepoint are modified.  I've
> had some local patches to report to GDB that the hit count and
> traceframe usage of tracepoint is updated.  After pending async remote
> notification are patches approved, I'll post them out.

So, I'm suggesting that sending an MI event from GDB to the frontend for
every dprintf hit is too much.  Does the same argument hold for async
remote notifications?  For efficiency, I'm thinking that agent dprintf should
not report every hit to GDB; instead, when GDB wants to know the hit
count (e.g, because of a -break-list command), it would ask the agent
for the current hit count.  This would cut down on the communication
from agent to GDB when using dprintf.

Does that make sense?

Marc

Re: [PATCH 1/4] Fix dprintf bugs

[Yao Qi]

On 03/07/2013 10:48 PM, Marc Khouzam wrote:
> So, I'm suggesting that sending an MI event from GDB to the frontend for
> every dprintf hit is too much.  Does the same argument hold for async
> remote notifications?  For efficiency, I'm thinking that agent dprintf should
> not report every hit to GDB; instead, when GDB wants to know the hit
> count (e.g, because of a -break-list command), it would ask the agent
> for the current hit count.  This would cut down on the communication
> from agent to GDB when using dprintf.

If we use async remote notification for hit count update, of course, 
GDBserver can't report *every* hit count update to GDB, which is 
relatively expensive.  GDBserver shouldn't send new async remote 
notification to GDB until the previous notification is ack'ed by GDB.

-- 
Yao (齐尧)

RE: [PATCH 1/4] Fix dprintf bugs

[Marc Khouzam]

> -----Original Message-----
> From: Yao Qi [mailto:yao@codesourcery.com] 
> Sent: Thursday, March 07, 2013 9:58 AM
> To: Marc Khouzam
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH 1/4] Fix dprintf bugs
> 
> On 03/07/2013 10:48 PM, Marc Khouzam wrote:
> > So, I'm suggesting that sending an MI event from GDB to the 
> frontend for
> > every dprintf hit is too much.  Does the same argument hold 
> for async
> > remote notifications?  For efficiency, I'm thinking that 
> agent dprintf should
> > not report every hit to GDB; instead, when GDB wants to know the hit
> > count (e.g, because of a -break-list command), it would ask 
> the agent
> > for the current hit count.  This would cut down on the communication
> > from agent to GDB when using dprintf.
> 
> If we use async remote notification for hit count update, of course, 
> GDBserver can't report *every* hit count update to GDB, which is 
> relatively expensive.  GDBserver shouldn't send new async remote 
> notification to GDB until the previous notification is ack'ed by GDB.

Again, I don't know the details of this communication so I might
be missing an important point.  Sending a new async notification
for a hit count change for dprintf, even if some of those will be
skipped when waiting for a GDB ack, sounds like a lot to me.
If I set a dprintf into a loop, there will be many printouts on
the target, but do we want to have so many async notifs sent
to GDB from the target?  Since we won't be sending MI events for
hit count changes for dprintf, do we need GDB to know dprintf
hit count 'live' or can we ask the target whenever that information
is actually needed?

Marc