[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

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]

[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

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