Re: [RFA] deleting breakpoints inside of 'commands' [Repost]

[Kevin Buettner <kevinb@cygnus.com>]

On Sep 21,  4:53pm, Don Howard wrote:

> One more try. =) This patch adds a new field to struct command_line:
> 'executing'.  If this field is non-zero, free_command_lines() will not
> delete that struct command_line.  Instead, it increments the value of
> 'executing'.  bpstats_do_action() uses this behavior to see if it needs
> to delete the command_line after executing all the statements in the
> list.

I've looked your patch over, and it looks correct to me.  Having said
that, I think that the correctness of this patch is much less obvious
than the version that made a copy of the command chain associated with
a breakpoint.  I don't fault you for this; the changes in your current
patch are somewhat more distributed which means that there's more code
to consider (and more ways for something to get fouled up later on).

I do have some other comments though; see below...

> 	* defs.h: (struct
> 	command_line): Added new field 'executing'.

Why was this broken up between to lines?  (Sorry for nit-picking.)

Also, I wonder if there's a better name for this field?  It is true
that ``executing'' will be non-zero when the command is executing, but
one might be mislead into thinking that it's a simple boolean when in
fact it's a (sort of) counter.  Also, the point of this field has more
to do with determining when it's okay to delete a command list...

> Index: breakpoint.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/breakpoint.c,v
> retrieving revision 1.53
> diff -p -u -r1.53 breakpoint.c
> --- breakpoint.c	2001/09/18 05:00:48	1.53
> +++ breakpoint.c	2001/09/21 23:49:48
> @@ -1800,6 +1800,7 @@ bpstat_do_actions (bpstat *bsp)
>    bpstat bs;
>    struct cleanup *old_chain;
>    struct command_line *cmd;
> +  struct command_line *cmd_head;
> 
>    /* Avoid endless recursion if a `source' command is contained
>       in bs->commands.  */
> @@ -1824,6 +1825,10 @@ top:
>    breakpoint_proceeded = 0;
>    for (; bs != NULL; bs = bs->next)
>      {
> +      cmd_head = bs->commands;
> +      if (cmd_head)
> +	cmd_head->executing = 1;

Upon first looking at this portion of your patch, I was thinking of
``executing'' as a sort of reference count, and it seemed to me that
the above line ought to be ``cmd_head->executing++;''.  But now that I
think about it some more, I see that ``executing'' isn't really a
reference count, but rather a sort of two-purpose flag which tells
free_command_lines() that a command is executing; however, it may also
be changed by free_command_lines(), so it's second purpose is to let
the latter parts of bpstat_do_actions() know if a command chain deletion
was deferred by free_command_lines.

Anyway, I found this somewhat surprising.  I think I would've been
less surprised if ``executing'' was more of a conventional reference
count.

> +
>        cmd = bs->commands;
>        while (cmd != NULL)
>  	{
> @@ -1834,6 +1839,15 @@ top:
>  	  else
>  	    cmd = cmd->next;
>  	}
> +

I'd like to see a comment right here describing what's going on.  I
understand it because I get to see all the logic wrapped up in the
nice tidy patch to which I'm now replying, but I'm thinking it might
not be so obvious to someone encountering this code later on...

> +      if (cmd_head->executing != 1)
> +	{
> +	  cmd_head->executing = 0;
> +	  free_command_lines (&cmd_head);
> +	}
> +      else
> +	cmd_head->executing = 0;
> +

>        if (breakpoint_proceeded)
>  	/* The inferior is proceeded by the command; bomb out now.
>  	   The bpstat chain has been blown away by wait_for_inferior.
> Index: defs.h
> ===================================================================
> RCS file: /cvs/src/src/gdb/defs.h,v
> retrieving revision 1.63
> diff -p -u -r1.63 defs.h
> --- defs.h	2001/09/07 21:33:08	1.63
> +++ defs.h	2001/09/21 23:49:51
> @@ -832,6 +832,7 @@ struct command_line
>      enum command_control_type control_type;
>      int body_count;
>      struct command_line **body_list;

A comment right here describing what the member (below) is about would
really aid in understanding the code.  You should make it clear that
the real purpose of this field is in deciding whether a particular
command chain may be deleted immediately or if it must be deferred if
the command chain winds up being self-deleting.

> +    int executing;
>    };
> 
>  extern struct command_line *read_command_lines (char *, int);
> Index: cli/cli-script.c
[...]

The rest of your patch is fine.

Kevin