Date:	Wed, 20 Aug 2008 15:31:53 +0300
From:	Boaz Harrosh <bharrosh@...asas.com>
To:	Ingo Molnar <mingo@...e.hu>
CC:	Rusty Russell <rusty@...tcorp.com.au>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Sam Ravnborg <sam@...nborg.org>
Subject: Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

Ingo Molnar wrote:
> * Boaz Harrosh <bharrosh@...asas.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists