BUILD_BUG_ON() should not use array type

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

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

Signed-off-by Jan Beulich <[email protected]>

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

drivers/net/niu.c +1 -1

@@ -5144,7 +5144,7 @@ static void niu_init_tx_mac(struct niu *
 	/* The XMAC_MIN register only accepts values for TX min which
 	 * have the low 3 bits cleared.
 	 */
-	BUILD_BUG_ON(min & 0x7);
+	BUG_ON(min & 0x7);
 
 	if (np->flags & NIU_FLAGS_XMAC)
 		niu_init_tx_xmac(np, min, max);

include/linux/kernel.h +0 -0

 /* Trap pasters of __FUNCTION__ at compile-time */
 #define __FUNCTION__ (__func__)

include/linux/virtio_config.h +1 -2

@@ -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);

include/linux/kernel.h struct sysinfo { · line 474

Quoted 2 lines

include/linux/kernel.h +2 -0

+/* Force a compilation error if condition is constant and true */
+#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))

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

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

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

It's tempting to just zap the one callsite and not add this at all,

include/linux/kernel.h struct sysinfo { · line 474

Quoted 11 lines

include/linux/kernel.h +4 -1

@@ -468,13 +468,17 @@ struct sysinfo {
 };
 
 /* Force a compilation error if condition is true */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUILD_BUG_ON(condition) ((void)BUILD_BUG_ON_ZERO(condition))
+
+/* Force a compilation error if condition is constant and true */
+#define MAYBE_BUILD_BUG_ON(cond) ((void)sizeof(char[1 - 2 * !!(cond)]))

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

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

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

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

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

include/linux/kernel.h struct sysinfo { · line 481

Quoted 7 lines

include/linux/kernel.h +2 -1

 /* Force a compilation error if condition is true, but also produce a
    result (of value 0 and type size_t), so the expression can be used
    e.g. in a structure initializer (or where-ever else comma expressions
    aren't permitted). */
-#define BUILD_BUG_ON_ZERO(e) (sizeof(char[1 - 2 * !!(e)]) - 1)
+#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
+#define BUILD_BUG_ON_NULL(e) ((void *)BUILD_BUG_ON_ZERO(e))

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