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

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

[Don Howard]

[This is a repost with a few nits fixed, and addressed to the correct
list =) ]

Here is an other variation on how to deal with 'commands' scripts that
delete their own breakpoint.  The patch below makes a copy of the
commands list before executing it and deletes the copy when finished.
Comments?


2001-09-14  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.
	* 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.52
diff -p -u -w -r1.52 breakpoint.c
--- breakpoint.c	2001/08/02 11:58:28	1.52
+++ breakpoint.c	2001/09/17 16:30:46
@@ -1824,7 +1824,8 @@ top:
   breakpoint_proceeded = 0;
   for (; bs != NULL; bs = bs->next)
     {
-      cmd = bs->commands;
+      cmd = dup_command_lines (bs->commands);
+
       while (cmd != NULL)
 	{
 	  execute_control_command (cmd);
@@ -1834,6 +1835,9 @@ top:
 	  else
 	    cmd = cmd->next;
 	}
+
+      free_command_lines (&cmd);
+
       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 -w -r1.63 defs.h
--- defs.h	2001/09/07 21:33:08	1.63
+++ defs.h	2001/09/17 16:30:47
@@ -837,6 +837,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/09/17 16:30:47
@@ -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' [Repost]

[Michael Snyder]

Don Howard wrote:
> 
> [This is a repost with a few nits fixed, and addressed to the correct
> list =) ]
> 
> Here is an other variation on how to deal with 'commands' scripts that
> delete their own breakpoint.  The patch below makes a copy of the
> commands list before executing it and deletes the copy when finished.
> Comments?

You mean other than "the concept makes my head hurt"?   ;-)

Well, let's see... seems like making a copy unconditionally
is going to slow down the execution of all command lists, 
for the benefit of those few that delete themselves.  

Can we think of a way to make the duplication conditional
on the need?  That is, on the current command list being
deleted?  Put off the cost until it becomes necessary?

I can think of one method, but it would require an extra pointer
in the breakpoint struct.  "execute_control_command" would set
this pointer to point to its command list, and delete_breakpoint
would check the pointer.  If it's non-null, then the duplication
would be done by delete_breakpoint, and the pointer updated to 
point to the duplicate.

Is it worth the effort?  Is this duplication costly 
compared to everything else already being done by
bpstat_do_actions?  Or am I worrying over nothing?

Michael
 

> 
> 2001-09-14  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.
>         * 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.52
> diff -p -u -w -r1.52 breakpoint.c
> --- breakpoint.c        2001/08/02 11:58:28     1.52
> +++ breakpoint.c        2001/09/17 16:30:46
> @@ -1824,7 +1824,8 @@ top:
>    breakpoint_proceeded = 0;
>    for (; bs != NULL; bs = bs->next)
>      {
> -      cmd = bs->commands;
> +      cmd = dup_command_lines (bs->commands);
> +
>        while (cmd != NULL)
>         {
>           execute_control_command (cmd);
> @@ -1834,6 +1835,9 @@ top:
>           else
>             cmd = cmd->next;
>         }
> +
> +      free_command_lines (&cmd);
> +
>        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 -w -r1.63 defs.h
> --- defs.h      2001/09/07 21:33:08     1.63
> +++ defs.h      2001/09/17 16:30:47
> @@ -837,6 +837,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/09/17 16:30:47
> @@ -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' [Repost]

[Fernando Nasser]

Michael Snyder wrote:
> 
> Don Howard wrote:
> >
> > [This is a repost with a few nits fixed, and addressed to the correct
> > list =) ]
> >
> > Here is an other variation on how to deal with 'commands' scripts that
> > delete their own breakpoint.  The patch below makes a copy of the
> > commands list before executing it and deletes the copy when finished.
> > Comments?
> 
> You mean other than "the concept makes my head hurt"?   ;-)
> 
> Well, let's see... seems like making a copy unconditionally
> is going to slow down the execution of all command lists,
> for the benefit of those few that delete themselves.
> 
> Can we think of a way to make the duplication conditional
> on the need?  That is, on the current command list being
> deleted?  Put off the cost until it becomes necessary?
> 
> I can think of one method, but it would require an extra pointer
> in the breakpoint struct.  "execute_control_command" would set
> this pointer to point to its command list, and delete_breakpoint
> would check the pointer.  If it's non-null, then the duplication
> would be done by delete_breakpoint, and the pointer updated to
> point to the duplicate.
> 
> Is it worth the effort?  Is this duplication costly
> compared to everything else already being done by
> bpstat_do_actions?  Or am I worrying over nothing?
> 

I share your concerns.  And I see no reason why this should be allowed
--
the script can always "disable" its own breakpoint with the same effect 
for all practical purposes.

A patch adding a "cannot delete self" error message would be nice.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Andrew Cagney]

>> Is it worth the effort?  Is this duplication costly
>> compared to everything else already being done by
>> bpstat_do_actions?  Or am I worrying over nothing?

I think this is in the noise.  GDB has performance problems with very 
large symbol files, it doesn't have problems with 3 line breakpoint scripts.

> I share your concerns.  And I see no reason why this should be allowed
> --
> the script can always "disable" its own breakpoint with the same effect 
> for all practical purposes.
> 
> A patch adding a "cannot delete self" error message would be nice.

I would really rather not see GDB introduce, undocumented, edge 
conditions like this.  I think the patch Don submitted had the very nice 
effect of eliminating the need for such a special case.

2c

Andrew

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

[Fernando Nasser]

Andrew Cagney wrote:
> 
> >> Is it worth the effort?  Is this duplication costly
> >> compared to everything else already being done by
> >> bpstat_do_actions?  Or am I worrying over nothing?
> 
> I think this is in the noise.  GDB has performance problems with very
> large symbol files, it doesn't have problems with 3 line breakpoint scripts.
> 

Why 3 line?  There are very long breakpoint scripts around.

Furthermore, when using breakpoint scripts to track some bugs (sometimes 
"condition" is not enough) it may be desirable to minimize the artifact.

> > I share your concerns.  And I see no reason why this should be allowed
> > --
> > the script can always "disable" its own breakpoint with the same effect
> > for all practical purposes.
> >
> > A patch adding a "cannot delete self" error message would be nice.
> 
> I would really rather not see GDB introduce, undocumented, edge
> conditions like this.  I think the patch Don submitted had the very nice
> effect of eliminating the need for such a special case.
> 

If at least it would make the copy only when needed (i.e., when someone
is
trying to delete the breakpoint that has the script attached on it)
things
would not be that bad.


Michael, do you remove your objection or does it still stands after
Andrew's
arguments?

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Michael Snyder]

Andrew Cagney wrote:
> 
> >> Is it worth the effort?  Is this duplication costly
> >> compared to everything else already being done by
> >> bpstat_do_actions?  Or am I worrying over nothing?
> 
> I think this is in the noise.  GDB has performance problems with very
> large symbol files, it doesn't have problems with 3 line breakpoint scripts.

I know GDB has performance problems with symbols, but I do not
know that it doesn't have performance problems with executing
command lists.  I know that when I used to work on the XRAY
debugger, macro performance was a really big issue, whereas
no one seems to have talked about it much in GDB... 


> > I share your concerns.  And I see no reason why this should be allowed
> > --
> > the script can always "disable" its own breakpoint with the same effect
> > for all practical purposes.
> >
> > A patch adding a "cannot delete self" error message would be nice.
> 
> I would really rather not see GDB introduce, undocumented, edge
> conditions like this.  I think the patch Don submitted had the very nice
> effect of eliminating the need for such a special case.

Obviously it would be bad for it to be undocumented.
But I do not agree that the restriction:

	a breakpoint command set cannot delete itself

is particularly ugly.  Obviously it would be (at least a little)
better to _not_ have such a restriction, but there is always
a cost/benefit analysis to these questions.

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

[Andrew Cagney]

> 
> I know GDB has performance problems with symbols, but I do not
> know that it doesn't have performance problems with executing
> command lists.  I know that when I used to work on the XRAY
> debugger, macro performance was a really big issue, whereas
> no one seems to have talked about it much in GDB... 

I'd expect gdb's macro performance to also be an issue.  I'd expect the 
problems to come from host<->target overhead and not this command 
duplication though.

Andrew

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

[Michael Snyder]

Andrew Cagney wrote:
> 
> >
> > I know GDB has performance problems with symbols, but I do not
> > know that it doesn't have performance problems with executing
> > command lists.  I know that when I used to work on the XRAY
> > debugger, macro performance was a really big issue, whereas
> > no one seems to have talked about it much in GDB...
> 
> I'd expect gdb's macro performance to also be an issue.  I'd expect the
> problems to come from host<->target overhead and not this command
> duplication though.

OK, well, virtually anything will pale to insignificance 
next to host<->target overhead.  As I said, if I'm the only
one who feels any concern about this, I will waive it.

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

[Fernando Nasser]

Michael Snyder wrote:
> 
> Andrew Cagney wrote:
> >
> > >
> > > I know GDB has performance problems with symbols, but I do not
> > > know that it doesn't have performance problems with executing
> > > command lists.  I know that when I used to work on the XRAY
> > > debugger, macro performance was a really big issue, whereas
> > > no one seems to have talked about it much in GDB...
> >
> > I'd expect gdb's macro performance to also be an issue.  I'd expect the
> > problems to come from host<->target overhead and not this command
> > duplication though.
> 
> OK, well, virtually anything will pale to insignificance
> next to host<->target overhead.  As I said, if I'm the only
> one who feels any concern about this, I will waive it.

I have the same concerns.
We haven't heard from Don yet.  Maybe he has some compromise solution.

Anyway, I find the copy solution a hack.

One way to fix this is to have the chain of commands as an object with
use count.  It is only freed when the count is down to zero again.

When you associate it with a breakpoint it goes up to 1.  When you
get it to execute it goes up to 2.

When a breakpoint is deleted, it deallocates it.  If the count goes 
to zero memory is freed.  But if the script is being executed (and
is deleting self) the count will go to 1 and nothing else happens
until the script finishes executing and the chain is freed (then
the count goes to zero and memory is deallocated).


-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Andrew Cagney]

> 
> I have the same concerns.
> We haven't heard from Don yet.  Maybe he has some compromise solution.
> 
> Anyway, I find the copy solution a hack.
> 
> One way to fix this is to have the chain of commands as an object with
> use count.  It is only freed when the count is down to zero again.
> 
> When you associate it with a breakpoint it goes up to 1.  When you
> get it to execute it goes up to 2.
> 
> When a breakpoint is deleted, it deallocates it.  If the count goes 
> to zero memory is freed.  But if the script is being executed (and
> is deleting self) the count will go to 1 and nothing else happens
> until the script finishes executing and the chain is freed (then
> the count goes to zero and memory is deallocated).

Rememeber, the patch doesn't have to be perfect, just acceptable.  In 
this case, the change eliminates a stray pointer problem (which would 
likely still occure with reference counters) and hence makes gdb far 
more robust - I put robustness and maintainability at a much higher 
priority level then performance.
When someone manages to demonstrate that the copy is a significant 
overhead (using ``set maint profile on/off'' [:-)]) then I think we 
should refine the code to do what you propose (or gasp add a garbage 
collector :-/).  However, Don, if you're upto the task.

Andrew

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

[Fernando Nasser]

Andrew Cagney wrote:
> 
> >
> > I have the same concerns.
> > We haven't heard from Don yet.  Maybe he has some compromise solution.
> >
> > Anyway, I find the copy solution a hack.
> >
> > One way to fix this is to have the chain of commands as an object with
> > use count.  It is only freed when the count is down to zero again.
> >
> > When you associate it with a breakpoint it goes up to 1.  When you
> > get it to execute it goes up to 2.
> >
> > When a breakpoint is deleted, it deallocates it.  If the count goes
> > to zero memory is freed.  But if the script is being executed (and
> > is deleting self) the count will go to 1 and nothing else happens
> > until the script finishes executing and the chain is freed (then
> > the count goes to zero and memory is deallocated).
> 
> Rememeber, the patch doesn't have to be perfect, just acceptable.  In
> this case, the change eliminates a stray pointer problem (which would
> likely still occure with reference counters) and hence makes gdb far
> more robust - I put robustness and maintainability at a much higher
> priority level then performance.
> When someone manages to demonstrate that the copy is a significant
> overhead (using ``set maint profile on/off'' [:-)]) then I think we
> should refine the code to do what you propose (or gasp add a garbage
> collector :-/).  However, Don, if you're upto the task.
> 

Yes, I am one that keeps saying that we should fix gross bugs as soon
as possible, even if we have to come back and improve on the fix.

However, it may be possible to fix it correctly with a little more
effort if Don is willing.  Otherwise we add it with a _huge_ FIXME
and add the cleanup task to the TODO list.

Don?

-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Don Howard]

On Wed, 19 Sep 2001, Andrew Cagney wrote:

> >
> > I have the same concerns.
> > We haven't heard from Don yet.  Maybe he has some compromise solution.
> >
> > Anyway, I find the copy solution a hack.
> >
> > One way to fix this is to have the chain of commands as an object with
> > use count.  It is only freed when the count is down to zero again.
> >
> > When you associate it with a breakpoint it goes up to 1.  When you
> > get it to execute it goes up to 2.
> >
> > When a breakpoint is deleted, it deallocates it.  If the count goes
> > to zero memory is freed.  But if the script is being executed (and
> > is deleting self) the count will go to 1 and nothing else happens
> > until the script finishes executing and the chain is freed (then
> > the count goes to zero and memory is deallocated).
>
> Rememeber, the patch doesn't have to be perfect, just acceptable.  In
> this case, the change eliminates a stray pointer problem (which would
> likely still occure with reference counters) and hence makes gdb far
> more robust - I put robustness and maintainability at a much higher
> priority level then performance.
> When someone manages to demonstrate that the copy is a significant
> overhead (using ``set maint profile on/off'' [:-)]) then I think we
> should refine the code to do what you propose (or gasp add a garbage
> collector :-/).  However, Don, if you're upto the task.
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


I don't understand what you are asking here.  I've followed the thread
and it seems that the unconditional copy is not acceptable.  I will look
at the suggestions that Fernando and Michael have suggested and see if I
can come up with another patch.


-- 
-Don
dhoward@redhat.com
gdb engineering

PRMS not TODO: [RFA] deleting breakpoints inside of 'commands' [Repost]

[Andrew Cagney]

Just fyi,

> However, it may be possible to fix it correctly with a little more
> effort if Don is willing.  Otherwise we add it with a _huge_ FIXME
> and add the cleanup task to the TODO list.

I moved all the TODO items into prms.  Hopefully that is an easier way 
of tracking them.

Andrew

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

[Kevin Buettner]

On Sep 19,  9:34am, Don Howard wrote:

> > > I have the same concerns.
> > > We haven't heard from Don yet.  Maybe he has some compromise solution.
> > >
> > > Anyway, I find the copy solution a hack.
> > >
> > > One way to fix this is to have the chain of commands as an object with
> > > use count.  It is only freed when the count is down to zero again.
> > >
> > > When you associate it with a breakpoint it goes up to 1.  When you
> > > get it to execute it goes up to 2.
> > >
> > > When a breakpoint is deleted, it deallocates it.  If the count goes
> > > to zero memory is freed.  But if the script is being executed (and
> > > is deleting self) the count will go to 1 and nothing else happens
> > > until the script finishes executing and the chain is freed (then
> > > the count goes to zero and memory is deallocated).
> >
> > Rememeber, the patch doesn't have to be perfect, just acceptable.  In
> > this case, the change eliminates a stray pointer problem (which would
> > likely still occure with reference counters) and hence makes gdb far
> > more robust - I put robustness and maintainability at a much higher
> > priority level then performance.
> > When someone manages to demonstrate that the copy is a significant
> > overhead (using ``set maint profile on/off'' [:-)]) then I think we
> > should refine the code to do what you propose (or gasp add a garbage
> > collector :-/).  However, Don, if you're upto the task.
>                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> I don't understand what you are asking here.  I've followed the thread
> and it seems that the unconditional copy is not acceptable.  I will look
> at the suggestions that Fernando and Michael have suggested and see if I
> can come up with another patch.

I don't think that reference counting is the right way to go.  You'll
be adding complexity to GDB in the form of making certain parts of GDB
responsible for updating the reference counts.  Also, there's the
overhead of maintaining the reference counts.  I agree that making a
copy of the commands might be a little bit slower, but it has the
advantage of being simple which makes it easier to verify correctness.

A slightly more complicated scheme would examine the command list for
commands which may alter/delete the list and then tag the entire list
as one that needs to be copied.  This would be done ahead of time
(probably at the time that the list is created).  There's no point in
scanning the command list every time we want to execute the commands
because it's nearly as cheap to make a copy.  (Both are linear time
operations.)

Kevin

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

[Michael Snyder]

Kevin Buettner wrote:
> 
> On Sep 19,  9:34am, Don Howard wrote:
> 
> > > > I have the same concerns.
> > > > We haven't heard from Don yet.  Maybe he has some compromise solution.
> > > >
> > > > Anyway, I find the copy solution a hack.
> > > >
> > > > One way to fix this is to have the chain of commands as an object with
> > > > use count.  It is only freed when the count is down to zero again.
> > > >
> > > > When you associate it with a breakpoint it goes up to 1.  When you
> > > > get it to execute it goes up to 2.
> > > >
> > > > When a breakpoint is deleted, it deallocates it.  If the count goes
> > > > to zero memory is freed.  But if the script is being executed (and
> > > > is deleting self) the count will go to 1 and nothing else happens
> > > > until the script finishes executing and the chain is freed (then
> > > > the count goes to zero and memory is deallocated).
> > >
> > > Rememeber, the patch doesn't have to be perfect, just acceptable.  In
> > > this case, the change eliminates a stray pointer problem (which would
> > > likely still occure with reference counters) and hence makes gdb far
> > > more robust - I put robustness and maintainability at a much higher
> > > priority level then performance.
> > > When someone manages to demonstrate that the copy is a significant
> > > overhead (using ``set maint profile on/off'' [:-)]) then I think we
> > > should refine the code to do what you propose (or gasp add a garbage
> > > collector :-/).  However, Don, if you're upto the task.
> >                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > I don't understand what you are asking here.  I've followed the thread
> > and it seems that the unconditional copy is not acceptable.  I will look
> > at the suggestions that Fernando and Michael have suggested and see if I
> > can come up with another patch.
> 
> I don't think that reference counting is the right way to go.  You'll
> be adding complexity to GDB in the form of making certain parts of GDB
> responsible for updating the reference counts.  Also, there's the
> overhead of maintaining the reference counts.  I agree that making a
> copy of the commands might be a little bit slower, but it has the
> advantage of being simple which makes it easier to verify correctness.
> 
> A slightly more complicated scheme would examine the command list for
> commands which may alter/delete the list and then tag the entire list
> as one that needs to be copied.  This would be done ahead of time
> (probably at the time that the list is created).  There's no point in
> scanning the command list every time we want to execute the commands
> because it's nearly as cheap to make a copy.  (Both are linear time
> operations.)

I thought about this, but then I thought that the command list
might include user-defined commands, which in turn might call
delete.  That makes it a recursive problem.  And I'm not sure
whether user commands might be re-defined later (after this
step has been done.)

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

[Kevin Buettner]

On Sep 19, 12:17pm, Michael Snyder wrote:

> > A slightly more complicated scheme would examine the command list for
> > commands which may alter/delete the list and then tag the entire list
> > as one that needs to be copied.  This would be done ahead of time
> > (probably at the time that the list is created).  There's no point in
> > scanning the command list every time we want to execute the commands
> > because it's nearly as cheap to make a copy.  (Both are linear time
> > operations.)
> 
> I thought about this, but then I thought that the command list
> might include user-defined commands, which in turn might call
> delete.  That makes it a recursive problem.  And I'm not sure
> whether user commands might be re-defined later (after this
> step has been done.)

Good point.  I hadn't considered this scenario.

I guess you could just consider any user defined commands to be potentially
dangerous and cause the command list to be copied in such circumstances.

Kevin

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

[Kevin Buettner]

On Sep 19,  4:53pm, Fernando Nasser wrote:

> Kevin Buettner wrote:
> > 
> > I don't think that reference counting is the right way to go.  You'll
> > be adding complexity to GDB in the form of making certain parts of GDB
> > responsible for updating the reference counts.  Also, there's the
> > overhead of maintaining the reference counts.  I agree that making a
> > copy of the commands might be a little bit slower, but it has the
> > advantage of being simple which makes it easier to verify correctness.
> > 
> 
> What complexity?  The same parts of GDB that allocate/deallocate the
> list
> (maybe two places?) will to call a slightly different function.  The one
> that deallocates checks the counter first intead of blindly freeing up
> memory.  The only addition is a couple of calls where we pick a list of
> commands to be executed (one place perhaps) to make sure the fact we are
> using it is known. 

This doesn't sound too bad; if the patches look reasonable, I may change
my mind...

HOWEVER, I'm against reference counts in general because it can be
very hard to get the counter increments / decrements put in the right
places.  If you screw it up, you either get memory leaks or memory
corruption or both.  If we're contemplating the use of reference
counts for other areas of GDB, I think we ought to rethink the problem
and use some other garbage collection technique instead.

Kevin

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

[Fernando Nasser]

Kevin Buettner wrote:
> 
> This doesn't sound too bad; if the patches look reasonable, I may change
> my mind...
> 

I am sconfident it can be small.  I don't know if this is what Don's
next patch
will do, but if it is we will know for sure.

> HOWEVER, I'm against reference counts in general because it can be
> very hard to get the counter increments / decrements put in the right
> places.  If you screw it up, you either get memory leaks or memory
> corruption or both.  If we're contemplating the use of reference
> counts for other areas of GDB, I think we ought to rethink the problem
> and use some other garbage collection technique instead.
> 

Lets not get carried over :-)  I am not proposing it as a GDB
programming
style in general.  I just suggested it as an algorithm to solve a
specific
problem with low overhead.  Don't worry.

The ref counts can get really messy if everyone has to deal with them.
But, if properly encapsulated, most programers will not even know that
they are
dealing with ref counts and the chances of leaks/corruption is very slow
(whoever programmed the object had worked 26 hours in a row or
something).

Take a look at the "Proxy" pattern on the "Design Patterns" book (and
blame
Andrew Cagney for reminding me of this book a few months ago ;-) ).

Regards,
Fernando

-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Don Howard]

Here is a patch that I hope will satisfy all sides.  The patch below
changes free_command_lines() to check if we are currently
'executing_breakpoint_commands'.  If so, the command list is appended to
a list of 'orphaned_command_lines'.  When the execution of the command
list completes, the orphaned_command_lines are then deleted.

Should I make the global variables static and add accessor/mutator
functions?  What is the policy on globals?

Comments?



2001-09-20  Don Howard  <dhoward@redhat.com>

	* cli/cli-script.c (free_command_lines): Avoid deleting
	command_line lists while executing that list.
	* breakpoint.c: (executing_breakpoint_commands): Make this
	global visible to other compilation units.
	(orphaned_breakpoint_commands) New global.
	(bpstat_do_actions): Free orphaned command_line structures once
	done executing them.


Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.53
diff -p -u -w -r1.53 breakpoint.c
--- breakpoint.c	2001/09/18 05:00:48	1.53
+++ breakpoint.c	2001/09/20 22:18:17
@@ -221,8 +221,11 @@ extern int addressprint;	/* Print machin
 static int internal_breakpoint_number = -1;

 /* Are we executing breakpoint commands?  */
-static int executing_breakpoint_commands;
+int executing_breakpoint_commands;

+/* List of breakpoint commands to be cleaned up after we are done executing them. */
+struct command_line * orphaned_breakpoint_commands;
+
 /* Walk the following statement or block through all breakpoints.
    ALL_BREAKPOINTS_SAFE does so even if the statment deletes the current
    breakpoint.  */
@@ -1845,6 +1848,7 @@ top:
     }

   executing_breakpoint_commands = 0;
+  free_command_lines (&orphaned_breakpoint_commands);
   discard_cleanups (old_chain);
 }

Index: gdb/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/09/20 22:18:17
@@ -1014,7 +1014,25 @@ free_command_lines (struct command_line
   register struct command_line *next;
   struct command_line **blist;
   int i;
+  extern int executing_breakpoint_commands;
+  extern struct command_line * orphaned_breakpoint_commands;

+  /* Avoid deleting command_line lists while executing that list. */
+  if (executing_breakpoint_commands)
+    {
+      struct command_line **b = NULL;
+
+      /* Find the end of the orphaned_breakpoint_commands list */
+      for (b = &orphaned_breakpoint_commands; *b; b = &((*b)->next))
+	/* Nothing */
+	;
+
+      /* Append the passed-in list */
+      *b = *lptr;
+      *lptr = NULL;
+      return;
+    }
+
   while (l)
     {
       if (l->body_count > 0)


-- 
-Don
dhoward@redhat.com
gdb engineering

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

[Andrew Cagney]

> Should I make the global variables static and add accessor/mutator
> functions?  What is the policy on globals?

They tend to not go down very well.  Patches adding ``extern''s to C 
files will simply be rejected.

Andrew

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

[Andrew Cagney]

> Kevin Buettner wrote:
> 
>> 
>> On Sep 26,  4:01pm, Fernando Nasser wrote:
>> 
> 
>> > And I don't think Don's latest patch is complex, or considerably more
>> > complex than the simplistic copy approach (or hack!).  It is elegant
>> > (although Kevin's comments are valid and should be incorporated).
>> >
>> > If Don can add a cleanup function and do the polishing suggested by Kevin
>> > on his last patch I suggest that we stick with that one.
> 
>> 
>> Here are my preferences...
>> 
>> I like the patch that makes a copy the best since the changes are easy
>> to understand and localized to one function.

As noted before it is mine as well.

Andrew

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

[Fernando Nasser]

Kevin Buettner wrote:
> 
> On Sep 26,  4:01pm, Fernando Nasser wrote:
> 
> > And I don't think Don's latest patch is complex, or considerably more
> > complex than the simplistic copy approach (or hack!).  It is elegant
> > (although Kevin's comments are valid and should be incorporated).
> >
> > If Don can add a cleanup function and do the polishing suggested by Kevin
> > on his last patch I suggest that we stick with that one.
> 
> Here are my preferences...
> 
> I like the patch that makes a copy the best since the changes are easy
> to understand and localized to one function.
> 
> Next best would be a patch (unwritten as of now) which implements
> reference counts for these command chains.  I think that a lot of
> folks understand reference counts and it takes less time to grok code
> written with this design pattern.  In all likelyhood, such a patch
> would look somewhat similar to Don's latest patch though; i.e, the
> changes would be distributed among several files and would likely be
> in the same functions that Don has touched with his latest patch.
> 
> Next best would be Don's latest patch revised to use an enum for
> the ``execute'' member.  I think this code would be easier to
> understand by giving the 0, 1, and 2+ values actual names that are
> meaningful.  I still have concerns about what might happen if
> bpstat_do_actions() is ever executed recursively...  I don't think this
> ever happens, so it's probably a non-issue, but if it could happen,
> then a reference counting mechanism would definitely be superior.
> 
> (All of the above approaches need to make use of a cleanup.)
> 
> Kevin
> 

That is a good analysis.  My preference is know to be the second above.
However, if a _real_ consensus is reached on the external list that we
should just use the duplicating patch I will remove my objections.


> P.S. I'd like to publicly thank Don for all the hard work he has
> put in on this patch.  I imagine it must be frustrating to be told
> to do the patch one way by one person, a different way by a second,
> and still a different way by a third...

I would like to second that.  It must be noted that Don has not, in 
any moment, has complained of anything.  On the contrary, he kept
thinking about the problem and trying to provide alternative 
solutions.  In the best tradition of the best Open Source contributors.

W.r.t. the pain of having to re-work patches comes with the job.  That
people will have different views and preferences comes with humanity.
This happens in any Open Source project -- we have been through this 
before in this list and the same thing happens daily on the PostgreSQL
database project lists.  We could have discussed alternatives without
Don writing patches, but it was his patches that kept bringing more
light into the problem (fr himself included).   And he learned that
globals are no-no's among other things, was able to understand some
more parts of the code etc.  It was not wasted time.

P.S.: I would like to see some people going through a Ph.D. and having
to change things in circle until your supervisor understands you. 


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Kevin Buettner]

On Sep 26,  4:01pm, Fernando Nasser wrote:

> And I don't think Don's latest patch is complex, or considerably more
> complex than the simplistic copy approach (or hack!).  It is elegant 
> (although Kevin's comments are valid and should be incorporated).
> 
> If Don can add a cleanup function and do the polishing suggested by Kevin
> on his last patch I suggest that we stick with that one.

Here are my preferences...

I like the patch that makes a copy the best since the changes are easy
to understand and localized to one function.

Next best would be a patch (unwritten as of now) which implements
reference counts for these command chains.  I think that a lot of
folks understand reference counts and it takes less time to grok code
written with this design pattern.  In all likelyhood, such a patch
would look somewhat similar to Don's latest patch though; i.e, the
changes would be distributed among several files and would likely be
in the same functions that Don has touched with his latest patch.

Next best would be Don's latest patch revised to use an enum for
the ``execute'' member.  I think this code would be easier to
understand by giving the 0, 1, and 2+ values actual names that are
meaningful.  I still have concerns about what might happen if
bpstat_do_actions() is ever executed recursively...  I don't think this
ever happens, so it's probably a non-issue, but if it could happen,
then a reference counting mechanism would definitely be superior.

(All of the above approaches need to make use of a cleanup.)

Kevin

P.S. I'd like to publicly thank Don for all the hard work he has
put in on this patch.  I imagine it must be frustrating to be told
to do the patch one way by one person, a different way by a second,
and still a different way by a third...

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

[Fernando Nasser]

Andrew Cagney wrote:
> 
> >
> > 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).
> 
> And guess what (sorry but this is funny :-) I suspect it does contain a
> bug.  Try:
> 
>         break main
>         commands
>         delete NN
>         leak-memory
>         end
> 
> The command ``leak-memory'' is invalid and will lead to an error() call
> and that will in turn long jump over the code that would free the list.
> 
> While the duplicate version contains the same bug, I suspect it is
> easier to fix vis:
> 
>         o       duplicate list
>         o       add list to a cleanup
>         o       run command
>         o       do cleanups
> 
> I suspect to do this with the non-duplicate version you'll need to add a
> catch_exceptions() call (nee catch_errors()) and check the state of that
> ->execute variable to figure out what to do.
> 

I don't see why it is not possible to register a cleanup in Don's latest
solution.  The function would do nothing unless a "delete NN" was executed
before we bail out.



> I do tend to agree with Kevin though.  Some times simplicity is best.
> 

I wish you have thought like that in previous instances. ;-)

And I don't think Don's latest patch is complex, or considerably more
complex than the simplistic copy approach (or hack!).  It is elegant 
(although Kevin's comments are valid and should be incorporated).



If Don can add a cleanup function and do the polishing suggested by Kevin
on his last patch I suggest that we stick with that one.


-- 
Fernando Nasser
Red Hat - Toronto                       E-Mail:  fnasser@redhat.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

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

[Andrew Cagney]

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

And guess what (sorry but this is funny :-) I suspect it does contain a 
bug.  Try:

	break main
	commands
	delete NN
	leak-memory
	end


The command ``leak-memory'' is invalid and will lead to an error() call 
and that will in turn long jump over the code that would free the list.

While the duplicate version contains the same bug, I suspect it is 
easier to fix vis:

	o	duplicate list
	o	add list to a cleanup
	o	run command
	o	do cleanups

I suspect to do this with the non-duplicate version you'll need to add a 
catch_exceptions() call (nee catch_errors()) and check the state of that 
->execute variable to figure out what to do.

I do tend to agree with Kevin though.  Some times simplicity is best.

Andrew

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

[Kevin Buettner]

On Sep 24,  5:10pm, Kevin Buettner wrote:

> > 	* defs.h: (struct
> > 	command_line): Added new field 'executing'.
> 
> Why was this broken up between to lines?  (Sorry for nit-picking.)
                                 ^^
Oops.  I meant "two" there.

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

[Kevin Buettner]

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