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:0.058 / U:1.984 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site