Re: [1/2] RFC: reference count breakpoint commands

[Tom Tromey <tromey at redhat dot com>]

>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes:

Tom> This adds reference counting for breakpoint commands.  This removes a
Tom> longstanding restriction: you can now change a breakpoint's commands
Tom> while the commands are running.  This patch also sets the stage for the
Tom> next page.

I am checking this in.  I rebased this patch on top of Volodya's recent
commits.  The updated patch is appended.

Tom

2010-03-24  Tom Tromey  <tromey@redhat.com>

	* breakpoint.h (struct counted_command_line): New struct.
	(struct breakpoint) <commands>: Change type.
	(struct bpstats) <commands>: Change type.
	<commands_left>: New field.
	* breakpoint.c (alloc_counted_command_line): New function.
	(incref_counted_command_line): Likewise.
	(decref_counted_command_line): Likewise.
	(do_cleanup_counted_command_line): Likewise.
	(make_cleanup_decref_counted_command_line): Likewise.
	(breakpoint_set_commands): Use decref_counted_command_line and
	alloc_counted_command_line.
	(commands_command): Don't error if breakpoint commands are
	executing.
	(commands_from_control_command): Likewise.
	(bpstat_free): Update.
	(bpstat_copy): Likewise.
	(bpstat_clear_actions): Likewise.
	(bpstat_do_actions_1): Likewise.
	(bpstat_stop_status): Likewise.
	(print_one_breakpoint_location): Likewise.
	(delete_breakpoint): Likewise.
	(bpstat_alloc): Initialize new field.
	(tracepoint_save_command): Update.
	* tracepoint.c (encode_actions): Update.
	(trace_dump_command): Update.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.465
diff -u -r1.465 breakpoint.c
--- breakpoint.c	23 Mar 2010 21:36:05 -0000	1.465
+++ breakpoint.c	24 Mar 2010 21:06:51 -0000
@@ -433,6 +433,63 @@
   return (b->type == bp_tracepoint || b->type == bp_fast_tracepoint);
 }
   
+/* Allocate a new counted_command_line with reference count of 1.
+   The new structure owns COMMANDS.  */
+
+static struct counted_command_line *
+alloc_counted_command_line (struct command_line *commands)
+{
+  struct counted_command_line *result
+    = xmalloc (sizeof (struct counted_command_line));
+  result->refc = 1;
+  result->commands = commands;
+  return result;
+}
+
+/* Increment reference count.  This does nothing if CMD is NULL.  */
+
+static void
+incref_counted_command_line (struct counted_command_line *cmd)
+{
+  if (cmd)
+    ++cmd->refc;
+}
+
+/* Decrement reference count.  If the reference count reaches 0,
+   destroy the counted_command_line.  Sets *CMDP to NULL.  This does
+   nothing if *CMDP is NULL.  */
+
+static void
+decref_counted_command_line (struct counted_command_line **cmdp)
+{
+  if (*cmdp)
+    {
+      if (--(*cmdp)->refc == 0)
+	{
+	  free_command_lines (&(*cmdp)->commands);
+	  xfree (*cmdp);
+	}
+      *cmdp = NULL;
+    }
+}
+
+/* A cleanup function that calls decref_counted_command_line.  */
+
+static void
+do_cleanup_counted_command_line (void *arg)
+{
+  decref_counted_command_line (arg);
+}
+
+/* Create a cleanup that calls decref_counted_command_line on the
+   argument.  */
+
+static struct cleanup *
+make_cleanup_decref_counted_command_line (struct counted_command_line **cmdp)
+{
+  return make_cleanup (do_cleanup_counted_command_line, cmdp);
+}
+
 /* Default address, symtab and line to put a breakpoint at
    for "break" command with no arg.
    if default_breakpoint_valid is zero, the other three are
@@ -783,8 +840,8 @@
       check_no_tracepoint_commands (commands);
     }
 
-  free_command_lines (&b->commands);
-  b->commands = commands;
+  decref_counted_command_line (&b->commands);
+  b->commands = alloc_counted_command_line (commands);
   breakpoints_changed ();
   observer_notify_breakpoint_modified (b->number);
 }
@@ -803,13 +860,6 @@
   int bnum;
   struct command_line *l;
 
-  /* If we allowed this, we would have problems with when to
-     free the storage, if we change the commands currently
-     being read from.  */
-
-  if (executing_breakpoint_commands)
-    error (_("Can't use the \"commands\" command among a breakpoint's commands."));
-
   p = arg;
   bnum = get_number (&p);
 
@@ -847,13 +897,6 @@
   char *p;
   int bnum;
 
-  /* If we allowed this, we would have problems with when to
-     free the storage, if we change the commands currently
-     being read from.  */
-
-  if (executing_breakpoint_commands)
-    error (_("Can't use the \"commands\" command among a breakpoint's commands."));
-
   /* An empty string for the breakpoint number means the last
      breakpoint, but get_number expects a NULL pointer.  */
   if (arg && !*arg)
@@ -868,12 +911,13 @@
   ALL_BREAKPOINTS (b)
     if (b->number == bnum)
       {
-	free_command_lines (&b->commands);
+	decref_counted_command_line (&b->commands);
 	if (cmd->body_count != 1)
 	  error (_("Invalid \"commands\" block structure."));
 	/* We need to copy the commands because if/while will free the
 	   list after it finishes execution.  */
-	b->commands = copy_command_lines (cmd->body_list[0]);
+	b->commands
+	  = alloc_counted_command_line (copy_command_lines (cmd->body_list[0]));
 	breakpoints_changed ();
 	observer_notify_breakpoint_modified (b->number);
 	return simple_control;
@@ -2697,7 +2741,7 @@
 {
   if (bs->old_val != NULL)
     value_free (bs->old_val);
-  free_command_lines (&bs->commands);
+  decref_counted_command_line (&bs->commands);
   xfree (bs);
 }
 
@@ -2739,8 +2783,7 @@
     {
       tmp = (bpstat) xmalloc (sizeof (*tmp));
       memcpy (tmp, bs, sizeof (*tmp));
-      if (bs->commands != NULL)
-	tmp->commands = copy_command_lines (bs->commands);
+      incref_counted_command_line (tmp->commands);
       if (bs->old_val != NULL)
 	{
 	  tmp->old_val = value_copy (bs->old_val);
@@ -2841,7 +2884,7 @@
 {
   for (; bs != NULL; bs = bs->next)
     {
-      free_command_lines (&bs->commands);
+      decref_counted_command_line (&bs->commands);
       if (bs->old_val != NULL)
 	{
 	  value_free (bs->old_val);
@@ -2907,6 +2950,7 @@
   breakpoint_proceeded = 0;
   for (; bs != NULL; bs = bs->next)
     {
+      struct counted_command_line *ccmd;
       struct command_line *cmd;
       struct cleanup *this_cmd_tree_chain;
 
@@ -2920,9 +2964,12 @@
          commands are only executed once, we don't need to copy it; we
          can clear the pointer in the bpstat, and make sure we free
          the tree when we're done.  */
-      cmd = bs->commands;
-      bs->commands = 0;
-      this_cmd_tree_chain = make_cleanup_free_command_lines (&cmd);
+      ccmd = bs->commands;
+      bs->commands = NULL;
+      this_cmd_tree_chain
+	= make_cleanup_decref_counted_command_line (&ccmd);
+      cmd = bs->commands_left;
+      bs->commands_left = NULL;
 
       while (cmd != NULL)
 	{
@@ -3305,6 +3352,7 @@
   bs->breakpoint_at = bl;
   /* If the condition is false, etc., don't do the commands.  */
   bs->commands = NULL;
+  bs->commands_left = NULL;
   bs->old_val = NULL;
   bs->print_it = print_it_normal;
   return bs;
@@ -3942,15 +3990,17 @@
 	      if (b->silent)
 		bs->print = 0;
 	      bs->commands = b->commands;
-	      if (bs->commands
-		  && (strcmp ("silent", bs->commands->line) == 0
-		      || (xdb_commands && strcmp ("Q",
-						  bs->commands->line) == 0)))
+	      incref_counted_command_line (bs->commands);
+	      bs->commands_left = bs->commands ? bs->commands->commands : NULL;
+	      if (bs->commands_left
+		  && (strcmp ("silent", bs->commands_left->line) == 0
+		      || (xdb_commands
+			  && strcmp ("Q",
+				     bs->commands_left->line) == 0)))
 		{
-		  bs->commands = bs->commands->next;
+		  bs->commands_left = bs->commands_left->next;
 		  bs->print = 0;
 		}
-	      bs->commands = copy_command_lines (bs->commands);
 	    }
 
 	  /* Print nothing for this entry if we dont stop or dont print.  */
@@ -4617,7 +4667,7 @@
       ui_out_text (uiout, " hits\n");
     }
 
-  l = b->commands;
+  l = b->commands ? b->commands->commands : NULL;
   if (!part_of_multiple && l)
     {
       struct cleanup *script_chain;
@@ -9021,7 +9071,7 @@
       break;
     }
 
-  free_command_lines (&bpt->commands);
+  decref_counted_command_line (&bpt->commands);
   xfree (bpt->cond_string);
   xfree (bpt->cond_exp);
   xfree (bpt->addr_string);
@@ -10446,7 +10496,7 @@
 	ui_out_redirect (uiout, fp);
 	TRY_CATCH (ex, RETURN_MASK_ERROR)
 	  {
-	    print_command_lines (uiout, tp->commands, 2);
+	    print_command_lines (uiout, tp->commands->commands, 2);
 	  }
 	ui_out_redirect (uiout, NULL);
 
Index: breakpoint.h
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.h,v
retrieving revision 1.110
diff -u -r1.110 breakpoint.h
--- breakpoint.h	23 Mar 2010 21:32:26 -0000	1.110
+++ breakpoint.h	24 Mar 2010 21:06:51 -0000
@@ -376,6 +376,17 @@
 typedef struct bp_location *bp_location_p;
 DEF_VEC_P(bp_location_p);
 
+/* A reference-counted struct command_line.  This lets multiple
+   breakpoints share a single command list.  */
+struct counted_command_line
+{
+  /* The reference count.  */
+  int refc;
+
+  /* The command list.  */
+  struct command_line *commands;
+};
+
 /* Note that the ->silent field is not currently used by any commands
    (though the code is in there if it was to be, and set_raw_breakpoint
    does set it to 0).  I implemented it because I thought it would be
@@ -414,7 +425,7 @@
        be continued automatically before really stopping.  */
     int ignore_count;
     /* Chain of command lines to execute when this breakpoint is hit.  */
-    struct command_line *commands;
+    struct counted_command_line *commands;
     /* Stack depth (address of frame).  If nonzero, break only if fp
        equals this.  */
     struct frame_id frame_id;
@@ -698,8 +709,11 @@
     bpstat next;
     /* Breakpoint that we are at.  */
     const struct bp_location *breakpoint_at;
-    /* Commands left to be done.  */
-    struct command_line *commands;
+    /* The associated command list.  */
+    struct counted_command_line *commands;
+    /* Commands left to be done.  This points somewhere in
+       base_command.  */
+    struct command_line *commands_left;
     /* Old value associated with a watchpoint.  */
     struct value *old_val;
 
Index: tracepoint.c
===================================================================
RCS file: /cvs/src/src/gdb/tracepoint.c,v
retrieving revision 1.154
diff -u -r1.154 tracepoint.c
--- tracepoint.c	24 Mar 2010 19:37:06 -0000	1.154
+++ tracepoint.c	24 Mar 2010 21:06:55 -0000
@@ -1385,7 +1385,7 @@
   gdbarch_virtual_frame_pointer (t->gdbarch,
 				 t->loc->address, &frame_reg, &frame_offset);
 
-  actions = t->commands;
+  actions = t->commands->commands;
 
   /* If there are default expressions to collect, make up a collect
      action and prepend to the action list to encode.  Note that since
@@ -1406,7 +1406,7 @@
 	{
 	  default_collect_action = xmalloc (sizeof (struct command_line));
 	  make_cleanup (xfree, default_collect_action);
-	  default_collect_action->next = t->commands;
+	  default_collect_action->next = t->commands->commands;
 	  default_collect_action->line = line;
 	  actions = default_collect_action;
 	}
@@ -2332,7 +2332,7 @@
     if (loc->address == regcache_read_pc (regcache))
       stepping_frame = 0;
 
-  for (action = t->commands; action; action = action->next)
+  for (action = t->commands->commands; action; action = action->next)
     {
       struct cmd_list_element *cmd;