Re: [PATCH] likely cleanup: remove unlikely for kfree(NULL)

[Kyle Moffett]

On Apr 27, 2006, at 04:17:50, Bart Hartgers wrote:
> Kyle Moffett wrote:
> > static inline void kfree(void *ptr)
> > {
> >     if (__builtin_constant_p((ptr == NULL))) {
> >         if (ptr)
> >             kfree_nonnull(ptr);
> >     } else {
> >         kfree_unknown(ptr);
> >     }
> > }
> >
> > void kfree_nonnull(void *ptr)
> > {
> >     /* kfree code here, no null check */
> > }
> >
> > void kfree_unknown(void *ptr)
> > {
> >     if (ptr)
> >         kfree_nonnull(ptr);
> > }
>
> I still think there is an inconsistency in gcc. If I call your  
> kfree with the following:
>
> void test( char *ptr )
> {
>         char *null = NULL;
>         kfree(ptr);	/* unknown */
>         *ptr = 'a';
>         kfree(ptr);	/* nonnull */
>         kfree(null);	/* should be optimised away */
> }
>
> ,the compiler (4.1) generates two calls to kfree_unknown instead of  
> one to kfree_nonnull and one to kfree_unknown. It seems that the  
> __builtin_constant_p((ptr==NULL)) check does not always trigger,  
> even if the compiler 'knows' ptr to be equal to NULL. I posted a  
> nasty hack around this problem yesterday.

I know.  You can "fix" this problem by changing the if statement to  
this:

if (__builtin_constant_p(ptr) || __builtin_constant_p((ptr == NULL)))

On the other hand, calling kfree(ptr) on a non-NULL constant pointer  
is a bug and will crash, and calling kfree(ptr) on a NULL constant  
ptr is just dead code and we should find and kill that separately.   
There's no reason to ever call kfree(<constant>).

Cheers,
Kyle Moffett

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/