lkml.org 
[lkml]   [2008]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions
    Ingo Molnar wrote:
    > * Boaz Harrosh <bharrosh@panasas.com> wrote:
    >
    >> If the user of virtio_has_feature() must pass a compile-time constant
    >> then it must be converted to a MACRO, and then the BUILD_BUG_ON will
    >> work. Or it should be changed to a BUG_ON() if fbit is a runtime
    >> variable.
    >

    The use of __builtin_constant_p in inline functions is broken. This
    is because it will give different results depending on the -O level
    used. So I think that using it in the Kernel with inlines is plain
    broken. And should be discouraged.

    That said, my trick with enum is exactly the same as __builtin_constant_p
    when -O is off, that is, does not traverse inline. But it is consistent
    across any optimization.

    > well, that's the question i'm asking: that sort of proposed
    > BUILD_BUG_ON() variantcannot be used in inline functions like
    > virtio_has_feature() does. If we get forced back to macros that's not an
    > improvement.
    >

    I think it is an improvement, in a sense that now we think something is happening
    but get silently ignored if compilation conditions are different, and/or the programmer
    had a mistake. The new way will show us what code will be produced in the worse
    case and will error if wrong.

    > Maybe the link-time last-line-of-defense mechanism i posed is the most
    > flexible one perhaps after all? (it's ugly too but none of this is
    > particularly pretty)
    >

    The link-time gives the same results. Only warns at link time instead of
    compile time. The difference between our approaches is the use of
    __builtin_constant_p which is suppose to work cross inline stack boundary,
    but in effect it does not if the optimization is not just right.

    > hm?
    >
    > Ingo

    Here is gcc documentation about __builtin_constant_p:

    — Built-in Function: int __builtin_constant_p (exp)

    You can use the built-in function __builtin_constant_p to determine if a value is known to be constant at compile-time and hence that GCC can perform constant-folding on expressions involving that value. The argument of the function is the value to test. The function returns the integer 1 if the argument is known to be a compile-time constant and 0 if it is not known to be a compile-time constant. A return of 0 does not indicate that the value is not a constant, but merely that GCC cannot prove it is a constant with the specified value of the -O option.

    You would typically use this function in an embedded application where memory was a critical resource. If you have some complex calculation, you may want it to be folded if it involves constants, but need to call a function if it does not. For example:

    #define Scale_Value(X) \
    (__builtin_constant_p (X) \
    ? ((X) * SCALE + OFFSET) : Scale (X))


    You may use this built-in function in either a macro or an inline function. However, if you use it in an inlined function and pass an argument of the function as the argument to the built-in, GCC will never return 1 when you call the inline function with a string constant or compound literal (see Compound Literals) and will not return 1 when you pass a constant numeric value to the inline function unless you specify the -O option.

    You may also use __builtin_constant_p in initializers for static data. For instance, you can write

    static const int table[] = {
    __builtin_constant_p (EXPRESSION) ? (EXPRESSION) : -1,
    /* ... */
    };


    This is an acceptable initializer even if EXPRESSION is not a constant expression. GCC must be more conservative about evaluating the built-in in this case, because it has no opportunity to perform optimization.

    Previous versions of GCC did not accept this built-in in data initializers. The earliest version where it is completely safe is 3.0.1.


    I have tried the test below:
    #include <stdio.h>

    #define __maybe_unused __attribute__((unused))

    #define BUILD_BUG_ON_ORIG(condition) ((void)sizeof(char[1 - 2*!!(condition)]))

    #define BUILD_BUG_ON_B(condition) \
    do { \
    enum { bad = !!(condition)}; \
    static struct { char arr[1 - 2*bad]; } x __maybe_unused;\
    } while(0)

    #define BUILD_BUG_ON_R(condition) \
    do { \
    static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused; \
    } while(0)

    extern unsigned int __BUILD_BUG_ON_non_constant;
    #define BUILD_BUG_ON_I(condition) \
    do { \
    (void)sizeof(char[1 - 2*!!(condition)]); \
    if (!__builtin_constant_p(condition)) \
    __BUILD_BUG_ON_non_constant++; \
    } while (0)

    #define BUILD_BUG_ON BUILD_BUG_ON_R

    int main()
    {
    int var;

    var = random();

    BUILD_BUG_ON(2 < 1);
    BUILD_BUG_ON(1 < 2);
    BUILD_BUG_ON(var < 2);

    printf("var=%d", var);
    return 0;
    }

    where I changed #define BUILD_BUG_ON BUILD_BUG_ON_X to the three
    variants (ORIG/B/R/I) here is what I get (optimization is off).

    _ORIG:
    2 < 1: good (is silent)
    1 < 2: good (error report)
    var < 2: bad (just ignored)

    _B && _R:
    2 < 1: good (is silent)
    1 < 2: good (error report)
    var < 2: good (error report)

    _I: (optimization is off)
    2 < 1: bad (link time error)
    1 < 2: good (error report)
    var < 2: good- (link time error)

    So I think the BUILD_BUG_ON_R should be accepted. This will force
    two changes in current Kernel (i386 allmodconfig), which in my
    opinion are case 3 above and should be fixed anyway.

    Please propose other tests we should try, for example with cross
    inline-functions/macros.

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

    \
     
     \ /
      Last update: 2008-08-20 14:35    [W:2.627 / U:0.384 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site