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

[Andrew Morton]

On Fri, 29 Aug 2008 12:46:34 +0100
"Jan Beulich" <jbeulich@xxxxxxxxxx> 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@xxxxxxxxxx>
> 
> ---
>  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);
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/