[PATCH] BUILD_BUG_ON() should not use array type

[PATCH] BUILD_BUG_ON() should not use array type

[Jan Beulich]

With gcc's extension of variable size arrays, using the size of an
array type for BUILD_BUG_ON() doesn't always work, since non-constant
conditions aren't being detected, and hence the intention of the
construct cannot be met anymore. It is therefore necessary to use a
construct where all compilers can be expected to only accept constant
expressions with certain values being invalid, and the only thing I
could think of are bit-fields.

Note that in the virtio_config.h case MAYBE_BUILD_BUG_ON() really just
serves documentation purposes - even if the expression is compile time
constant (__builtin_constant_p() yields true), the array is still deemed
of variable length by gcc, and hence the whole expression doesn#t have
the intended effect.

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 drivers/net/niu.c             |    2 +-
 include/linux/kernel.h        |    8 ++++++--
 include/linux/virtio_config.h |    3 +--
 3 files changed, 8 insertions(+), 5 deletions(-)

--- linux-2.6.27-rc5/drivers/net/niu.c	2008-08-21 14:37:31.000000000 +0200
+++ 2.6.27-rc5-build-bug-on/drivers/net/niu.c	2008-08-28 16:51:40.000000000 +0200
@@ -5144,7 +5144,7 @@ static void niu_init_tx_mac(struct niu *
 	/* The XMAC_MIN register only accepts values for TX min which
 	 * have the low 3 bits cleared.
 	 */
-	BUILD_BUG_ON(min & 0x7);
+	BUG_ON(min & 0x7);
 
 	if (np->flags & NIU_FLAGS_XMAC)
 		niu_init_tx_xmac(np, min, max);
--- linux-2.6.27-rc5/include/linux/kernel.h	2008-08-21 14:37:35.000000000 +0200
+++ 2.6.27-rc5-build-bug-on/include/linux/kernel.h	2008-08-28 15:10:28.000000000 +0200
@@ -468,13 +468,17 @@ 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)BUILD_BUG_ON_ZERO(condition))
+
+/* Force a compilation error if condition is constant and true */
+#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
 
 /* 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
    e.g. in a structure initializer (or where-ever else comma expressions
    aren't permitted). */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
+#define BUILD_BUG_ON_NULL(e) ((void *)BUILD_BUG_ON_ZERO(e))
 
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)
--- linux-2.6.27-rc5/include/linux/virtio_config.h	2008-08-21 14:37:35.000000000 +0200
+++ 2.6.27-rc5-build-bug-on/include/linux/virtio_config.h	2008-08-28 15:08:47.000000000 +0200
@@ -96,8 +96,7 @@ static inline bool virtio_has_feature(co
 				      unsigned int fbit)
 {
 	/* Did you forget to fix assumptions on max features? */
-	if (__builtin_constant_p(fbit))
-		BUILD_BUG_ON(fbit >= 32);
+	MAYBE_BUILD_BUG_ON(fbit >= 32);
 
 	virtio_check_driver_offered_feature(vdev, fbit);
 	return test_bit(fbit, vdev->features);

Re: [PATCH] BUILD_BUG_ON() should not use array type

[Andrew Morton]

On Fri, 29 Aug 2008 12:46:34 +0100
"Jan Beulich" <jbeulich@novell.com> wrote:

> With gcc's extension of variable size arrays, using the size of an
> array type for BUILD_BUG_ON() doesn't always work, since non-constant
> conditions aren't being detected, and hence the intention of the
> construct cannot be met anymore. It is therefore necessary to use a
> construct where all compilers can be expected to only accept constant
> expressions with certain values being invalid, and the only thing I
> could think of are bit-fields.
> 
> Note that in the virtio_config.h case MAYBE_BUILD_BUG_ON() really just
> serves documentation purposes - even if the expression is compile time
> constant (__builtin_constant_p() yields true), the array is still deemed
> of variable length by gcc, and hence the whole expression doesn#t have
> the intended effect.
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  drivers/net/niu.c             |    2 +-
>  include/linux/kernel.h        |    8 ++++++--
>  include/linux/virtio_config.h |    3 +--
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> --- linux-2.6.27-rc5/drivers/net/niu.c	2008-08-21 14:37:31.000000000 +0200
> +++ 2.6.27-rc5-build-bug-on/drivers/net/niu.c	2008-08-28 16:51:40.000000000 +0200
> @@ -5144,7 +5144,7 @@ static void niu_init_tx_mac(struct niu *
>  	/* The XMAC_MIN register only accepts values for TX min which
>  	 * have the low 3 bits cleared.
>  	 */
> -	BUILD_BUG_ON(min & 0x7);
> +	BUG_ON(min & 0x7);
>  
>  	if (np->flags & NIU_FLAGS_XMAC)
>  		niu_init_tx_xmac(np, min, max);
> --- linux-2.6.27-rc5/include/linux/kernel.h	2008-08-21 14:37:35.000000000 +0200
> +++ 2.6.27-rc5-build-bug-on/include/linux/kernel.h	2008-08-28 15:10:28.000000000 +0200
> @@ -468,13 +468,17 @@ 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)BUILD_BUG_ON_ZERO(condition))
> +
> +/* Force a compilation error if condition is constant and true */
> +#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))

MAYBE_BUILD_BUG_ON() hurts my brain.  It's doing:

	if (__builtin_constant_p(expr))
		BUILD_BUG_ON(expr);

yes?  For inlined (or macro) callsites which can be used with constant
or non-constant args.

It's tempting to just zap the one callsite and not add this at all, but
I suppose that it's a legitimate thing to want to do, and that other
users of it may well turn up.  Many of them are probably just using run-time
BUG_ON(), which is sensible, but less efficient.

However it would benefit from a clearer description and perhaps a
better name.  BUILD_BUG_ON_IF_CONSTANT?


>  /* 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
>     e.g. in a structure initializer (or where-ever else comma expressions
>     aren't permitted). */
> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
> +#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
> +#define BUILD_BUG_ON_NULL(e) ((void *)BUILD_BUG_ON_ZERO(e))

What does BUILD_BUG_ON_NULL() do and why did you add it?  It has no
callers.

>  /* Trap pasters of __FUNCTION__ at compile-time */
>  #define __FUNCTION__ (__func__)
> --- linux-2.6.27-rc5/include/linux/virtio_config.h	2008-08-21 14:37:35.000000000 +0200
> +++ 2.6.27-rc5-build-bug-on/include/linux/virtio_config.h	2008-08-28 15:08:47.000000000 +0200
> @@ -96,8 +96,7 @@ static inline bool virtio_has_feature(co
>  				      unsigned int fbit)
>  {
>  	/* Did you forget to fix assumptions on max features? */
> -	if (__builtin_constant_p(fbit))
> -		BUILD_BUG_ON(fbit >= 32);
> +	MAYBE_BUILD_BUG_ON(fbit >= 32);
>  
>  	virtio_check_driver_offered_feature(vdev, fbit);
>  	return test_bit(fbit, vdev->features);
> 
> 
> 

Re: [PATCH] BUILD_BUG_ON() should not use array type

[Alexey Dobriyan]

> > +/* Force a compilation error if condition is constant and true */
> > +#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
> 
> MAYBE_BUILD_BUG_ON() hurts my brain.  It's doing:
> 
> 	if (__builtin_constant_p(expr))
> 		BUILD_BUG_ON(expr);
> 
> yes?  For inlined (or macro) callsites which can be used with constant
> or non-constant args.
> 
> It's tempting to just zap the one callsite and not add this at all,

Yes, please. There should be no maybes in kernel.

Imagine probabilistic BUG_ON(). Imagine probabilistic refcounting: maybe_get_net().

Re: [PATCH] BUILD_BUG_ON() should not use array type

[Jan Beulich]

>> --- linux-2.6.27-rc5/include/linux/kernel.h	2008-08-21 14:37:35.000000000 +0200
>> +++ 2.6.27-rc5-build-bug-on/include/linux/kernel.h	2008-08-28 15:10:28.000000000 +0200
>> @@ -468,13 +468,17 @@ 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)BUILD_BUG_ON_ZERO(condition))
>> +
>> +/* Force a compilation error if condition is constant and true */
>> +#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))
>
>MAYBE_BUILD_BUG_ON() hurts my brain.  It's doing:
>
>	if (__builtin_constant_p(expr))
>		BUILD_BUG_ON(expr);
>
>yes?  For inlined (or macro) callsites which can be used with constant
>or non-constant args.

Yes, its attempting to have the effect of that if() construct.

>It's tempting to just zap the one callsite and not add this at all, but
>I suppose that it's a legitimate thing to want to do, and that other
>users of it may well turn up.  Many of them are probably just using run-time
>BUG_ON(), which is sensible, but less efficient.
>
>However it would benefit from a clearer description and perhaps a
>better name.  BUILD_BUG_ON_IF_CONSTANT?

Admittedly I didn't like the name much either, but so I also didn't like
any other name I could think of. And really, I'd also favor removing
the one use site altogether (hence the comment about the construct
there not having any effect), but thought it'd be a step backward
documentation-wise.

So for re-submitting an adjusted patch I'd need to be clear about the
way to go...

>>  /* 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
>>     e.g. in a structure initializer (or where-ever else comma expressions
>>     aren't permitted). */
>> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
>> +#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>> +#define BUILD_BUG_ON_NULL(e) ((void *)BUILD_BUG_ON_ZERO(e))
>
>What does BUILD_BUG_ON_NULL() do and why did you add it?  It has no
>callers.

I was under the (apparently wrong) impression that while plain 0 can be
used to initialize a pointer, an expression evaluating to zero would need
a cast. While that's not the case as I just checked, I'd still prefer keeping
the construct both for documentation purposes and because in the error
case without the case you'd not only get the intended error message,
but also an extra (and perhaps confusing) warning.

Jan