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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux