[RFA]: Fix do_cleanups if oldchain is NULL

[RFA]: Fix do_cleanups if oldchain is NULL

[Jeff Johnston]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

I recently solved a bug on the ia64 concerning cleanups.  What was 
happening was that a cleanup list was being re-initialized to NULL 
inside a loop and later do_cleanups() was called.  This caused the 
entire cleanup list to be run because the design is to run the list 
until the passed in cleanup is reached.  This caused other errors when 
the stream being used was deleted, etc...

This patch adds a check to do_my_cleanups() so no cleanups will be 
performed if the passed in chain is NULL.

Ok to commit?

-- Jeff J.

2003-12-17  Jeff Johnston  <jjohnstn@redhat.com>

        * utils.c (do_my_cleanups): Don't do cleanups if old chain
        passed in is NULL.



[-- Attachment #2: utils.patch --]
[-- Type: text/plain, Size: 794 bytes --]

Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.110
diff -u -p -r1.110 utils.c
--- utils.c	21 Sep 2003 01:26:45 -0000	1.110
+++ utils.c	17 Dec 2003 20:35:56 -0000
@@ -316,11 +316,14 @@ do_my_cleanups (struct cleanup **pmy_cha
 		struct cleanup *old_chain)
 {
   struct cleanup *ptr;
-  while ((ptr = *pmy_chain) != old_chain)
+  if (old_chain != NULL)
     {
-      *pmy_chain = ptr->next;	/* Do this first incase recursion */
-      (*ptr->function) (ptr->arg);
-      xfree (ptr);
+      while ((ptr = *pmy_chain) != old_chain)
+        {
+          *pmy_chain = ptr->next;	/* Do this first incase recursion */
+          (*ptr->function) (ptr->arg);
+          xfree (ptr);
+        }
     }
 }
 

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[Andrew Cagney]

> I recently solved a bug on the ia64 concerning cleanups.  What was happening was that a cleanup list was being re-initialized to NULL inside a loop and later do_cleanups() was called.  This caused the entire cleanup list to be run because the design is to run the list until the passed in cleanup is reached.  This caused other errors when the stream being used was deleted, etc...
> 
> This patch adds a check to do_my_cleanups() so no cleanups will be performed if the passed in chain is NULL.
> 
> Ok to commit?

(hmm, no one thought to review this while I was on hols :-()
I think the bug is in the calling code, and not utils.c.  That patch 
unfortunatly makes a fundamental change to the core of the cleanup code 
and there's no easy way of demonstrating that other callers aren't 
assuming that NULL implies do all cleanups.

Andrew


> -- Jeff J.
> 
> 2003-12-17  Jeff Johnston  <jjohnstn@redhat.com>
> 
>        * utils.c (do_my_cleanups): Don't do cleanups if old chain
>        passed in is NULL.

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[Daniel Jacobowitz]

On Fri, Jan 02, 2004 at 11:06:49AM -0500, Andrew Cagney wrote:
> >I recently solved a bug on the ia64 concerning cleanups.  What was 
> >happening was that a cleanup list was being re-initialized to NULL inside 
> >a loop and later do_cleanups() was called.  This caused the entire cleanup 
> >list to be run because the design is to run the list until the passed in 
> >cleanup is reached.  This caused other errors when the stream being used 
> >was deleted, etc...
> >
> >This patch adds a check to do_my_cleanups() so no cleanups will be 
> >performed if the passed in chain is NULL.
> >
> >Ok to commit?
> 
> (hmm, no one thought to review this while I was on hols :-()

Sorry, I was holding off on reviewing other people's patches during my vacation
so that I could catch up on my own.

> I think the bug is in the calling code, and not utils.c.  That patch 
> unfortunatly makes a fundamental change to the core of the cleanup code 
> and there's no easy way of demonstrating that other callers aren't 
> assuming that NULL implies do all cleanups.

How about making NULL an internal error then, as below?  I think we
should either do that or document its behavior, and it seems
accident-prone.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

2003-01-02  Daniel Jacobowitz  <drow@mvista.com>

	* utils.c (do_my_cleanups): Raise an internal error if the old_chain
	is NULL.

Index: utils.c
===================================================================
RCS file: /cvs/src/src/gdb/utils.c,v
retrieving revision 1.111
diff -u -p -r1.111 utils.c
--- utils.c	2 Jan 2004 17:35:01 -0000	1.111
+++ utils.c	2 Jan 2004 17:49:37 -0000
@@ -1,8 +1,8 @@
 /* General utility routines for GDB, the GNU debugger.
 
    Copyright 1986, 1988, 1989, 1990, 1991, 1992, 1993, 1994, 1995,
-   1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003 Free Software
-   Foundation, Inc.
+   1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003, 2004
+   Free Software Foundation, Inc.
 
    This file is part of GDB.
 
@@ -318,6 +318,11 @@ do_my_cleanups (struct cleanup **pmy_cha
 		struct cleanup *old_chain)
 {
   struct cleanup *ptr;
+
+  if (old_chain == NULL)
+    internal_error (__FILE__, __LINE__,
+		    "NULL chain passed to do_my_cleanups");
+
   while ((ptr = *pmy_chain) != old_chain)
     {
       *pmy_chain = ptr->next;	/* Do this first incase recursion */

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[Andrew Cagney]

> 
> How about making NULL an internal error then, as below?  I think we
> should either do that or document its behavior, and it seems
> accident-prone.

I thought about that, and would likely be ok if there were only normal 
simple cleanups.  Its the "other" (i.e., final, run, exec, ...) cleanups 
that trouble me.

Andrew

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[Daniel Jacobowitz]

On Mon, Jan 05, 2004 at 10:32:38AM -0500, Andrew Cagney wrote:
> >
> >How about making NULL an internal error then, as below?  I think we
> >should either do that or document its behavior, and it seems
> >accident-prone.
> 
> I thought about that, and would likely be ok if there were only normal 
> simple cleanups.  Its the "other" (i.e., final, run, exec, ...) cleanups 
> that trouble me.

Right you are, infcmd.c:run_command:
   do_run_cleanups (NULL);

We could put the internal error in do_cleanups instead, but I'm not
sure that it's worthwhile.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[J. Johnston]

Andrew Cagney wrote:
>> I recently solved a bug on the ia64 concerning cleanups.  What was 
>> happening was that a cleanup list was being re-initialized to NULL 
>> inside a loop and later do_cleanups() was called.  This caused the 
>> entire cleanup list to be run because the design is to run the list 
>> until the passed in cleanup is reached.  This caused other errors when 
>> the stream being used was deleted, etc...
>>
>> This patch adds a check to do_my_cleanups() so no cleanups will be 
>> performed if the passed in chain is NULL.
>>
>> Ok to commit?
> 
> 
> (hmm, no one thought to review this while I was on hols :-()
> I think the bug is in the calling code, and not utils.c.  That patch 
> unfortunatly makes a fundamental change to the core of the cleanup code 
> and there's no easy way of demonstrating that other callers aren't 
> assuming that NULL implies do all cleanups.
> 
> Andrew
> 

Perhaps, but there is no way to properly initialize a cleanup to avoid compiler 
warnings.  If you set it to NULL which is the obvious choice, this currently 
means run all cleanups.  IMHO, this is the wrong choice for the default.  I 
found no cases where NULL was passed in directly.  Do you see a case where a gdb 
routine has the right to run all previous cleanups except in an exit scenario?

-- Jeff J.

> 
>> -- Jeff J.
>>
>> 2003-12-17  Jeff Johnston  <jjohnstn@redhat.com>
>>
>>        * utils.c (do_my_cleanups): Don't do cleanups if old chain
>>        passed in is NULL.
> 
> 
> 
> 

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[Daniel Jacobowitz]

On Mon, Jan 05, 2004 at 01:59:53PM -0500, J. Johnston wrote:
> 
> 
> Andrew Cagney wrote:
> >>I recently solved a bug on the ia64 concerning cleanups.  What was 
> >>happening was that a cleanup list was being re-initialized to NULL 
> >>inside a loop and later do_cleanups() was called.  This caused the 
> >>entire cleanup list to be run because the design is to run the list 
> >>until the passed in cleanup is reached.  This caused other errors when 
> >>the stream being used was deleted, etc...
> >>
> >>This patch adds a check to do_my_cleanups() so no cleanups will be 
> >>performed if the passed in chain is NULL.
> >>
> >>Ok to commit?
> >
> >
> >(hmm, no one thought to review this while I was on hols :-()
> >I think the bug is in the calling code, and not utils.c.  That patch 
> >unfortunatly makes a fundamental change to the core of the cleanup code 
> >and there's no easy way of demonstrating that other callers aren't 
> >assuming that NULL implies do all cleanups.
> >
> >Andrew
> >
> 
> Perhaps, but there is no way to properly initialize a cleanup to avoid 
> compiler warnings.

Sure there is:
  struct cleanup *old_chain = make_cleanup (null_cleanup, 0);

> If you set it to NULL which is the obvious choice, this 
> currently means run all cleanups.  IMHO, this is the wrong choice for the 
> default.  I found no cases where NULL was passed in directly.  Do you see a 
> case where a gdb routine has the right to run all previous cleanups except 
> in an exit scenario?

This function is not just used for do_cleanups, but also for
do_run_cleanups et cetera.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA]: Fix do_cleanups if oldchain is NULL

[J. Johnston]

Daniel Jacobowitz wrote:
> On Mon, Jan 05, 2004 at 01:59:53PM -0500, J. Johnston wrote:
> 
>>
>>Andrew Cagney wrote:
>>
>>>>I recently solved a bug on the ia64 concerning cleanups.  What was 
>>>>happening was that a cleanup list was being re-initialized to NULL 
>>>>inside a loop and later do_cleanups() was called.  This caused the 
>>>>entire cleanup list to be run because the design is to run the list 
>>>>until the passed in cleanup is reached.  This caused other errors when 
>>>>the stream being used was deleted, etc...
>>>>
>>>>This patch adds a check to do_my_cleanups() so no cleanups will be 
>>>>performed if the passed in chain is NULL.
>>>>
>>>>Ok to commit?
>>>
>>>
>>>(hmm, no one thought to review this while I was on hols :-()
>>>I think the bug is in the calling code, and not utils.c.  That patch 
>>>unfortunatly makes a fundamental change to the core of the cleanup code 
>>>and there's no easy way of demonstrating that other callers aren't 
>>>assuming that NULL implies do all cleanups.
>>>
>>>Andrew
>>>
>>
>>Perhaps, but there is no way to properly initialize a cleanup to avoid 
>>compiler warnings.
> 
> 
> Sure there is:
>   struct cleanup *old_chain = make_cleanup (null_cleanup, 0);
> 

Ok, that suits me fine.

> 
>>If you set it to NULL which is the obvious choice, this 
>>currently means run all cleanups.  IMHO, this is the wrong choice for the 
>>default.  I found no cases where NULL was passed in directly.  Do you see a 
>>case where a gdb routine has the right to run all previous cleanups except 
>>in an exit scenario?
> 
> 
> This function is not just used for do_cleanups, but also for
> do_run_cleanups et cetera.
> 

I just saw the note regarding do_run_cleanup (NULL).  My fault for not looking 
ahead in my mass of e-mails :(.  I didn't check for that scenario.  I no longer 
have any issue.

-- Jeff J.