From: Ingo Molnar <mingo@elte.hu>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>,
Alexey Dobriyan <adobriyan@gmail.com>,
torvalds@linuxfoundation.org, akpm@linuxfoundation.org,
linux-kernel@vger.kernel.org, Sam Ravnborg <sam@ravnborg.org>
Subject: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions
Date: Sun, 17 Aug 2008 12:32:41 +0200 [thread overview]
Message-ID: <20080817103241.GB21303@elte.hu> (raw)
In-Reply-To: <alpine.LFD.1.10.0808161305170.3324@nehalem.linux-foundation.org>
* Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Sat, 16 Aug 2008, Rusty Russell wrote:
> >
> > Interesting idea, but I've come to actually like the semantic explicitness of
> > BUILD_BUG_ON. There's a difference between "we should never get here"
> > and "this should never exist".
>
> Agreed. I think Alexey's patch is broken.
>
> The thing is, BUILD_BUG_ON() is a different thing. It says "this is a
> build error", while BUG_ON() says "this is an error if we reach it".
>
> Very different.
agreed.
There's one aspect of BUILD_BUG_ON() that is quite dangerous though: it
does not 'upgrade' into a runtime check if an expression is not
constant. And it does not warn either. So BUILD_BUG_ON() can degrade
into a no-op very silently, and that is inherently dangerous.
That aspect bit me once: i added a BUILD_BUG_ON() under the assumption
that it would catch a mis-sized virtual memory sizing detail in
arch/x86/, but it just remained silent.
To fix these problems i've added the two commits below to tip/core/debug
[one to extend BUILD_BUG_ON, one to clean up its location] - any
objections against that direction? I've started testing it through to
make sure we dont have any stale non-constant BUILD_BUG_ON() instances
around.
( Note, i have not changed BUILD_BUG_ON_ZERO() because that is used in
structure initializers so no comma expression can be used in them.
Such structure initializers wont allow non-constant expressions
anyway, so there's not much extra value in checking for that. )
( Note #2, BUILD_BUG_ON() had to remain a macro, so that
__builtin_constant_expression_p() can do its work. )
Ingo
>From f5b5d41dd51a31fe70e3a04fb80a3b90b84c6a4e Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 17 Aug 2008 11:58:58 +0200
Subject: [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
constant expressions get detected at build time via:
kernel/sched.c: In function ‘test':
kernel/sched.c:9187: error: size of array ‘type name' is negative
make[1]: *** [kernel/sched.o] Error 1
but non-constant expressions (for example BUILD_BUG_ON(variable)) simply
get discarded by the compiler - turning BUILD_BUG_ON() into a dangerous
construct.
So add another layer at the link level to detect such mishaps:
kernel/built-in.o: In function `test':
: undefined reference to `__BUILD_BUG_ON_non_constant'
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
include/linux/kernel.h | 18 ++++++++++++++++--
1 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 2651f80..36c841e 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -467,8 +467,22 @@ 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 [array index becomes
+ * negative], and a linker error if condition is not constant [non-defined
+ * variable is used as an array index]:
+ *
+ * ( The linker trick relies on gcc optimizing out a multiplication with
+ * constant zero - which should be reasonable enough. )
+ */
+extern unsigned int __BUILD_BUG_ON_non_constant;
+
+#define BUILD_BUG_ON(condition) \
+do { \
+ (void)sizeof(char[1 - 2*!!(condition)]); \
+ if (!__builtin_constant_p(condition)) \
+ __BUILD_BUG_ON_non_constant++; \
+} 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
>From 7c516ee411f38cffbd4ab09b089c210202f9bd0f Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Sun, 17 Aug 2008 12:18:01 +0200
Subject: [PATCH] debug, x86: move BUILD_BUG_ON() and __FUNCTION__
move BUILD_BUG_ON variants and the __FUNCTION__ definition from
kernel.h to compiler.h.
Besides being the correct location for such trivial wrappers around
compiler functionality, this also allows the removal of a duplicate
(and now slighly incompatible) definition of BUILD_BUG_ON from
arch/x86/boot/boot.h.
[ boot.h cannot just include kernel.h to pick up the new definition of
BUILD_BUG_ON(), as it is also built into user-space utilities on the
host system. ]
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
arch/x86/boot/boot.h | 3 ---
include/linux/compiler.h | 30 ++++++++++++++++++++++++++++++
include/linux/kernel.h | 26 --------------------------
3 files changed, 30 insertions(+), 29 deletions(-)
diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 616b804..f09b79a 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -27,9 +27,6 @@
#include "bitops.h"
#include <asm/cpufeature.h>
-/* Useful macros */
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
-
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..727862f 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -194,4 +194,34 @@ 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 [array index becomes
+ * negative], and a linker error if condition is not constant [non-defined
+ * variable is used as an array index]:
+ *
+ * ( The linker trick relies on gcc optimizing out a multiplication with
+ * constant zero - which should be reasonable enough. )
+ */
+#ifndef __ASSEMBLY__
+extern unsigned int __BUILD_BUG_ON_non_constant;
+#endif
+
+#define BUILD_BUG_ON(condition) \
+do { \
+ (void)sizeof(char[1 - 2*!!(condition)]); \
+ if (!__builtin_constant_p(condition)) \
+ __BUILD_BUG_ON_non_constant++; \
+} 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
+ * 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__)
+
#endif /* __LINUX_COMPILER_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 36c841e..1ceafa4 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -467,32 +467,6 @@ struct sysinfo {
char _f[20-2*sizeof(long)-sizeof(int)]; /* Padding: libc5 uses this.. */
};
-/*
- * Force a compilation error if condition is true [array index becomes
- * negative], and a linker error if condition is not constant [non-defined
- * variable is used as an array index]:
- *
- * ( The linker trick relies on gcc optimizing out a multiplication with
- * constant zero - which should be reasonable enough. )
- */
-extern unsigned int __BUILD_BUG_ON_non_constant;
-
-#define BUILD_BUG_ON(condition) \
-do { \
- (void)sizeof(char[1 - 2*!!(condition)]); \
- if (!__builtin_constant_p(condition)) \
- __BUILD_BUG_ON_non_constant++; \
-} 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
- 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
next prev parent reply other threads:[~2008-08-17 10:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-08-16 10:09 [PATCH] BUILD_BUG_ON sucks Alexey Dobriyan
2008-08-16 10:55 ` Rusty Russell
2008-08-16 20:07 ` Linus Torvalds
2008-08-17 10:32 ` Ingo Molnar [this message]
2008-08-17 16:56 ` [PATCH] debug: fix BUILD_BUG_ON() for non-constant expressions Linus Torvalds
2008-08-17 17:33 ` Ingo Molnar
2008-08-17 17:53 ` Ingo Molnar
2008-08-17 18:39 ` Linus Torvalds
2008-08-17 18:45 ` Ingo Molnar
2008-08-18 1:09 ` Rusty Russell
2008-08-18 7:54 ` Ingo Molnar
2008-08-18 9:55 ` Boaz Harrosh
2008-08-18 12:32 ` Boaz Harrosh
2008-08-19 13:34 ` Ingo Molnar
2008-08-19 16:33 ` Boaz Harrosh
2008-08-20 10:59 ` Ingo Molnar
2008-08-20 12:31 ` Boaz Harrosh
2008-08-20 12:39 ` adobriyan
2008-08-20 13:07 ` Boaz Harrosh
2008-08-21 12:17 ` Ingo Molnar
2008-08-25 1:19 ` Rusty Russell
2008-08-20 13:21 ` Boaz Harrosh
2008-08-16 17:46 ` [PATCH] BUILD_BUG_ON sucks Andrew Morton
2008-08-17 12:19 ` Theodore Tso
2008-08-17 16:33 ` Andrew Morton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20080817103241.GB21303@elte.hu \
--to=mingo@elte.hu \
--cc=adobriyan@gmail.com \
--cc=akpm@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rusty@rustcorp.com.au \
--cc=sam@ravnborg.org \
--cc=torvalds@linux-foundation.org \
--cc=torvalds@linuxfoundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).