login
Search
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 --
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__ ...
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
--
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
--
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
--
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 --
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
--
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 --
I thought you suggested that on purpose. Since at the end it is all
one expression, the compiler propagates the cast to all participants.
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/
--
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. :) --
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)
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
--
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
--
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
--
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
--
it is only an expression because of the (void)() cast, which is what
"do{}while(0)" is effectively an "{}" plus the added bonus
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
No it is scoped in a dead do{}while(0). What gets optimized out most
I will test for your approach, give me a sec ...
Boaz
--
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 likewise demands a terminating ; (or a continuation of the 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 --
Using only sizeof() produces tons of "expression has no effect" warning 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? I was not criticizing your approach, I was commenting on: I've tested and nothing is produced, even without any optimization. Boaz --
That's the problem - it uses the same sizeof(char[]) approach, and hence 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 --
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
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.
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)
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) ...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 --
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
--
oops some checkpatch love missing see new patch Boaz --
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 --
The name does not matter. The scope of x is confined to the do {} while()
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
I would like it if you sent your Signed-off-by: (or something) on this patch.
Thanks for your help
Boaz
--
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 --
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.
--
I have not checked this exact variant, but the problems I had is that wrong or non-const code was ignored by the compiler, There is no unused vars because the result is a zero-sized Is it that important? the code submitted does what is required to the letter, should we spend more effort on this? Boaz --
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
--
yeah. 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 --