Re: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions

[Boaz Harrosh <bharrosh@panasas.com>]

Boaz Harrosh wrote:
> Ingo Molnar wrote:
>> * Rusty Russell <rusty@rustcorp.com.au> wrote:
>>
>>> On Monday 18 August 2008 03:33:19 Ingo Molnar wrote:
>>>> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>>>>> Gag me now.
>>>>>
>>>>> Why not just do
>>>>>
>>>>>  #define __BBO(c)                sizeof(const char[1 - 2*!!(c)])
>>>>>  #define __BBONC(c)              __BBO(!__builtin_constant_p(c))
>>>>>  #define BUILD_BUG_ON_ZERO(c)    (__BBO(c) - __BBONC(c))
>>>>>  #define BUILD_BUG_ON(c)         (void)BUILD_BUG_ON_ZERO(c)
>>>>>
>>>>> and be done with it?
>>>> yeah, i first tried a few variants of that (compile-time warnings are
>>>> much better than link time warnings), but none worked when i tested
>>>> various failure modes.
>>> Hey, I thought I was the "undisputed ruler of Ugly-land".
>>>
>>> How about this instead:
>>>
>>> #define BUILD_BUG_ON(condition)						\
>>> do {									\
>>> 	static struct { char arr[1 - 2*!!(condition)]; } x __maybe_unused;	\
>>> } while(0)
>> hm, have you tried it and do we get a severe enough link error about 
>> that? If the macro gets ignored by the compiler that's really a hard 
>> error - such things are essential safeguards of kernel sanity:
>>
>> 	/*
>> 	 * Build-time sanity checks on the kernel image and module
>> 	 * area mappings. (these are purely build-time and produce no code)
>> 	 */
>> 	BUILD_BUG_ON(MODULES_VADDR < KERNEL_IMAGE_START);
>> 	BUILD_BUG_ON(MODULES_VADDR-KERNEL_IMAGE_START < KERNEL_IMAGE_SIZE);
>> 	BUILD_BUG_ON(MODULES_LEN + KERNEL_IMAGE_SIZE > 2*PUD_SIZE);
>> 	BUILD_BUG_ON((KERNEL_IMAGE_START & ~PMD_MASK) != 0);
>> 	BUILD_BUG_ON((MODULES_VADDR & ~PMD_MASK) != 0);
>> 	BUILD_BUG_ON(!(MODULES_VADDR > __START_KERNEL));
>> 	BUILD_BUG_ON(!(((MODULES_END - 1) & PGDIR_MASK) ==
>> 				(__START_KERNEL & PGDIR_MASK)));
>> 	BUILD_BUG_ON(__fix_to_virt(__end_of_fixed_addresses) <= MODULES_END);
>>
>> ( and propagating them into runtime failures not only increases bloat,
>>   it also makes failures harder to debug. These checks 'run' _early_. )
>>
>> Link time warnings are easy enough to miss.
>>
>> So unless there's a better way of doing it all at compile time (i'd 
>> really prefer that!) i'd prefer the link time error about botched 
>> BUILD_BUG_ON() conditions - as my commits introduce.
>>
>> 	Ingo
>> --
> 
> #define BUILD_BUG_ON(condition)						\
> do {
> 	enum { bad = !!(condition)}; 									\
> 	static struct { char arr[1 - 2*bad]; } x __maybe_unused;	\
> } while(0)
> 
> the enum definition will not let in anything not compile-time constant.
> But then I fail on: (include/linux/virtio_config.h:99)
> 
> 	if (__builtin_constant_p(fbit))
> 		BUILD_BUG_ON(fbit >= 32);
> 
> is that code broken?
> 
> Boaz
> 

with this patch:
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index aaa998f..91d98f2 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -468,7 +468,12 @@ struct sysinfo {
 };
 
 /* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+// #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUILD_BUG_ON(condition)					\
+do { 								\
+	enum { bad = !!(condition)}; 				\
+	static struct { char arr[1 - 2*bad]; } x __maybe_unused;\
+} while(0)
 
 /* Force a compilation error if condition is true, but also produce a
    result (of value 0 and type size_t), so the expression can be used


I found another BUILD_BUG_ON() none const bug, here:

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index e4765b7..1469a38 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -5133,9 +5133,9 @@ static void niu_init_tx_bmac(struct niu *np, u64 min, u64 max)
 
 static void niu_init_tx_mac(struct niu *np)
 {
-	u64 min, max;
+	u64 max;
 
-	min = 64;
+	enum { min = 64 } ;
 	if (np->dev->mtu > ETH_DATA_LEN)
 		max = 9216;
 	else