xfree() -- set ptr to nil (fwd)

xfree() -- set ptr to nil (fwd)

[John R. Moore]

Whilst fixing xfree() callsI noticed that xfree() itself has a peculiarity
that needs attention:

The call goes like this:

if (ptr != NULL)
  free(ptr);

Nice, but why not the following:

if (ptr)
  {
     free (ptr);
     prt = NULL);
  }

The latter catches any re-calls to xfree(), unless the compiler sets the
ptr to nil for one (gcc doesn't appear to). Anyhow, it's a good practice
to do this anyhow.

Any opinions?  The only reason I can think not to is to insure that gdb
core dumps on succesive xfree() calls to the same pointer (and hence
insure efficient code, but in that case, why bother with xfree() in the
first place.

John Moore

Re: xfree() -- set ptr to nil (fwd)

[Kevin Buettner]

On Feb 12,  3:07pm, John R. Moore wrote:

> Whilst fixing xfree() callsI noticed that xfree() itself has a peculiarity
> that needs attention:
> 
> The call goes like this:
> 
> if (ptr != NULL)
>   free(ptr);
> 
> Nice, but why not the following:
> 
> if (ptr)
>   {
>      free (ptr);
>      prt = NULL);
>   }
> 
> The latter catches any re-calls to xfree(), unless the compiler sets the
> ptr to nil for one (gcc doesn't appear to). Anyhow, it's a good practice
> to do this anyhow.
> 
> Any opinions?  The only reason I can think not to is to insure that gdb
> core dumps on succesive xfree() calls to the same pointer (and hence
> insure efficient code, but in that case, why bother with xfree() in the
> first place.

Let me see if I understand you correctly.  You'd like to replace

    void
    xfree (void *ptr)
    {
      if (ptr != NULL)
	free (ptr);
    }

with

    void
    xfree (void *ptr)
    {
      if (ptr)
	{
	  free (ptr);
	  ptr = NULL;
	}
    }

right?

If so, how will this work?  ``ptr'' is a local variable and will not
be modified outside the scope of xfree().

What you have in mind could be done with a macro and I have seen
this done in other programs.  (But rather than insuring that gdb
core dumps on successive xfree() calls, it instead causes gdb to
core dump when attempting to use an already freed-and-nulled pointer.)

Kevin

Re: xfree() -- set ptr to nil (fwd)

[John R. Moore]

Yes, I've usually seen this as a macro

#define XFREE(ptr) do \
{ \
 if (ptr) \
  { \
    free (ptr); \
    ptr = NULL; \
  } \
} while (0)

Ok, then, do we want to replace xfree() with something like XFREE() ?

John   (Can I hide my first post? :-)  )


On Mon, 12 Feb 2001, Kevin Buettner wrote:

> On Feb 12,  3:07pm, John R. Moore wrote:
>
> > Whilst fixing xfree() callsI noticed that xfree() itself has a peculiarity
> > that needs attention:
> >
> > The call goes like this:
> >
> > if (ptr != NULL)
> >   free(ptr);
> >
> > Nice, but why not the following:
> >
> > if (ptr)
> >   {
> >      free (ptr);
> >      prt = NULL);
> >   }
> >
> > The latter catches any re-calls to xfree(), unless the compiler sets the
> > ptr to nil for one (gcc doesn't appear to). Anyhow, it's a good practice
> > to do this anyhow.
> >
> > Any opinions?  The only reason I can think not to is to insure that gdb
> > core dumps on succesive xfree() calls to the same pointer (and hence
> > insure efficient code, but in that case, why bother with xfree() in the
> > first place.
>
> Let me see if I understand you correctly.  You'd like to replace
>
>     void
>     xfree (void *ptr)
>     {
>       if (ptr != NULL)
> 	free (ptr);
>     }
>
> with
>
>     void
>     xfree (void *ptr)
>     {
>       if (ptr)
> 	{
> 	  free (ptr);
> 	  ptr = NULL;
> 	}
>     }
>
> right?
>
> If so, how will this work?  ``ptr'' is a local variable and will not
> be modified outside the scope of xfree().
>
> What you have in mind could be done with a macro and I have seen
> this done in other programs.  (But rather than insuring that gdb
> core dumps on successive xfree() calls, it instead causes gdb to
> core dump when attempting to use an already freed-and-nulled pointer.)
>
> Kevin
>

Re: xfree() -- set ptr to nil (fwd)

[Daniel Berlin]

On Mon, 12 Feb 2001, John R. Moore wrote:

>
> Yes, I've usually seen this as a macro
>
> #define XFREE(ptr) do \
> { \
>  if (ptr) \
>   { \
>     free (ptr); \
>     ptr = NULL; \
>   } \
> } while (0)
>
> Ok, then, do we want to replace xfree() with something like XFREE() ?

No, we try to get *rid* of macros, not add them.
--Dan

Re: xfree() -- set ptr to nil (fwd)

[John R. Moore]

Ok, I see the light. Thanks for responding quickly.

Re: xfree() -- set ptr to nil (fwd)

[J.T. Conklin]

>>>>> "John" == John R Moore <jmoore@cygnus.com> writes:
John> Yes, I've usually seen this as a macro
John>
John> #define XFREE(ptr) do \
John> { \
John>  if (ptr) \
John>   { \
John>     free (ptr); \
John>     ptr = NULL; \
John>   } \
John> } while (0)
John>
John> Ok, then, do we want to replace xfree() with something like XFREE() ?

I'd say no, unless it has been established that we have a problem
where pointers are freed multiple times AND it's been determined that
its too difficult to fix the code to avoid the bug.

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: xfree() -- set ptr to nil (fwd)

[J.T. Conklin]

>>>>> "John" == John R Moore <jmoore@cygnus.com> writes:
John> Nice, but why not the following:
John>
John> if (ptr)
John>   {
John>      free (ptr);
John>      prt = NULL);
John>   }
John>
John> The latter catches any re-calls to xfree(), unless the compiler sets the
John> ptr to nil for one (gcc doesn't appear to). Anyhow, it's a good practice
John> to do this anyhow.
John>
John> Any opinions?  The only reason I can think not to is to insure that gdb
John> core dumps on succesive xfree() calls to the same pointer (and hence
John> insure efficient code, but in that case, why bother with xfree() in the
John> first place.

This won't work.  Remember ptr is passed by value.  While you can
change ptr's value within xfree(), that doesn't change the value of
the variable that was passed to it.

IMO, xfree()'s reason for existance is not protecting against multiple
frees of the same object, but supporting frees of a possibly NULL ptr.
Modern free() implementations already support this, but we support old
pre-ANSI C libraries as well.  

        --jtc

-- 
J.T. Conklin
RedBack Networks

Re: xfree() -- set ptr to nil (fwd)

[Michael Elizabeth Chastain]

John Moore writes:

> Ok, then, do we want to replace xfree() with something like XFREE() ?

This gets into a philosophical discussion.  Here is my philosophy.
Others may differ.

Basically, I am interested in work that leads to patches that get applied
and have concrete, observable gains.  If someone can identify specific
bugs that we've had in gdb as a result of xfree problems, then I would
be interested in safety work such as s/xfree/XFREE/g, arranging to run
gdb regularly under Purify, and projects like that.

But I haven't seen problems like that so s/xfree/XFREE/g does not appeal
to me.

This is just a guideline.  I really liked Kevin Buettner's PARAMS removal
patches, for example.  In that case, the code afterwards was uneqivocally
simpler than the code before.

My two cents.  And I'm not a maintainer, just a write-after-approval guy.

Michael Elizabeth Chastain
<chastain@redhat.com>
"love without fear"

Re: xfree() -- set ptr to nil (fwd)

[Andrew Cagney]

Some platforms dump core from free(NULL), yet an ISO-C free(NULL)
doesn't.  xfree() addresses this inconsistency by ensuring ISO-C
behavour.

	enjoy,
		Andrew

PS: Drugs^D^D^D^D^D Macros are bad M'kay ....