[RFA] deleting breakpoints inside of 'commands' (again)

[RFA] deleting breakpoints inside of 'commands' (again)

[Don Howard]

This implementation makes a copy of commands list that contain 'delete'
or 'clear' commands.  It also attempts to clean up when an error in
encountered.  Comments?



2001-10-31  Don Howard  <dhoward@redhat.com>

        * breakpoint.c (bpstat_do_actions): Avoid deleting a 'commands'
        list while executing that list.  Thanks to Pierre Muller
        <muller@ics.u-strasbg.fr> for suggesting this implementation.
	(cleanup_dup_command_lines): New function.
        * cli/cli-script.c (dup_command_lines): New function.
        * defs.h: Added declaration of new function.


Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.55
diff -p -u -w -r1.55 breakpoint.c
--- breakpoint.c	2001/10/20 23:54:29	1.55
+++ breakpoint.c	2001/10/31 21:48:28
@@ -1790,6 +1790,12 @@ cleanup_executing_breakpoints (PTR ignor
   executing_breakpoint_commands = 0;
 }

+static void
+cleanup_dup_command_lines (PTR cmds)
+{
+  free_command_lines (cmds);
+}
+
 /* 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
@@ -1800,7 +1806,6 @@ bpstat_do_actions (bpstat *bsp)
 {
   bpstat bs;
   struct cleanup *old_chain;
-  struct command_line *cmd;

   /* Avoid endless recursion if a `source' command is contained
      in bs->commands.  */
@@ -1824,17 +1829,40 @@ top:

   breakpoint_proceeded = 0;
   for (; bs != NULL; bs = bs->next)
+    {
+      struct command_line *cmd;
+      struct cleanup *new_old_chain;
+
+      cmd = 0;
+      new_old_chain = 0;
+
+      /* If the command list for this breakpoint includes a statement
+	 that deletes breakpoints, we assume that the target may be
+	 this breakpoint, so we make a copy of the command list to
+	 avoid walking a list that has been deleted. */
+
+      for (cmd = bs->commands; cmd; cmd = cmd->next)
     {
-      cmd = bs->commands;
-      while (cmd != NULL)
+	  if (!new_old_chain &&
+	      (strncmp (cmd->line, "dele", 4) == 0 ||
+	       strncmp (cmd->line, "clea", 4) == 0))
 	{
+	      cmd = dup_command_lines (cmd);
+	      new_old_chain = make_cleanup (cleanup_dup_command_lines, cmd);
+	    }
+
 	  execute_control_command (cmd);

 	  if (breakpoint_proceeded)
 	    break;
-	  else
-	    cmd = cmd->next;
 	}
+
+      if (new_old_chain)
+	{
+	  free_command_lines (&cmd);
+	  discard_cleanups (new_old_chain);
+	}
+
       if (breakpoint_proceeded)
 	/* The inferior is proceeded by the command; bomb out now.
 	   The bpstat chain has been blown away by wait_for_inferior.
@@ -1844,6 +1872,7 @@ top:
       else
 	bs->commands = NULL;
     }
+

   executing_breakpoint_commands = 0;
   discard_cleanups (old_chain);
Index: defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.64
diff -p -u -w -r1.64 defs.h
--- defs.h	2001/10/17 20:35:31	1.64
+++ defs.h	2001/10/31 21:48:28
@@ -843,6 +843,7 @@ struct command_line
 extern struct command_line *read_command_lines (char *, int);

 extern void free_command_lines (struct command_line **);
+extern struct command_line * dup_command_lines (struct command_line *);

 /* To continue the execution commands when running gdb asynchronously.
    A continuation structure contains a pointer to a function to be called
Index: cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.7
diff -p -u -w -r1.7 cli-script.c
--- cli-script.c	2001/06/17 15:16:12	1.7
+++ cli-script.c	2001/10/31 21:48:29
@@ -1005,6 +1005,59 @@ read_command_lines (char *prompt_arg, in
   return (head);
 }

+/* Duplicate a chain of struct command_line's */
+
+struct command_line *
+dup_command_lines (struct command_line *l)
+{
+  struct command_line *dup = NULL;
+  register struct command_line *next = NULL;
+
+
+  for (; l ; l = l->next)
+    {
+      if (next == NULL)
+       {
+         dup = next = (struct command_line *)
+           xmalloc (sizeof (struct command_line));
+       }
+      else
+       {
+         next->next = (struct command_line *)
+           xmalloc (sizeof (struct command_line));
+
+         next = next->next;
+       }
+
+
+      if (next == NULL)
+       return NULL;
+
+
+      next->next = NULL;
+      next->line = xstrdup (l->line);
+      next->control_type = l->control_type;
+      next->body_count = l->body_count;
+
+
+      if (l->body_count > 0)
+       {
+         int i;
+         struct command_line **blist = l->body_list;
+
+         next->body_list =
+           (struct command_line **) xmalloc (sizeof (struct command_line *)
+                                             * l->body_count);
+
+         for (i = 0; i < l->body_count; i++, blist++)
+           next->body_list[i] = dup_command_lines (*blist);
+       }
+    }
+
+  return dup;
+}
+
+
 /* Free a chain of struct command_line's.  */

 void

-- 
-Don
dhoward@redhat.com
gdb engineering

Re: [RFA] deleting breakpoints inside of 'commands' (again)

[Don Howard]

[sorry about taking so long to get back to this]


On Tue, 6 Nov 2001, Andrew Cagney wrote:

> > 
> > Doesn't the function below catch that?  There is a recursive call to
> > look for just the problem that you mention.  Is there some other way of
> > deleting commands that I need to look out for?
> 
> I'm not sure.
> 
> Consider:
> 
> 	command callee
>            delete command callee
> 	end
> 	breakpoint foo
> 	commands $bp
> 	  callee
> 	end
> 
> it will realise that there is a delete but will it protect its self from 
> a command deleting its self.
> 
> > [BTW, I personally think that the unconditional copy is much less
> > error prone (and therefore better) even if it's slower. Just my $0.02]
> 
> Agreed (And slower is relative - what happens if there are 100's or 
> 1000's of commands - that search becomes the bottle neck.)
> 
> Andrew




I've added a call to lookup_cmd().  This allows user-defined commands to 
be examined also, which should address your concern.

I've also added checks that examine the pre and post hook commands.



2001-12-13  Don Howard  <dhoward@redhat.com>
 
        * breakpoint.c (bpstat_do_actions): Avoid deleting a 'commands'
        list while executing that list.  Thanks to Pierre Muller
        <muller@ics.u-strasbg.fr> for suggesting this implementation.
        (cleanup_dup_command_lines): New function.
        (bpstat_actions_delete_breakpoints): Ditto.
        * cli/cli-script.c (dup_command_lines): New function.
        * defs.h: Added declaration of new function.


Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.57
diff -p -u -w -r1.57 breakpoint.c
--- breakpoint.c	2001/11/11 16:39:59	1.57
+++ breakpoint.c	2001/12/14 08:13:07
@@ -1790,6 +1790,102 @@ cleanup_executing_breakpoints (PTR ignor
   executing_breakpoint_commands = 0;
 }
 
+static void
+cleanup_dup_command_lines (PTR cmds)
+{
+  free_command_lines (cmds);
+}
+
+
+/* Walk a list of struct command_lines and try to determine if any
+   command deletes breakpoints */
+
+static int 
+bpstat_actions_delete_breakpoints (struct command_line * cmd)
+{
+  for (; cmd; cmd = cmd->next)
+    {
+      struct cmd_list_element *ce;
+      char *line = cmd->line;
+      int i;
+      int ret;
+
+      ce = lookup_cmd (&line, cmdlist, "", 0, 1);
+
+      if (ce == NULL)
+	return 0;
+      
+
+      /* Check the command */
+      if (ce->class == class_user && !ce->hook_in)
+	{
+	  ce->hook_in = 1;
+	  ret = bpstat_actions_delete_breakpoints (ce->user_commands);
+	  ce->hook_in = 0;
+
+	  if (ret)
+	    return 1;
+	}
+      else if (strcmp (ce->name, "delete") == 0 
+	       || strcmp (ce->name, "clear") == 0)
+	{
+	  return 1;
+	}
+
+
+      /* Check the pre-hook */
+      if (ce->hook_pre)
+	{
+	  if (ce->hook_pre->class == class_user && !ce->hook_in)
+	    {
+	      ce->hook_in = 1;
+	      ret = bpstat_actions_delete_breakpoints (ce->hook_pre->user_commands);
+	      ce->hook_in = 0;
+	      
+	      if (ret)
+		return 1;
+	    }
+	  else if (strcmp (ce->hook_pre->name, "delete") == 0
+		   || strcmp (ce->hook_pre->name, "clear") == 0)
+	    {
+	      return 1;
+	    }
+	}
+
+
+      /* Check the post-hook */
+      if (ce->hook_post)
+	{
+	  if (ce->hook_post->class == class_user && !ce->hook_in)
+	    {
+	      ce->hook_in = 1;
+	      ret = bpstat_actions_delete_breakpoints (ce->hook_post->user_commands);
+	      ce->hook_in = 0;
+	      
+	      if (ret)
+		return 1;
+	    }
+	  else if (strcmp (ce->hook_post->name, "delete") == 0
+		   || strcmp (ce->hook_post->name, "clear") == 0)
+	    {
+	      return 1;
+	    }
+	}
+
+      
+      /* If this is a multi-part command (while, if, etc), check the
+	 body. */
+      for (i=0; i<cmd->body_count; i++)
+	if (bpstat_actions_delete_breakpoints (cmd->body_list[i]))
+	  return 1;
+
+    }
+
+  return 0;
+
+}
+
+
 /* 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
@@ -1800,7 +1896,6 @@ bpstat_do_actions (bpstat *bsp)
 {
   bpstat bs;
   struct cleanup *old_chain;
-  struct command_line *cmd;
 
   /* Avoid endless recursion if a `source' command is contained
      in bs->commands.  */
@@ -1825,16 +1920,37 @@ top:
   breakpoint_proceeded = 0;
   for (; bs != NULL; bs = bs->next)
     {
-      cmd = bs->commands;
-      while (cmd != NULL)
+      struct command_line *cmd;
+      struct cleanup *new_old_chain;
+
+      cmd = 0;
+      new_old_chain = 0;
+
+      /* If the command list for this breakpoint includes a statement
+	 that deletes breakpoints, we assume that the target may be
+	 this breakpoint, so we make a copy of the command list to
+	 avoid walking a list that has been deleted. */
+      
+      for (cmd = bs->commands; cmd; cmd = cmd->next)
+	{
+	  if (!new_old_chain && bpstat_actions_delete_breakpoints (cmd))
 	{
+	      cmd = dup_command_lines (cmd);
+	      new_old_chain = make_cleanup (cleanup_dup_command_lines, cmd);
+	    }
+
 	  execute_control_command (cmd);
 
 	  if (breakpoint_proceeded)
 	    break;
-	  else
-	    cmd = cmd->next;
 	}
+
+      if (new_old_chain)
+	{
+	  free_command_lines (&cmd);
+	  discard_cleanups (new_old_chain);
+	}
+
       if (breakpoint_proceeded)
 	/* The inferior is proceeded by the command; bomb out now.
 	   The bpstat chain has been blown away by wait_for_inferior.
@@ -1844,6 +1960,7 @@ top:
       else
 	bs->commands = NULL;
     }
+
 
   executing_breakpoint_commands = 0;
   discard_cleanups (old_chain);
Index: gdb/defs.h
===================================================================
RCS file: /cvs/src/src/gdb/defs.h,v
retrieving revision 1.67
diff -p -u -w -r1.67 defs.h
--- defs.h	2001/12/06 20:59:11	1.67
+++ defs.h	2001/12/14 08:13:08
@@ -839,6 +839,7 @@ struct command_line
 extern struct command_line *read_command_lines (char *, int);
 
 extern void free_command_lines (struct command_line **);
+extern struct command_line * dup_command_lines (struct command_line *);
 
 /* To continue the execution commands when running gdb asynchronously. 
    A continuation structure contains a pointer to a function to be called 
Index: gdb/cli/cli-script.c
===================================================================
RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
retrieving revision 1.8
diff -p -u -w -r1.8 cli-script.c
--- cli-script.c	2001/12/13 22:42:23	1.8
+++ cli-script.c	2001/12/14 08:13:08
@@ -1003,6 +1003,59 @@ read_command_lines (char *prompt_arg, in
   return (head);
 }
 
+/* Duplicate a chain of struct command_line's */
+
+struct command_line *
+dup_command_lines (struct command_line *l)
+{
+  struct command_line *dup = NULL;
+  register struct command_line *next = NULL;
+
+
+  for (; l ; l = l->next)
+    {
+      if (next == NULL)
+       {
+         dup = next = (struct command_line *)
+           xmalloc (sizeof (struct command_line));
+       }
+      else
+       {
+         next->next = (struct command_line *)
+           xmalloc (sizeof (struct command_line));
+
+         next = next->next;
+       }
+
+
+      if (next == NULL)
+       return NULL;
+
+
+      next->next = NULL;
+      next->line = xstrdup (l->line);
+      next->control_type = l->control_type;
+      next->body_count = l->body_count;
+
+
+      if (l->body_count > 0)
+       {
+         int i;
+         struct command_line **blist = l->body_list;
+
+         next->body_list =
+           (struct command_line **) xmalloc (sizeof (struct command_line *)
+                                             * l->body_count);
+
+         for (i = 0; i < l->body_count; i++, blist++)
+           next->body_list[i] = dup_command_lines (*blist);
+       }
+    }
+
+  return dup;
+}
+
+
 /* Free a chain of struct command_line's.  */
 
 void
-- 
dhoward@redhat.com
gdb engineering

Re: [RFA] deleting breakpoints inside of 'commands' (again)

[Don Howard]

[patch ping]

I never received any feedback on this, and had forgotten about it.  
Comments?



On Fri, 14 Dec 2001, Don Howard wrote:

> On Tue, 6 Nov 2001, Andrew Cagney wrote:
> 
> > > 
> > > Doesn't the function below catch that?  There is a recursive call to
> > > look for just the problem that you mention.  Is there some other way of
> > > deleting commands that I need to look out for?
> > 
> > I'm not sure.
> > 
> > Consider:
> > 
> > 	command callee
> >            delete command callee
> > 	end
> > 	breakpoint foo
> > 	commands $bp
> > 	  callee
> > 	end
> > 
> > it will realise that there is a delete but will it protect its self from 
> > a command deleting its self.
> > 
> > > [BTW, I personally think that the unconditional copy is much less
> > > error prone (and therefore better) even if it's slower. Just my $0.02]
> > 
> > Agreed (And slower is relative - what happens if there are 100's or 
> > 1000's of commands - that search becomes the bottle neck.)
> > 
> > Andrew
> 
> 
> 
> 
> I've added a call to lookup_cmd().  This allows user-defined commands to 
> be examined also, which should address your concern.
> 
> I've also added checks that examine the pre and post hook commands.
> 
> 
> 
> 2001-12-13  Don Howard  <dhoward@redhat.com>
>  
>         * breakpoint.c (bpstat_do_actions): Avoid deleting a 'commands'
>         list while executing that list.  Thanks to Pierre Muller
>         <muller@ics.u-strasbg.fr> for suggesting this implementation.
>         (cleanup_dup_command_lines): New function.
>         (bpstat_actions_delete_breakpoints): Ditto.
>         * cli/cli-script.c (dup_command_lines): New function.
>         * defs.h: Added declaration of new function.
> 
> 
> Index: gdb/breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.57
> diff -p -u -w -r1.57 breakpoint.c
> --- breakpoint.c	2001/11/11 16:39:59	1.57
> +++ breakpoint.c	2001/12/14 08:13:07
> @@ -1790,6 +1790,102 @@ cleanup_executing_breakpoints (PTR ignor
>    executing_breakpoint_commands = 0;
>  }
>  
> +static void
> +cleanup_dup_command_lines (PTR cmds)
> +{
> +  free_command_lines (cmds);
> +}
> +
> +
> +/* Walk a list of struct command_lines and try to determine if any
> +   command deletes breakpoints */
> +
> +static int 
> +bpstat_actions_delete_breakpoints (struct command_line * cmd)
> +{
> +  for (; cmd; cmd = cmd->next)
> +    {
> +      struct cmd_list_element *ce;
> +      char *line = cmd->line;
> +      int i;
> +      int ret;
> +
> +      ce = lookup_cmd (&line, cmdlist, "", 0, 1);
> +
> +      if (ce == NULL)
> +	return 0;
> +      
> +
> +      /* Check the command */
> +      if (ce->class == class_user && !ce->hook_in)
> +	{
> +	  ce->hook_in = 1;
> +	  ret = bpstat_actions_delete_breakpoints (ce->user_commands);
> +	  ce->hook_in = 0;
> +
> +	  if (ret)
> +	    return 1;
> +	}
> +      else if (strcmp (ce->name, "delete") == 0 
> +	       || strcmp (ce->name, "clear") == 0)
> +	{
> +	  return 1;
> +	}
> +
> +
> +      /* Check the pre-hook */
> +      if (ce->hook_pre)
> +	{
> +	  if (ce->hook_pre->class == class_user && !ce->hook_in)
> +	    {
> +	      ce->hook_in = 1;
> +	      ret = bpstat_actions_delete_breakpoints (ce->hook_pre->user_commands);
> +	      ce->hook_in = 0;
> +	      
> +	      if (ret)
> +		return 1;
> +	    }
> +	  else if (strcmp (ce->hook_pre->name, "delete") == 0
> +		   || strcmp (ce->hook_pre->name, "clear") == 0)
> +	    {
> +	      return 1;
> +	    }
> +	}
> +
> +
> +      /* Check the post-hook */
> +      if (ce->hook_post)
> +	{
> +	  if (ce->hook_post->class == class_user && !ce->hook_in)
> +	    {
> +	      ce->hook_in = 1;
> +	      ret = bpstat_actions_delete_breakpoints (ce->hook_post->user_commands);
> +	      ce->hook_in = 0;
> +	      
> +	      if (ret)
> +		return 1;
> +	    }
> +	  else if (strcmp (ce->hook_post->name, "delete") == 0
> +		   || strcmp (ce->hook_post->name, "clear") == 0)
> +	    {
> +	      return 1;
> +	    }
> +	}
> +
> +      
> +      /* If this is a multi-part command (while, if, etc), check the
> +	 body. */
> +      for (i=0; i<cmd->body_count; i++)
> +	if (bpstat_actions_delete_breakpoints (cmd->body_list[i]))
> +	  return 1;
> +
> +    }
> +
> +  return 0;
> +
> +}
> +
> +
>  /* 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
> @@ -1800,7 +1896,6 @@ bpstat_do_actions (bpstat *bsp)
>  {
>    bpstat bs;
>    struct cleanup *old_chain;
> -  struct command_line *cmd;
>  
>    /* Avoid endless recursion if a `source' command is contained
>       in bs->commands.  */
> @@ -1825,16 +1920,37 @@ top:
>    breakpoint_proceeded = 0;
>    for (; bs != NULL; bs = bs->next)
>      {
> -      cmd = bs->commands;
> -      while (cmd != NULL)
> +      struct command_line *cmd;
> +      struct cleanup *new_old_chain;
> +
> +      cmd = 0;
> +      new_old_chain = 0;
> +
> +      /* If the command list for this breakpoint includes a statement
> +	 that deletes breakpoints, we assume that the target may be
> +	 this breakpoint, so we make a copy of the command list to
> +	 avoid walking a list that has been deleted. */
> +      
> +      for (cmd = bs->commands; cmd; cmd = cmd->next)
> +	{
> +	  if (!new_old_chain && bpstat_actions_delete_breakpoints (cmd))
>  	{
> +	      cmd = dup_command_lines (cmd);
> +	      new_old_chain = make_cleanup (cleanup_dup_command_lines, cmd);
> +	    }
> +
>  	  execute_control_command (cmd);
>  
>  	  if (breakpoint_proceeded)
>  	    break;
> -	  else
> -	    cmd = cmd->next;
>  	}
> +
> +      if (new_old_chain)
> +	{
> +	  free_command_lines (&cmd);
> +	  discard_cleanups (new_old_chain);
> +	}
> +
>        if (breakpoint_proceeded)
>  	/* The inferior is proceeded by the command; bomb out now.
>  	   The bpstat chain has been blown away by wait_for_inferior.
> @@ -1844,6 +1960,7 @@ top:
>        else
>  	bs->commands = NULL;
>      }
> +
>  
>    executing_breakpoint_commands = 0;
>    discard_cleanups (old_chain);
> Index: gdb/defs.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/defs.h,v
> retrieving revision 1.67
> diff -p -u -w -r1.67 defs.h
> --- defs.h	2001/12/06 20:59:11	1.67
> +++ defs.h	2001/12/14 08:13:08
> @@ -839,6 +839,7 @@ struct command_line
>  extern struct command_line *read_command_lines (char *, int);
>  
>  extern void free_command_lines (struct command_line **);
> +extern struct command_line * dup_command_lines (struct command_line *);
>  
>  /* To continue the execution commands when running gdb asynchronously. 
>     A continuation structure contains a pointer to a function to be called 
> Index: gdb/cli/cli-script.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/cli/cli-script.c,v
> retrieving revision 1.8
> diff -p -u -w -r1.8 cli-script.c
> --- cli-script.c	2001/12/13 22:42:23	1.8
> +++ cli-script.c	2001/12/14 08:13:08
> @@ -1003,6 +1003,59 @@ read_command_lines (char *prompt_arg, in
>    return (head);
>  }
>  
> +/* Duplicate a chain of struct command_line's */
> +
> +struct command_line *
> +dup_command_lines (struct command_line *l)
> +{
> +  struct command_line *dup = NULL;
> +  register struct command_line *next = NULL;
> +
> +
> +  for (; l ; l = l->next)
> +    {
> +      if (next == NULL)
> +       {
> +         dup = next = (struct command_line *)
> +           xmalloc (sizeof (struct command_line));
> +       }
> +      else
> +       {
> +         next->next = (struct command_line *)
> +           xmalloc (sizeof (struct command_line));
> +
> +         next = next->next;
> +       }
> +
> +
> +      if (next == NULL)
> +       return NULL;
> +
> +
> +      next->next = NULL;
> +      next->line = xstrdup (l->line);
> +      next->control_type = l->control_type;
> +      next->body_count = l->body_count;
> +
> +
> +      if (l->body_count > 0)
> +       {
> +         int i;
> +         struct command_line **blist = l->body_list;
> +
> +         next->body_list =
> +           (struct command_line **) xmalloc (sizeof (struct command_line *)
> +                                             * l->body_count);
> +
> +         for (i = 0; i < l->body_count; i++, blist++)
> +           next->body_list[i] = dup_command_lines (*blist);
> +       }
> +    }
> +
> +  return dup;
> +}
> +
> +
>  /* Free a chain of struct command_line's.  */
>  
>  void
> 

-- 
dhoward@redhat.com
gdb engineering