[PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[Jan Beulich]

gcc permitting variable length arrays makes the current construct
used for BUILD_BUG_ON() useless, as that doesn't produce any diagnostic
if the controlling expression isn't really constant. Instead, this
patch makes it so that a bit field gets used here. Consequently, those
uses where the condition isn't really constant now also need fixing.

Note that in the gfp.h, kmemcheck.h, and virtio_config.h cases
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/char/tpm/tpm.c        |    2 --
 drivers/net/niu.c             |    2 +-
 include/linux/gfp.h           |    2 +-
 include/linux/kernel.h        |    8 ++++++--
 include/linux/kmemcheck.h     |    2 +-
 include/linux/virtio_config.h |    3 +--
 6 files changed, 10 insertions(+), 9 deletions(-)

--- linux-2.6.31-rc6/drivers/char/tpm/tpm.c	2009-08-18 15:31:17.000000000 +0200
+++ 2.6.31-rc6-build-bug-on/drivers/char/tpm/tpm.c	2009-08-17 15:21:11.000000000 +0200
@@ -696,7 +696,6 @@ int __tpm_pcr_read(struct tpm_chip *chip
 
 	cmd.header.in = pcrread_header;
 	cmd.params.pcrread_in.pcr_idx = cpu_to_be32(pcr_idx);
-	BUILD_BUG_ON(cmd.header.in.length > READ_PCR_RESULT_SIZE);
 	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
 			  "attempting to read a pcr value");
 
@@ -760,7 +759,6 @@ int tpm_pcr_extend(u32 chip_num, int pcr
 		return -ENODEV;
 
 	cmd.header.in = pcrextend_header;
-	BUILD_BUG_ON(be32_to_cpu(cmd.header.in.length) > EXTEND_PCR_SIZE);
 	cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
 	memcpy(cmd.params.pcrextend_in.hash, hash, TPM_DIGEST_SIZE);
 	rc = transmit_cmd(chip, &cmd, cmd.header.in.length,
--- linux-2.6.31-rc6/drivers/net/niu.c	2009-08-18 15:31:34.000000000 +0200
+++ 2.6.31-rc6-build-bug-on/drivers/net/niu.c	2009-08-17 15:21:11.000000000 +0200
@@ -5615,7 +5615,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.31-rc6/include/linux/gfp.h	2009-08-18 15:31:54.000000000 +0200
+++ 2.6.31-rc6-build-bug-on/include/linux/gfp.h	2009-08-17 15:21:11.000000000 +0200
@@ -220,7 +220,7 @@ static inline enum zone_type gfp_zone(gf
 					 ((1 << ZONES_SHIFT) - 1);
 
 	if (__builtin_constant_p(bit))
-		BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
+		MAYBE_BUILD_BUG_ON((GFP_ZONE_BAD >> bit) & 1);
 	else {
 #ifdef CONFIG_DEBUG_VM
 		BUG_ON((GFP_ZONE_BAD >> bit) & 1);
--- linux-2.6.31-rc6/include/linux/kernel.h	2009-08-18 15:31:55.000000000 +0200
+++ 2.6.31-rc6-build-bug-on/include/linux/kernel.h	2009-08-17 15:21:11.000000000 +0200
@@ -675,13 +675,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 *)sizeof(struct { int:-!!(e); }))
 
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)
--- linux-2.6.31-rc6/include/linux/kmemcheck.h	2009-08-18 15:31:55.000000000 +0200
+++ 2.6.31-rc6-build-bug-on/include/linux/kmemcheck.h	2009-08-17 15:21:11.000000000 +0200
@@ -140,7 +140,7 @@ static inline void kmemcheck_mark_initia
 	do if (ptr) {							\
 		int _n = (long) &((ptr)->name##_end)			\
 			- (long) &((ptr)->name##_begin);		\
-		BUILD_BUG_ON(_n < 0);					\
+		MAYBE_BUILD_BUG_ON(_n < 0);				\
 									\
 		kmemcheck_mark_initialized(&((ptr)->name##_begin), _n);	\
 	} while (0)
--- linux-2.6.31-rc6/include/linux/virtio_config.h	2009-08-18 15:31:55.000000000 +0200
+++ 2.6.31-rc6-build-bug-on/include/linux/virtio_config.h	2009-08-17 15:21:11.000000000 +0200
@@ -109,8 +109,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);
 
 	if (fbit < VIRTIO_TRANSPORT_F_START)
 		virtio_check_driver_offered_feature(vdev, fbit);

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[Andrew Morton]

On Tue, 18 Aug 2009 16:59:25 +0100
"Jan Beulich" <JBeulich@novell.com> wrote:

> gcc permitting variable length arrays makes the current construct
> used for BUILD_BUG_ON() useless, as that doesn't produce any diagnostic
> if the controlling expression isn't really constant. Instead, this
> patch makes it so that a bit field gets used here. Consequently, those
> uses where the condition isn't really constant now also need fixing.
> 
> Note that in the gfp.h, kmemcheck.h, and virtio_config.h cases
> 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.

sparc64:

In file included from arch/sparc/kernel/vio.c:17:
/usr/src/devel/arch/sparc/include/asm/vio.h: In function `vio_dring_avail':
/usr/src/devel/arch/sparc/include/asm/vio.h:261: error: bit-field `<anonymous>' width not an integer constant

static inline u32 vio_dring_avail(struct vio_dring_state *dr,
				  unsigned int ring_size)
{
	BUILD_BUG_ON(!is_power_of_2(ring_size));

	return (dr->pending -
		((dr->prod - dr->cons) & (ring_size - 1)));
}

changing it to MAYBE_BUILD_BUG_ON seems to have fixed it.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 17 Sep 2009 16:24:37 -0700

> sparc64:
> 
> In file included from arch/sparc/kernel/vio.c:17:
> /usr/src/devel/arch/sparc/include/asm/vio.h: In function `vio_dring_avail':
> /usr/src/devel/arch/sparc/include/asm/vio.h:261: error: bit-field `<anonymous>' width not an integer constant
> 
> static inline u32 vio_dring_avail(struct vio_dring_state *dr,
> 				  unsigned int ring_size)
> {
> 	BUILD_BUG_ON(!is_power_of_2(ring_size));
> 
> 	return (dr->pending -
> 		((dr->prod - dr->cons) & (ring_size - 1)));
> }
> 
> changing it to MAYBE_BUILD_BUG_ON seems to have fixed it.

That's completely bogus.

First of all, arch/sparc/kernel/vio.c never calls this function
so it should never be evaluated.

Second of all, all the places that do call this function only
pass pure constants as the 'ring_size' parameter.

drivers/block/sunvdc.c:

static inline u32 vdc_tx_dring_avail(struct vio_dring_state *dr)
{
	return vio_dring_avail(dr, VDC_TX_RING_SIZE);
}

drivers/net/sunvnet.c:

static inline u32 vnet_tx_dring_avail(struct vio_dring_state *dr)
{
	return vio_dring_avail(dr, VNET_TX_RING_SIZE);
}

Making this MAYBE_BUILD_BUG_ON shouldn't be necessary.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[Andrew Morton]

On Thu, 17 Sep 2009 17:01:56 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 17 Sep 2009 16:24:37 -0700
> 
> > sparc64:
> > 
> > In file included from arch/sparc/kernel/vio.c:17:
> > /usr/src/devel/arch/sparc/include/asm/vio.h: In function `vio_dring_avail':
> > /usr/src/devel/arch/sparc/include/asm/vio.h:261: error: bit-field `<anonymous>' width not an integer constant
> > 
> > static inline u32 vio_dring_avail(struct vio_dring_state *dr,
> > 				  unsigned int ring_size)
> > {
> > 	BUILD_BUG_ON(!is_power_of_2(ring_size));
> > 
> > 	return (dr->pending -
> > 		((dr->prod - dr->cons) & (ring_size - 1)));
> > }
> > 
> > changing it to MAYBE_BUILD_BUG_ON seems to have fixed it.
> 
> That's completely bogus.
> 
> First of all, arch/sparc/kernel/vio.c never calls this function
> so it should never be evaluated.
> 
> Second of all, all the places that do call this function only
> pass pure constants as the 'ring_size' parameter.
> 
> drivers/block/sunvdc.c:
> 
> static inline u32 vdc_tx_dring_avail(struct vio_dring_state *dr)
> {
> 	return vio_dring_avail(dr, VDC_TX_RING_SIZE);
> }
> 
> drivers/net/sunvnet.c:
> 
> static inline u32 vnet_tx_dring_avail(struct vio_dring_state *dr)
> {
> 	return vio_dring_avail(dr, VNET_TX_RING_SIZE);
> }
> 
> Making this MAYBE_BUILD_BUG_ON shouldn't be necessary.

There's a shortcoming in the current BUILD_BUG_ON() - it silently does
nothing if passed a non-constant arg.

I suspect that in the 2.6.31 code, that BUILD_BUG_ON() just does
nothing at all, and that Jan's patch is now exposing this.  It might be
compiler-version dependent too.


<tests it>

Yup, on base 2.6.31, this:

--- a/arch/sparc/kernel/vio.c~a
+++ a/arch/sparc/kernel/vio.c
@@ -23,6 +23,8 @@ static const struct vio_device_id *vio_m
 	const char *type, *compat;
 	int len;
 
+	vio_dring_avail(NULL, 33);
+
 	type = dev->type;
 	compat = dev->compat;
 	len = dev->compat_len;
_

compiles without error with gcc-3.4.5.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[Andrew Morton]

On Thu, 17 Sep 2009 17:15:04 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> There's a shortcoming in the current BUILD_BUG_ON() - it silently does
> nothing if passed a non-constant arg.
> 
> I suspect that in the 2.6.31 code, that BUILD_BUG_ON() just does
> nothing at all, and that Jan's patch is now exposing this.  It might be
> compiler-version dependent too.
> 
> 
> <tests it>
> 
> Yup, on base 2.6.31, this:
> 
> --- a/arch/sparc/kernel/vio.c~a
> +++ a/arch/sparc/kernel/vio.c
> @@ -23,6 +23,8 @@ static const struct vio_device_id *vio_m
>  	const char *type, *compat;
>  	int len;
>  
> +	vio_dring_avail(NULL, 33);
> +
>  	type = dev->type;
>  	compat = dev->compat;
>  	len = dev->compat_len;
> _
> 
> compiles without error with gcc-3.4.5.

And I can't immediately find a way to make any compile-time error occur here, with
or without Jan's patch.  hm.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 17 Sep 2009 17:15:04 -0700

> There's a shortcoming in the current BUILD_BUG_ON() - it silently does
> nothing if passed a non-constant arg.
> 
> I suspect that in the 2.6.31 code, that BUILD_BUG_ON() just does
> nothing at all, and that Jan's patch is now exposing this.  It might be
> compiler-version dependent too.
> 
> 
> <tests it>
> 
> Yup, on base 2.6.31, this:

Ok, I'll have to either change this function to a macro or
get rid of the check.

Thanks.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 17 Sep 2009 17:31:54 -0700 (PDT)

> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 17 Sep 2009 17:15:04 -0700
> 
>> There's a shortcoming in the current BUILD_BUG_ON() - it silently does
>> nothing if passed a non-constant arg.
>> 
>> I suspect that in the 2.6.31 code, that BUILD_BUG_ON() just does
>> nothing at all, and that Jan's patch is now exposing this.  It might be
>> compiler-version dependent too.
>> 
>> 
>> <tests it>
>> 
>> Yup, on base 2.6.31, this:
> 
> Ok, I'll have to either change this function to a macro or
> get rid of the check.

I can't even get GCC to see the constant evaluated by is_power_of_2().

I give up, I'll just remove the BUILD_BUG_ON() entirely.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[Rusty Russell]

On Wed, 19 Aug 2009 01:29:25 am Jan Beulich wrote:
> Note that in the gfp.h, kmemcheck.h, and virtio_config.h cases
> 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.

How annoying.  A gcc build assert extension would be great here?

Thanks,
Rusty.

Re: [PATCH] fix BUILD_BUG_ON() and a couple of bogus uses of it

[Rusty Russell]

On Wed, 19 Aug 2009 01:29:25 am Jan Beulich wrote:
> gcc permitting variable length arrays makes the current construct
> used for BUILD_BUG_ON() useless, as that doesn't produce any diagnostic
> if the controlling expression isn't really constant. Instead, this
> patch makes it so that a bit field gets used here. Consequently, those
> uses where the condition isn't really constant now also need fixing.
> 
> Note that in the gfp.h, kmemcheck.h, and virtio_config.h cases
> 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>

We used to use an undefined symbol here; diagnostics are worse but it catches
more stuff.

Perhaps a hybrid is the way to go?

#ifndef __OPTIMIZE__
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
#else
/* If it's a constant, catch it at compile time, otherwise at link time. */
extern int __build_bug_on_failed;
#define BUILD_BUG_ON(condition) \
	do { 								\
		((void)sizeof(char[1 - 2*!!(condition)]));		\
		if (condition) __build_bug_on_failed = 1;		\
	} while(0)
#endif

Thanks,
Rusty.