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

[Hui Zhu <teawater@gmail.com>]

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