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

[Boaz Harrosh <bharrosh@panasas.com>]

Ingo Molnar wrote:
> * Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>>> 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.
> 
> nice trick!
> 
>> 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?
> 
> hmm ... that's a bit sad, gcc ought to have been able to figure this 
> out. Can this be fixed somehow, without losing the strength of the 
> checking here? I think we should not change BUILD_BUG_ON() to make it 
> less useful.
> 
> 	Ingo

The BUILD_BUG_ON I proposed is not less useful. Actually with current
BUILD_BUG_ON above code does nothing, since fbit is a function parameter
it will translate to nothing. Certainly it will not check the size of passed
parameter, since that was converted on the stack to unsigned int. So my
definition will catch the bad programing here.

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.

>From what I understood this is what you wanted, some way to make sure
a BUILD_BUG_ON will check that the check is actually useful (compile-time)
and is not silently ignored.

Same thing at the other place I sent.

Boaz