[PATCHSET 0/5] BUILD_BUG_ON: error on none-const expressions

[PATCHSET 0/5] BUILD_BUG_ON: error on none-const expressions

[Boaz Harrosh]

Submitted a patchset to fix BUILD_BUG_ON to no longer let through
none compile-time constant expressions. This was debated a few
times on LKML. The final solution is as proposed by Rusty Russell,
which produces all expected results in my tests.
[see: http://www.spinics.net/lists/kernel/msg772904.html]

[PATCH 1/5] debug, x86: move BUILD_BUG_ON() ARRAY_SIZE and __FUNCTION__
  Ingo this is your patch. Please verify?

[PATCH 2/5] net/niu: Fix none-const BUILD_BUG_ON usage
[PATCH 3/5] virtio: Fix none-const BUILD_BUG_ON usage
[PATCH 4/5] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON
  These three need maintainers approval. I hope I did not forget
  anyone

[PATCH 5/5] debug: BUILD_BUG_ON: error on none-const expressions
  Finally after all call sites are fixed this can go in.
  (Rusty this one is From: you)

I have only compiled ARCH=x86 (64/32) allmodconfig. So this might
break on other ARCHs. It should spend a night in Linux-Next to expose
these places.

Thanks
Boaz

[PATCH] debug, x86: move BUILD_BUG_ON() ARRAY_SIZE and __FUNCTION__

[Boaz Harrosh]

From: Ingo Molnar <mingo@elte.hu>

move BUILD_BUG_ON variants, ARRAY_SIZE and __FUNCTION__
definitions from kernel.h to compiler.h.

Besides being the correct location for such trivial wrappers around
compiler functionality, this also allows the removal of duplicate
definitions from arch/x86/boot/boot.h.

[ boot.h cannot just include kernel.h to pick up the new definitions,
  as it is also built into user-space utilities on the host system. ]

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 arch/x86/boot/boot.h     |    5 -----
 include/linux/compiler.h |   14 ++++++++++++++
 include/linux/kernel.h   |   14 --------------
 3 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index cc0ef13..f09b79a 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -27,11 +27,6 @@
 #include "bitops.h"
 #include <asm/cpufeature.h>
 
-/* Useful macros */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
-#define ARRAY_SIZE(x) (sizeof(x) / sizeof(*(x)))
-
 extern struct setup_header hdr;
 extern struct boot_params boot_params;
 
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index c8bd2da..90fa975 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -194,4 +194,18 @@ extern void __chk_io_ptr(const volatile void __iomem *);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
+/* Force a compilation error if condition is true */
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+
+/* 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)
+
+/* Trap pasters of __FUNCTION__ at compile-time */
+#define __FUNCTION__ (__func__)
+
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
+
 #endif /* __LINUX_COMPILER_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2651f80..29ca10d 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -42,8 +42,6 @@ extern const char linux_proc_banner[];
 #define PTR_ALIGN(p, a)		((typeof(p))ALIGN((unsigned long)(p), (a)))
 #define IS_ALIGNED(x, a)		(((x) & ((typeof(x))(a) - 1)) == 0)
 
-#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
-
 #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
 #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
 #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
@@ -467,18 +465,6 @@ struct sysinfo {
 	char _f[20-2*sizeof(long)-sizeof(int)];	/* Padding: libc5 uses this.. */
 };
 
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
-/* 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)
-
-/* Trap pasters of __FUNCTION__ at compile-time */
-#define __FUNCTION__ (__func__)
-
 /* This helps us to avoid #ifdef CONFIG_NUMA */
 #ifdef CONFIG_NUMA
 #define NUMA_BUILD 1

[PATCH 2/5] net/niu: Fix none-const BUILD_BUG_ON usage

[Boaz Harrosh]

It would be easy to constify the expression but I have removed
the BUILD_BUG_ON. (Left the useful comment though). Since
it has no point here, a BUILD_BUG_ON is used when two
inter-dependent but separate pieces of code must match. which
is not the case here.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: David S. Miller <davem@davemloft.net>
---
 drivers/net/niu.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index e4765b7..b517d0c 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -5135,17 +5135,15 @@ static void niu_init_tx_mac(struct niu *np)
 {
 	u64 min, max;
 
+	/* NOTE: XMAC_MIN register only accepts values for TX min which
+	 * have the low 3 bits cleared.
+	 */
 	min = 64;
 	if (np->dev->mtu > ETH_DATA_LEN)
 		max = 9216;
 	else
 		max = 1522;
 
-	/* The XMAC_MIN register only accepts values for TX min which
-	 * have the low 3 bits cleared.
-	 */
-	BUILD_BUG_ON(min & 0x7);
-
 	if (np->flags & NIU_FLAGS_XMAC)
 		niu_init_tx_xmac(np, min, max);
 	else
-- 
1.5.6.rc1.5.gadf6

[PATCH 3/5] virtio: Fix none-const BUILD_BUG_ON usage

[Boaz Harrosh]

BUILD_BUG_ON can not be used cross inline parametrization boundary.
I've put the BUILD_BUG_ON(_ZERO) in a macro outside of the inline
definition. (So it is in the caller scope).

NOTE to Rusty:
  If I remove the "__builtin_constant_p ?" part then code compiles
  just fine, because all call sights of virtio_has_feature() use
  constants for fbit. But it seems like it was not intended to be
  forced.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/virtio_config.h |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index bf8ec28..d9402d8 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -92,17 +92,19 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev,
  * @vdev: the device
  * @fbit: the feature bit
  */
-static inline bool virtio_has_feature(const struct virtio_device *vdev,
+static inline bool __virtio_has_feature(const struct virtio_device *vdev,
 				      unsigned int fbit)
 {
-	/* Did you forget to fix assumptions on max features? */
-	if (__builtin_constant_p(fbit))
-		BUILD_BUG_ON(fbit >= 32);
-
 	virtio_check_driver_offered_feature(vdev, fbit);
 	return test_bit(fbit, vdev->features);
 }
 
+/* Did you forget to fix assumptions on max features? */
+#define virtio_has_feature(vdev, fbit) \
+	((__builtin_constant_p(fbit) ? BUILD_BUG_ON_ZERO(fbit >= 32) : 0) + \
+	__virtio_has_feature(vdev, fbit))
+	
+
 /**
  * virtio_config_val - look for a feature and get a virtio config entry.
  * @vdev: the virtio device
-- 
1.5.6.rc1.5.gadf6

[PATCH 4/5] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Boaz Harrosh]

A "Set" of the sign-bit in an "&" operation causes a compiler warning.
Make calculations unsigned.

[ The warning was masked by the use of (void)() cast in the old
  BUILD_BUG_ON() ]

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
TO: Ivo van Doorn <IvDoorn@gmail.com>
TO: John W. Linville <linville@tuxdriver.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
index 7e88ce5..c0e8706 100644
--- a/drivers/net/wireless/rt2x00/rt2x00reg.h
+++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
@@ -134,8 +134,8 @@ struct rt2x00_field32 {
  * Note that we cannot use the is_power_of_2() function since this
  * check must be done at compile-time.
  */
-#define is_power_of_two(x)	( !((x) & ((x)-1)) )
-#define low_bit_mask(x)		( ((x)-1) & ~(x) )
+#define is_power_of_two(x)	( !((unsigned)(x) & ((x)-1)) )
+#define low_bit_mask(x)		( ((unsigned)(x)-1) & ~(x) )
 #define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
 
 /*

Re: [PATCH 4/5] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Ivo Van Doorn]

On Mon, Sep 1, 2008 at 3:21 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>
> A "Set" of the sign-bit in an "&" operation causes a compiler warning.
> Make calculations unsigned.
>
> [ The warning was masked by the use of (void)() cast in the old
>  BUILD_BUG_ON() ]
>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> TO: Ivo van Doorn <IvDoorn@gmail.com>
> TO: John W. Linville <linville@tuxdriver.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..c0e8706 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -134,8 +134,8 @@ struct rt2x00_field32 {
>  * Note that we cannot use the is_power_of_2() function since this
>  * check must be done at compile-time.
>  */
> -#define is_power_of_two(x)     ( !((x) & ((x)-1)) )
> -#define low_bit_mask(x)                ( ((x)-1) & ~(x) )
> +#define is_power_of_two(x)     ( !((unsigned)(x) & ((x)-1)) )
> +#define low_bit_mask(x)                ( ((unsigned)(x)-1) & ~(x) )
>  #define is_valid_mask(x)       is_power_of_two(1 + (x) + low_bit_mask(x))

Could the patch not become a lot easier (and perhaps cleaner) when the
only change is:
#define is_valid_mask(x)       is_power_of_two(1 + (x) +
low_bit_mask((unsigned)x))
that way all instances of x will be cast to unsigned...

Ivo

Re: [PATCH 4/5] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Benny Halevy]

On Sep. 01, 2008, 16:21 +0300, Boaz Harrosh <bharrosh@panasas.com> wrote:
> A "Set" of the sign-bit in an "&" operation causes a compiler warning.
> Make calculations unsigned.
> 
> [ The warning was masked by the use of (void)() cast in the old
>   BUILD_BUG_ON() ]
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> TO: Ivo van Doorn <IvDoorn@gmail.com>
> TO: John W. Linville <linville@tuxdriver.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..c0e8706 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -134,8 +134,8 @@ struct rt2x00_field32 {
>   * Note that we cannot use the is_power_of_2() function since this
>   * check must be done at compile-time.
>   */
> -#define is_power_of_two(x)	( !((x) & ((x)-1)) )
> -#define low_bit_mask(x)		( ((x)-1) & ~(x) )
> +#define is_power_of_two(x)	( !((unsigned)(x) & ((x)-1)) )
> +#define low_bit_mask(x)		( ((unsigned)(x)-1) & ~(x) )

Why not unsigned long?

Benny

>  #define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
>  
>  /*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

From: Rusty Russell <rusty@rustcorp.com.au>

Fix BUILD_BUG_ON to not silently drop non-compile-time
visible expressions.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Theodore Tso <tytso@mit.edu>
CC: Linus Torvalds <torvalds@linux-foundation.org>
CC: Jan Beulich <jbeulich@novell.com>
CC: David S. Miller <davem@davemloft.net>
CC: Ivo van Doorn <IvDoorn@gmail.com>
CC: John W. Linville <linville@tuxdriver.com>
---
 include/linux/compiler.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 90fa975..f677ab9 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -195,7 +195,10 @@ extern void __chk_io_ptr(const volatile void __iomem *);
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
 /* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUILD_BUG_ON(condition)					  \
+do {									  \
+	static struct { char arr[1 - 2*!!(condition)]; } 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
-- 
1.5.6.rc1.5.gadf6

[PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Boaz Harrosh]

A "Set" of a sign-bit in an "&" operation causes a compiler warning.
Make calculations unsigned.

[ The warning was masked by the use of (void)() cast in the old
  BUILD_BUG_ON() ]

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
TO: Ivo van Doorn <IvDoorn@gmail.com>
TO: John W. Linville <linville@tuxdriver.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
index 7e88ce5..e71b793 100644
--- a/drivers/net/wireless/rt2x00/rt2x00reg.h
+++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
@@ -136,8 +136,8 @@ struct rt2x00_field32 {
  */
 #define is_power_of_two(x)	( !((x) & ((x)-1)) )
 #define low_bit_mask(x)		( ((x)-1) & ~(x) )
-#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
-
+#define is_valid_mask(x)	is_power_of_two(1 + (x) + \
+					low_bit_mask((unsigned long)x))
 /*
  * Macro's to find first set bit in a variable.
  * These macro's behaves the same as the __ffs() function with
-- 
1.5.6.rc1.5.gadf6

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Jan Beulich]

>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 15:28 >>>
>--- a/include/linux/compiler.h
>+++ b/include/linux/compiler.h
>@@ -195,7 +195,10 @@ extern void __chk_io_ptr(const volatile void __iomem *);
> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
> 
> /* Force a compilation error if condition is true */
>-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
>+#define BUILD_BUG_ON(condition)					  \
>+do {									  \
>+	static struct { char arr[1 - 2*!!(condition)]; } 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 have to admit that I'm surprise this compiles: You replace an expression
with a statement, and hence you reduce the places where BUILD_BUG_ON()
can validly be used. Of course you could wrap the whole thing in ({}),
but I can't see why not to use a bit-field to achieve the intended effect.

Also, are you sure the compiler will eliminate the dead variable in all
cases?

Finally, using as common a variable as 'x' here seems dangerous, too:
What if somewhere x is #define-d to something more complex than a
simple identifier?

Jan

Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Ivo Van Doorn]

On Mon, Sep 1, 2008 at 3:44 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>
> A "Set" of a sign-bit in an "&" operation causes a compiler warning.
> Make calculations unsigned.
>
> [ The warning was masked by the use of (void)() cast in the old
>  BUILD_BUG_ON() ]
>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> TO: Ivo van Doorn <IvDoorn@gmail.com>
> TO: John W. Linville <linville@tuxdriver.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..e71b793 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
>  */
>  #define is_power_of_two(x)     ( !((x) & ((x)-1)) )
>  #define low_bit_mask(x)                ( ((x)-1) & ~(x) )
> -#define is_valid_mask(x)       is_power_of_two(1 + (x) + low_bit_mask(x))
> -
> +#define is_valid_mask(x)       is_power_of_two(1 + (x) + \
> +                                       low_bit_mask((unsigned long)x))

I know I typed it wrong, but you are missing the unsigned long cast for the
is_power_of_two argument here (Which could also be done in the
is_valid_mask() definition).

>  /*
>  * Macro's to find first set bit in a variable.
>  * These macro's behaves the same as the __ffs() function with
> --
> 1.5.6.rc1.5.gadf6
>
>
>

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 15:28 >>>
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -195,7 +195,10 @@ extern void __chk_io_ptr(const volatile void __iomem *);
>> #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
>>
>> /* Force a compilation error if condition is true */
>> -#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
>> +#define BUILD_BUG_ON(condition)					  \
>> +do {									  \
>> +	static struct { char arr[1 - 2*!!(condition)]; } 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 have to admit that I'm surprise this compiles: You replace an expression
> with a statement, and hence you reduce the places where BUILD_BUG_ON()
> can validly be used. 

it is only an expression because of the (void)() cast, which is what
I'm trying to avoid.

> Of course you could wrap the whole thing in ({}),

"do{}while(0)" is effectively an "{}" plus the added bonus
of demanding an ";" ;-)

> but I can't see why not to use a bit-field to achieve the intended effect.

Sure, I can also use my suggested enum construct.
(http://www.spinics.net/lists/kernel/msg772904.html)

But the thing I'm avoiding most is the (void)() cast, which makes the
optimizer very lazy. See:
[PATCH 4/5] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON
that has only popped out when I remove the (void)() cast.

> Also, are you sure the compiler will eliminate the dead variable in all
> cases?
> 
> Finally, using as common a variable as 'x' here seems dangerous, too:
> What if somewhere x is #define-d to something more complex than a
> simple identifier?

No it is scoped in a dead do{}while(0). What gets optimized out most
is the do nothing do{}while(0). The inside is just ignored.

> 
> Jan
> 

I will test for your approach, give me a sec ...

Boaz
 

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Jan Beulich]

>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 16:21 >>>
>> I have to admit that I'm surprise this compiles: You replace an expression
>> with a statement, and hence you reduce the places where BUILD_BUG_ON()
>> can validly be used. 
>
>it is only an expression because of the (void)() cast, which is what
>I'm trying to avoid.

No, sizeof() alone is an expression, too. Also, by using a statement you'll
have more problems with fixing BUILD_BUG_ON_ZERO(), which must be
an expression.

>> Of course you could wrap the whole thing in ({}),
>
>"do{}while(0)" is effectively an "{}" plus the added bonus
>of demanding an ";" ;-)

An expression likewise demands a terminating ; (or a continuation of the
expression, i.e. by using an operator)

>> Also, are you sure the compiler will eliminate the dead variable in all
>> cases?
>> 
>> Finally, using as common a variable as 'x' here seems dangerous, too:
>> What if somewhere x is #define-d to something more complex than a
>> simple identifier?
>
>No it is scoped in a dead do{}while(0). What gets optimized out most
>is the do nothing do{}while(0). The inside is just ignored.

I don't think compilers in general and gcc in particular work this way
(i.e. automatically throwing away everything included in a dead block).

Jan

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 16:21 >>>
>>> I have to admit that I'm surprise this compiles: You replace an expression
>>> with a statement, and hence you reduce the places where BUILD_BUG_ON()
>>> can validly be used. 
>> it is only an expression because of the (void)() cast, which is what
>> I'm trying to avoid.
> 
> No, sizeof() alone is an expression, too. 

Using only sizeof() produces tons of "expression has no effect" warning
all over the place.

> Also, by using a statement you'll
> have more problems with fixing BUILD_BUG_ON_ZERO(), which must be
> an expression.
> 

What is broken with my BUILD_BUG_ON_ZERO(). I tried all tests and
it works fine. Do you have a test with unwanted results?
(Actually it's the original one I have not touched it).

>>> Of course you could wrap the whole thing in ({}),
>> "do{}while(0)" is effectively an "{}" plus the added bonus
>> of demanding an ";" ;-)
> 
> An expression likewise demands a terminating ; (or a continuation of the
> expression, i.e. by using an operator)
> 

I was not criticizing your approach, I was commenting on:
"{}" vs. do{}while(0) 

>>> Also, are you sure the compiler will eliminate the dead variable in all
>>> cases?
>>>
>>> Finally, using as common a variable as 'x' here seems dangerous, too:
>>> What if somewhere x is #define-d to something more complex than a
>>> simple identifier?
>> No it is scoped in a dead do{}while(0). What gets optimized out most
>> is the do nothing do{}while(0). The inside is just ignored.
> 
> I don't think compilers in general and gcc in particular work this way
> (i.e. automatically throwing away everything included in a dead block).
> 

I've tested and nothing is produced, even without any optimization.
But I'm not an expert.

> Jan
> 

Boaz

Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Boaz Harrosh]

Ivo Van Doorn wrote:
> On Mon, Sep 1, 2008 at 3:44 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> A "Set" of a sign-bit in an "&" operation causes a compiler warning.
>> Make calculations unsigned.
>>
>> [ The warning was masked by the use of (void)() cast in the old
>>  BUILD_BUG_ON() ]
>>
>> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
>> TO: Ivo van Doorn <IvDoorn@gmail.com>
>> TO: John W. Linville <linville@tuxdriver.com>
>> CC: Ingo Molnar <mingo@elte.hu>
>> CC: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
>> index 7e88ce5..e71b793 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
>> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
>> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
>>  */
>>  #define is_power_of_two(x)     ( !((x) & ((x)-1)) )
>>  #define low_bit_mask(x)                ( ((x)-1) & ~(x) )
>> -#define is_valid_mask(x)       is_power_of_two(1 + (x) + low_bit_mask(x))
>> -
>> +#define is_valid_mask(x)       is_power_of_two(1 + (x) + \
>> +                                       low_bit_mask((unsigned long)x))
> 
> I know I typed it wrong, but you are missing the unsigned long cast for the
> is_power_of_two argument here (Which could also be done in the
> is_valid_mask() definition).
> 

I thought you suggested that on purpose. Since at the end it is all
one expression, the compiler propagates the cast to all participants.

Do you want that I send a fix for readability's sake?

>>  /*
>>  * Macro's to find first set bit in a variable.
>>  * These macro's behaves the same as the __ffs() function with
>> --
>> 1.5.6.rc1.5.gadf6
>>

Is below what you mean? but if so then perhaps my original one is
clearer. Note that it compiles and works as is.

---
diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
index 7e88ce5..e71b793 100644
--- a/drivers/net/wireless/rt2x00/rt2x00reg.h
+++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
@@ -136,8 +136,8 @@ struct rt2x00_field32 {
  */
 #define is_power_of_two(x)	( !((x) & ((x)-1)) )
 #define low_bit_mask(x)		( ((x)-1) & ~(x) )
-#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
-
+#define is_valid_mask(x)	is_power_of_two(1LU + (unsigned long)(x) + \
+					low_bit_mask((unsigned long)x))
 /*
  * Macro's to find first set bit in a variable.
  * These macro's behaves the same as the __ffs() function with
-- 1.5.6.rc1.5.gadf6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ 

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Jan Beulich]

>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 17:00 >>>
>What is broken with my BUILD_BUG_ON_ZERO(). I tried all tests and
>it works fine. Do you have a test with unwanted results?
>(Actually it's the original one I have not touched it).

That's the problem - it uses the same sizeof(char[]) approach, and hence
has the same problems that you just try to fix for BUILD_BUG_ON().

>>>> Of course you could wrap the whole thing in ({}),
>>> "do{}while(0)" is effectively an "{}" plus the added bonus
>>> of demanding an ";" ;-)
>> 
>> An expression likewise demands a terminating ; (or a continuation of the
>> expression, i.e. by using an operator)
>> 
>
>I was not criticizing your approach, I was commenting on:
>"{}" vs. do{}while(0) 

I think we're having some mis-understanding here: What I'm concerned
about is that you can't use statements where-ever expressions can be
used (the opposite is true, because expressions are statements). My
main concern is the limitation of its use inside macros, where one could
try to use comma expressions to combine multiple BUILD_BUG_ON()-s.

Jan

Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Ivo van Doorn]

On Monday 01 September 2008, Boaz Harrosh wrote:
> Ivo Van Doorn wrote:
> > On Mon, Sep 1, 2008 at 3:44 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> >> A "Set" of a sign-bit in an "&" operation causes a compiler warning.
> >> Make calculations unsigned.
> >>
> >> [ The warning was masked by the use of (void)() cast in the old
> >>  BUILD_BUG_ON() ]
> >>
> >> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> >> TO: Ivo van Doorn <IvDoorn@gmail.com>
> >> TO: John W. Linville <linville@tuxdriver.com>
> >> CC: Ingo Molnar <mingo@elte.hu>
> >> CC: Rusty Russell <rusty@rustcorp.com.au>
> >> ---
> >>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> index 7e88ce5..e71b793 100644
> >> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
> >>  */
> >>  #define is_power_of_two(x)     ( !((x) & ((x)-1)) )
> >>  #define low_bit_mask(x)                ( ((x)-1) & ~(x) )
> >> -#define is_valid_mask(x)       is_power_of_two(1 + (x) + low_bit_mask(x))
> >> -
> >> +#define is_valid_mask(x)       is_power_of_two(1 + (x) + \
> >> +                                       low_bit_mask((unsigned long)x))
> > 
> > I know I typed it wrong, but you are missing the unsigned long cast for the
> > is_power_of_two argument here (Which could also be done in the
> > is_valid_mask() definition).
> > 
> 
> I thought you suggested that on purpose. Since at the end it is all
> one expression, the compiler propagates the cast to all participants.
> 
> Do you want that I send a fix for readability's sake?

Yes, thanks.

> >>  /*
> >>  * Macro's to find first set bit in a variable.
> >>  * These macro's behaves the same as the __ffs() function with
> >> --
> >> 1.5.6.rc1.5.gadf6
> >>
> 
> Is below what you mean? but if so then perhaps my original one is
> clearer. Note that it compiles and works as is.

Below patch is good. Your original one would have had the same comment
as the original one, where the cast was only done on 1 x instead of both usages.

I think the below version is the most clear solution. :)

Ivo

> ---
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..e71b793 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
>   */
>  #define is_power_of_two(x)	( !((x) & ((x)-1)) )
>  #define low_bit_mask(x)		( ((x)-1) & ~(x) )
> -#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
> -
> +#define is_valid_mask(x)	is_power_of_two(1LU + (unsigned long)(x) + \
> +					low_bit_mask((unsigned long)x))
>  /*
>   * Macro's to find first set bit in a variable.
>   * These macro's behaves the same as the __ffs() function with
> -- 1.5.6.rc1.5.gadf6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ 
> 
> 

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 17:00 >>>
>> What is broken with my BUILD_BUG_ON_ZERO(). I tried all tests and
>> it works fine. Do you have a test with unwanted results?
>> (Actually it's the original one I have not touched it).
> 
> That's the problem - it uses the same sizeof(char[]) approach, and hence
> has the same problems that you just try to fix for BUILD_BUG_ON().
> 

No it does not have this problem. Have you tested it? 
I have! It works fine. (Complains on non-const expressions)

The problem with the original BUILD_BUG_ON was the 
(void)() cast and the interaction with the optimizer.

>>>>> Of course you could wrap the whole thing in ({}),
>>>> "do{}while(0)" is effectively an "{}" plus the added bonus
>>>> of demanding an ";" ;-)
>>> An expression likewise demands a terminating ; (or a continuation of the
>>> expression, i.e. by using an operator)
>>>
>> I was not criticizing your approach, I was commenting on:
>> "{}" vs. do{}while(0) 
> 
> I think we're having some mis-understanding here: What I'm concerned
> about is that you can't use statements where-ever expressions can be
> used (the opposite is true, because expressions are statements). My
> main concern is the limitation of its use inside macros, where one could
> try to use comma expressions to combine multiple BUILD_BUG_ON()-s.
> 

So it has not been used and it will not in the future.
In macros just use multiple statements separated by ";"
and surrounded by do{}while(0). To reach the same effect.

Note that BUG_ON is a statement and now BUILD_BUG_ON() is also.
for expressions use BUILD_BUG_ON_ZERO() 

> Jan
> 

OK I have tested with your version of BUILD_BUG_ON{,_ZERO} and I have
two problems with it.

[1] Problem number one is with this sequence of code:

(a)
static inline bool __virtio_has_feature(const struct virtio_device *vdev,
				      unsigned int fbit)
{
	virtio_check_driver_offered_feature(vdev, fbit);
	return test_bit(fbit, vdev->features);
}

/* Did you forget to fix assumptions on max features? */
#define virtio_has_feature(vdev, fbit) \
	((__builtin_constant_p(fbit) ? BUILD_BUG_ON_ZERO(fbit >= 32) : 0) + \
	__virtio_has_feature(vdev, fbit))

...
(b)
static inline int virtio_config_buf(struct virtio_device *vdev,
				    unsigned int fbit,
				    unsigned int offset,
				    void *buf, unsigned len)
{
	if (!virtio_has_feature(vdev, fbit))
		return -ENOENT;
...
(c)
	if (data && !virtio_has_feature(vdev, VIRTIO_NET_F_CSUM))
		return -ENOSYS;

I have not fount a way to code (a) in a way that actually works
for both (b) and (c). That is, good with const expressions and
bypassed with non-const. (See explanations below)

with your code:
> --- 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);

The MAYBE_BUILD_BUG_ON just gets ignored in all cases and
coding (c) as:

	if (data && !virtio_has_feature(vdev, 717))
		return -ENOSYS;

Will not complain at all.

[2] Problem number two is with this sequence of code:

#define is_power_of_two(x)	( !((x) & ((x)-1)) )
#define low_bit_mask(x)		( ((x)-1) & ~(x) )
#define is_valid_mask(x)	is_power_of_two(1 + (x) + \
					low_bit_mask(x))

#define FIELD_CHECK(__mask, __type)			\
	BUILD_BUG_ON(!__builtin_constant_p(__mask) ||	\
		     !(__mask) ||			\
		     !is_valid_mask(__mask) ||		\
		     (__mask) != (__type)(__mask))	\

#define FIELD8(__mask)				\
({						\
	FIELD_CHECK(__mask, u8);		\
	(struct rt2x00_field8) {		\
		compile_ffs8(__mask), (__mask)	\
	};					\
})

I expect the compiler to complain since I'm abusing the
"sign-bit" and need unsigned calculations, but the 
(void)() cast musks the all operation off. Actually the 
all FIELD_CHECK is ignored, I have tried illegal values
and they are let through.



>From what I understand the (void)() cast gets thrown away
by the optimizer very fast before it is fully generated hence
the silence in lots of "bad" cases. The BUILD_BUG_ON_ZERO does
not have this problem because it cannot be optimized out.
On the other hand with your bit field approach the constant-expression
is requested by the compiler very early and cannot be
"optimized out" earlier like in the problem [1] case.

I agree that this is all very ugly in theory. But in practice
it is simple, works(*), and lets me do things like problem[1]
that still produce results.

(*) By works I mean:
 Has no false-positives and no false-negatives. On two accounts:
 constness is enforced, condition is fully checked.

Please show example code that fails my bold (maybe even blunt)
statements, where you would expect one result and the compiler
does something else.

Boaz

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Jan Beulich]

>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 18:41 >>>
>Jan Beulich wrote:
>>>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 17:00 >>>
>>> What is broken with my BUILD_BUG_ON_ZERO(). I tried all tests and
>>> it works fine. Do you have a test with unwanted results?
>>> (Actually it's the original one I have not touched it).
>> 
>> That's the problem - it uses the same sizeof(char[]) approach, and hence
>> has the same problems that you just try to fix for BUILD_BUG_ON().
>> 
>
>No it does not have this problem. Have you tested it? 
>I have! It works fine. (Complains on non-const expressions)

For static variables, yes. But not for automatic variables and the like:

#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)

int test(int i) {
	int x = BUILD_BUG_ON_ZERO(i);

	return x;
}

You could argue that I could place a simple BUILD_BUG_ON() later in
the code, but that easily defeats the documentation purposes the
construct also has (my general position on this is that the check should
be in or immediately before the statement that depends on the
enforced restriction).

Jan

Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Boaz Harrosh]

Ivo van Doorn wrote:
> On Monday 01 September 2008, Boaz Harrosh wrote:
>> Ivo Van Doorn wrote:
>>
<snip>
>> Do you want that I send a fix for readability's sake?
> 
> Yes, thanks.
> 
<snip>
>> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
>> index 7e88ce5..e71b793 100644
>> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
>> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
>> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
>>   */
>>  #define is_power_of_two(x)	( !((x) & ((x)-1)) )
>>  #define low_bit_mask(x)		( ((x)-1) & ~(x) )
>> -#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
>> -
>> +#define is_valid_mask(x)	is_power_of_two(1LU + (unsigned long)(x) + \
>> +					low_bit_mask((unsigned long)x))
BTW
doing only:
-#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
+#define is_valid_mask(x)	is_power_of_two(1LU (x) + low_bit_mask(x))

Also fixes the problem. (By definition of C type promotion rules)

Should I do just That?

>>  /*
>>   * Macro's to find first set bit in a variable.
>>   * These macro's behaves the same as the __ffs() function with
>> -- 1.5.6.rc1.5.gadf6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ 
>>

This is my suggested new patch:
---
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Mon, 1 Sep 2008 14:47:19 +0300
Subject: [PATCH] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

A "Set" to a sign-bit in an "&" operation causes a compiler warning.
Make calculations unsigned.

[ The warning was masked by the old definition of BUILD_BUG_ON() ]

Also remove __builtin_constant_p from FIELD_CHECK since BUILD_BUG_ON
no longer permits non-const values.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
TO: Ivo van Doorn <IvDoorn@gmail.com>
TO: John W. Linville <linville@tuxdriver.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/wireless/rt2x00/rt2x00reg.h |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
index 7e88ce5..2ea7866 100644
--- a/drivers/net/wireless/rt2x00/rt2x00reg.h
+++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
@@ -136,7 +136,7 @@ struct rt2x00_field32 {
  */
 #define is_power_of_two(x)	( !((x) & ((x)-1)) )
 #define low_bit_mask(x)		( ((x)-1) & ~(x) )
-#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
+#define is_valid_mask(x)	is_power_of_two(1LU + (x) + low_bit_mask(x))
 
 /*
  * Macro's to find first set bit in a variable.
@@ -173,8 +173,7 @@ struct rt2x00_field32 {
  * does not exceed the given typelimit.
  */
 #define FIELD_CHECK(__mask, __type)			\
-	BUILD_BUG_ON(!__builtin_constant_p(__mask) ||	\
-		     !(__mask) ||			\
+	BUILD_BUG_ON(!(__mask) ||			\
 		     !is_valid_mask(__mask) ||		\
 		     (__mask) != (__type)(__mask))	\
 
-- 
1.5.6.rc1.5.gadf6

Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Ivo van Doorn]

> From: Boaz Harrosh <bharrosh@panasas.com>
> Date: Mon, 1 Sep 2008 14:47:19 +0300
> Subject: [PATCH] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON
> 
> A "Set" to a sign-bit in an "&" operation causes a compiler warning.
> Make calculations unsigned.
> 
> [ The warning was masked by the old definition of BUILD_BUG_ON() ]
> 
> Also remove __builtin_constant_p from FIELD_CHECK since BUILD_BUG_ON
> no longer permits non-const values.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> TO: Ivo van Doorn <IvDoorn@gmail.com>
> TO: John W. Linville <linville@tuxdriver.com>
> CC: Ingo Molnar <mingo@elte.hu>
> CC: Rusty Russell <rusty@rustcorp.com.au>

Thanks.

Acked-by: Ivo van Doorn <IvDoorn@gmail.com>

> ---
>  drivers/net/wireless/rt2x00/rt2x00reg.h |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..2ea7866 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -136,7 +136,7 @@ struct rt2x00_field32 {
>   */
>  #define is_power_of_two(x)	( !((x) & ((x)-1)) )
>  #define low_bit_mask(x)		( ((x)-1) & ~(x) )
> -#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
> +#define is_valid_mask(x)	is_power_of_two(1LU + (x) + low_bit_mask(x))
>  
>  /*
>   * Macro's to find first set bit in a variable.
> @@ -173,8 +173,7 @@ struct rt2x00_field32 {
>   * does not exceed the given typelimit.
>   */
>  #define FIELD_CHECK(__mask, __type)			\
> -	BUILD_BUG_ON(!__builtin_constant_p(__mask) ||	\
> -		     !(__mask) ||			\
> +	BUILD_BUG_ON(!(__mask) ||			\
>  		     !is_valid_mask(__mask) ||		\
>  		     (__mask) != (__type)(__mask))	\
>  

Re: [PATCH 5/5] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 18:41 >>>
>> Jan Beulich wrote:
>>>>>> Boaz Harrosh <bharrosh@panasas.com> 01.09.08 17:00 >>>
>>>> What is broken with my BUILD_BUG_ON_ZERO(). I tried all tests and
>>>> it works fine. Do you have a test with unwanted results?
>>>> (Actually it's the original one I have not touched it).
>>> That's the problem - it uses the same sizeof(char[]) approach, and hence
>>> has the same problems that you just try to fix for BUILD_BUG_ON().
>>>
>> No it does not have this problem. Have you tested it? 
>> I have! It works fine. (Complains on non-const expressions)
> 
> For static variables, yes. But not for automatic variables and the like:
> 
> #define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
> 
> int test(int i) {
> 	int x = BUILD_BUG_ON_ZERO(i);
> 
> 	return x;
> }
> 
> You could argue that I could place a simple BUILD_BUG_ON() later in
> the code, but that easily defeats the documentation purposes the
> construct also has (my general position on this is that the check should
> be in or immediately before the statement that depends on the
> enforced restriction).
> 
> Jan
> 

I was able to reproduce your problem now. 

I have made new patchset the final BUILD_BUG_ON_ZERO() I took from you.
But in BUILD_BUG_ON I use a slight variation that satisfies my wishes
and makes BUILD_BUG_ON more like BUG_ON in syntax.

I had to change virtio_has_feature() semantics, though, but I believe it's
not that bad.

Jan maybe you want to put your Signed-off-by: on the last patch?
Please review?

[ I'll post new version of patches that change as reply to the original
 patches ]

Boaz

[PATCH 3/5 ver2] virtio: Fix non-const BUILD_BUG_ON usage

[Boaz Harrosh]

In virtio_has_feature() BUILD_BUG_ON can not be used cross inline
parametrization boundary. I've put the BUILD_BUG_ON(_ZERO) in a
macro outside of the inline definition. (So it is in the caller scope).

However It is no longer legal to pass BUILD_BUG_ON a non-const
expression so users like virtio_config_buf() should caller
__virtio_has_feature() which does the actual code minus the
check.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 include/linux/virtio_config.h |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index bf8ec28..7accb1d 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -92,17 +92,18 @@ void virtio_check_driver_offered_feature(const struct virtio_device *vdev,
  * @vdev: the device
  * @fbit: the feature bit
  */
-static inline bool virtio_has_feature(const struct virtio_device *vdev,
+static inline bool __virtio_has_feature(const struct virtio_device *vdev,
 				      unsigned int fbit)
 {
-	/* Did you forget to fix assumptions on max features? */
-	if (__builtin_constant_p(fbit))
-		BUILD_BUG_ON(fbit >= 32);
-
 	virtio_check_driver_offered_feature(vdev, fbit);
 	return test_bit(fbit, vdev->features);
 }
 
+/* Did you forget to fix assumptions on max features? */
+#define virtio_has_feature(vdev, fbit) \
+	(BUILD_BUG_ON_ZERO(fbit >= 32) + __virtio_has_feature(vdev, fbit))
+	
+
 /**
  * virtio_config_val - look for a feature and get a virtio config entry.
  * @vdev: the virtio device
@@ -120,7 +121,7 @@ static inline int virtio_config_buf(struct virtio_device *vdev,
 				    unsigned int offset,
 				    void *buf, unsigned len)
 {
-	if (!virtio_has_feature(vdev, fbit))
+	if (!__virtio_has_feature(vdev, fbit))
 		return -ENOENT;
 
 	vdev->config->get(vdev, offset, buf, len);
-- 
1.5.6.rc1.5.gadf6

[PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Boaz Harrosh]

A "Set" to a sign-bit in an "&" operation causes a compiler warning.
Make calculations unsigned.

[ The warning was masked by the old definition of BUILD_BUG_ON() ]

Also remove __builtin_constant_p from FIELD_CHECK since BUILD_BUG_ON
no longer permits non-const values.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Acked-by: Ivo van Doorn <IvDoorn@gmail.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
---
 drivers/net/wireless/rt2x00/rt2x00reg.h |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
index 7e88ce5..2ea7866 100644
--- a/drivers/net/wireless/rt2x00/rt2x00reg.h
+++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
@@ -136,7 +136,7 @@ struct rt2x00_field32 {
  */
 #define is_power_of_two(x)	( !((x) & ((x)-1)) )
 #define low_bit_mask(x)		( ((x)-1) & ~(x) )
-#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
+#define is_valid_mask(x)	is_power_of_two(1LU + (x) + low_bit_mask(x))
 
 /*
  * Macro's to find first set bit in a variable.
@@ -173,8 +173,7 @@ struct rt2x00_field32 {
  * does not exceed the given typelimit.
  */
 #define FIELD_CHECK(__mask, __type)			\
-	BUILD_BUG_ON(!__builtin_constant_p(__mask) ||	\
-		     !(__mask) ||			\
+	BUILD_BUG_ON(!(__mask) ||			\
 		     !is_valid_mask(__mask) ||		\
 		     (__mask) != (__type)(__mask))	\
 
-- 
1.5.6.rc1.5.gadf6

[PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Fix BUILD_BUG_ON to not silently drop non-compile-time
visible expressions. It will now produce a compilation
error if so.

The code to BUILD_BUG_ON_ZERO was done by:
Jan Beulich <jbeulich@novell.com>

The code to BUILD_BUG_ON is a small variation to Jan's
code inspired by Rusty Russell. Which gives me better
compilation semantics, and makes BUILD_BUG_ON behave
more like BUG_ON.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
TO: Jan Beulich <jbeulich@novell.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Theodore Tso <tytso@mit.edu>
CC: David S. Miller <davem@davemloft.net>
---
 include/linux/compiler.h |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 90fa975..23dc269 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -194,14 +194,15 @@ extern void __chk_io_ptr(const volatile void __iomem *);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
 /* 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
+   e.g. in a structure initializer (or where-ever full statements
    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); }))
+
+/* Force a compilation error if condition is true */
+#define BUILD_BUG_ON(e) \
+	do { struct {int:-!!(e); } x __maybe_unused;} while(0)
 
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)
-- 
1.5.6.rc1.5.gadf6

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

oops some checkpatch love missing see new patch

Boaz

Re: [PATCH 5/5 ver3] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Fix BUILD_BUG_ON to not silently drop non-compile-time
visible expressions. It will now produce a compilation
error if so.

The code to BUILD_BUG_ON_ZERO was done by:
Jan Beulich <jbeulich@novell.com>

The code to BUILD_BUG_ON is a small variation to Jan's
code inspired by Rusty Russell. Which gives me better
compilations semantics, and makes BUILD_BUG_ON behave
more like BUG_ON.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
TO: Jan Beulich <jbeulich@novell.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: Rusty Russell <rusty@rustcorp.com.au>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Theodore Tso <tytso@mit.edu>
CC: David S. Miller <davem@davemloft.net>
---
 include/linux/compiler.h |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 90fa975..f882410 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -194,14 +194,15 @@ extern void __chk_io_ptr(const volatile void __iomem *);
  */
 #define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
 
-/* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
 /* 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
+   e.g. in a structure initializer (or where-ever full statements
    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); }))
+
+/* Force a compilation error if condition is true */
+#define BUILD_BUG_ON(e) \
+	do { struct {int: -!!(e); } x __maybe_unused; } while (0)
 
 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)
-- 
1.5.6.rc1.5.gadf6

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Jan Beulich]

>>> Boaz Harrosh <bharrosh@panasas.com> 02.09.08 17:57 >>>
>-#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
>+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>+
>+/* Force a compilation error if condition is true */
>+#define BUILD_BUG_ON(e) \
>+	do { struct {int:-!!(e); } x __maybe_unused;} while(0)
 
As indicated before, you should at the very least use __x as the variable
name.

But didn't you have reservations against using a bitfield here? Or was it
really just the void cast on the sizeof() that you disliked?

Jan

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@panasas.com> 02.09.08 17:57 >>>
>> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
>> +#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>> +
>> +/* Force a compilation error if condition is true */
>> +#define BUILD_BUG_ON(e) \
>> +	do { struct {int:-!!(e); } x __maybe_unused;} while(0)
>  
> As indicated before, you should at the very least use __x as the variable
> name.
> 

The name does not matter. The scope of x is confined to the do {} while()
and will not interfere with any local or global name.  

> But didn't you have reservations against using a bitfield here? Or was it
> really just the void cast on the sizeof() that you disliked?
> 

I like it it's fine. Also an added bonus is that on the good case it compiles
to a size-less structure in a code-less block so even the most stupid 
non-optimizing compiler will get it right. OK that could be done with 0-length
array too. But for consistency's sake I like it that both macros are the same.

> Jan
> 

I would like it if you sent your Signed-off-by: (or something) on this patch.
Thanks for your help
Boaz

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Jan Beulich]

>>> Boaz Harrosh <bharrosh@panasas.com> 03.09.08 10:57 >>>
>Jan Beulich wrote:
>>>>> Boaz Harrosh <bharrosh@panasas.com> 02.09.08 17:57 >>>
>>> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
>>> +#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>>> +
>>> +/* Force a compilation error if condition is true */
>>> +#define BUILD_BUG_ON(e) \
>>> +	do { struct {int:-!!(e); } x __maybe_unused;} while(0)
>>  
>> As indicated before, you should at the very least use __x as the variable
>> name.
>> 
>
>The name does not matter. The scope of x is confined to the do {} while()
>and will not interfere with any local or global name.  

I'm sorry to repeat this: If x is #define-d to anything but a simple identifier,
this will break no matter that it's in a private scope. The absence of any
identifier was a benefit of the sizeof() approach here.

Jan

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Jan Beulich wrote:
>>>> Boaz Harrosh <bharrosh@panasas.com> 03.09.08 10:57 >>>
>> Jan Beulich wrote:
>>>>>> Boaz Harrosh <bharrosh@panasas.com> 02.09.08 17:57 >>>
>>>> -#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
>>>> +#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
>>>> +
>>>> +/* Force a compilation error if condition is true */
>>>> +#define BUILD_BUG_ON(e) \
>>>> +	do { struct {int:-!!(e); } x __maybe_unused;} while(0)
>>>  
>>> As indicated before, you should at the very least use __x as the variable
>>> name.
>>>
>> The name does not matter. The scope of x is confined to the do {} while()
>> and will not interfere with any local or global name.  
> 
> I'm sorry to repeat this: If x is #define-d to anything but a simple identifier,
> this will break no matter that it's in a private scope. The absence of any
> identifier was a benefit of the sizeof() approach here.
> 
> Jan
> 

#defines are a shoot-in-the-leg, they should be CAPITAL letters and very
long and unique. If any one wants to #define x, they are welcome, it will
break the kernel even before my macro. No x in a private scope is fine
#define x is not, sorry ...

OK, I would change it, but I'm too lazy to do a new post just for that.

Boaz

Re: [PATCHSET 0/5] BUILD_BUG_ON: error on none-const expressions

[Ingo Molnar]

* Boaz Harrosh <bharrosh@panasas.com> wrote:

> Submitted a patchset to fix BUILD_BUG_ON to no longer let through
> none compile-time constant expressions. This was debated a few
> times on LKML. The final solution is as proposed by Rusty Russell,
> which produces all expected results in my tests.
> [see: http://www.spinics.net/lists/kernel/msg772904.html]
> 
> [PATCH 1/5] debug, x86: move BUILD_BUG_ON() ARRAY_SIZE and __FUNCTION__
>   Ingo this is your patch. Please verify?

yeah.

Acked-by: Ingo Molnar <mingo@elte.hu>

> [PATCH 5/5] debug: BUILD_BUG_ON: error on none-const expressions
>   Finally after all call sites are fixed this can go in.
>   (Rusty this one is From: you)

v3 looks good to me.

Andrew, are you picking this up? Looks like the perfect fit for -mm to 
me. I can drop the previous version from -tip so that there's no 
linux-next conflict.

	Ingo

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Rusty Russell]

On Wednesday 03 September 2008 01:57:31 Boaz Harrosh wrote:
> +#define BUILD_BUG_ON(e) \
> +       do { struct {int:-!!(e); } x __maybe_unused;} while(0)

Why did you hate the void cast again?  Simplest should 
be "(void)BUILD_BUG_ON_ZERO(e)".  But if not, it seems to me that it's 
cleaner to do:

#define BUILD_BUG_ON(e) \
	do { } while(BUILD_BUG_ON_ZERO(e))

No chance of the compiler emitting unused vars.

Cheers,
Rusty.

Re: [PATCH 5/5 ver2] debug: BUILD_BUG_ON: error on non-const expressions

[Boaz Harrosh]

Rusty Russell wrote:
> On Wednesday 03 September 2008 01:57:31 Boaz Harrosh wrote:
>> +#define BUILD_BUG_ON(e) \
>> +       do { struct {int:-!!(e); } x __maybe_unused;} while(0)
> 
> Why did you hate the void cast again?  Simplest should 
> be "(void)BUILD_BUG_ON_ZERO(e)".  But if not, it seems to me that it's 
> cleaner to do:
> 
> #define BUILD_BUG_ON(e) \
> 	do { } while(BUILD_BUG_ON_ZERO(e))
> 

I have not checked this exact variant, but the problems I had
is that wrong or non-const code was ignored by the compiler,
which is what I tried to solve.

> No chance of the compiler emitting unused vars.
> 

There is no unused vars because the result is a zero-sized
struct which will never emit any code.

> Cheers,
> Rusty.
> 

Is it that important? the code submitted does what is required
to the letter, should we spend more effort on this?

Boaz