include/linux/bug.h | 47 ++++++++++++++++++++++------------------
include/linux/compiler-gcc.h | 3 ++
include/linux/compiler-gcc3.h | 8 +++---
include/linux/compiler-gcc4.h | 28 ++++++++++++------------
include/linux/compiler.h | 32 +++++++++++++++++++++++++--
5 files changed, 76 insertions(+), 42 deletions(-)
Changes in v5:
o Move BUILD_BUG* guts to a new compiletime_assert() macro in compiler.h.
o Fix a double-evaluation problem.
o Fix another bad macro definition when __CHECKER__ defined.
Changes in v4:
o Squash a minor commit (per Borislav Petkov)
o Change some comment text (per Borislav Petkov)
o Add some acks
This patch set is a dependency of the generic red-black tree patch set, which
I have now split up into three smaller sets and is based off of linux-next.
The major aim of this patch set is to cleanup compiler-gcc*.h and improve
the manageability of of compiler features at various versions (when they
are broken, etc.), add some needed features to bug.h and clean that up as
well.
compiler-gcc*.h
o Introduce GCC_VERSION - (e.g., gcc 4.7.1 becomes 40701).
o Reorder all features based upon the version introduced (readability).
o Change all version checks to use GCC_VERSION.
o Remove redundant __linktime_error macro.
o Introduce compiletime_assert() macro.
bug.h
o Improve BUILD_BUG_ON(expr) - now generates compile-time error post-gcc-4.4
o Remove duplicate error messages in some cases.
o Fix double-evaluation problem in BUILD_BUG_ON.
o Fix bad macro definitions when using __CHECKER__.
o Introduce BUILD_BUG_ON_MSG and clean up the implementations of the
BUILD_BUG* macros.
This helps to keep the file from getting confusing, removes one
duplicate version check and should encourage future editors to put new
macros where they belong.
Signed-off-by: Daniel Santos <[email protected]>
Acked-by: David Rientjes <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
---
include/linux/compiler-gcc4.h | 20 +++++++++++---------
1 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 412bc6c..8914293 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -13,6 +13,10 @@
#define __must_check __attribute__((warn_unused_result))
#define __compiler_offsetof(a,b) __builtin_offsetof(a,b)
+#if __GNUC_MINOR__ > 0
+# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
+#endif
+
#if __GNUC_MINOR__ >= 3
/* Mark functions as cold. gcc will assume any path leading to a call
to them will be unlikely. This means a lot of manual unlikely()s
@@ -31,6 +35,12 @@
#define __linktime_error(message) __attribute__((__error__(message)))
+#ifndef __CHECKER__
+# define __compiletime_warning(message) __attribute__((warning(message)))
+# define __compiletime_error(message) __attribute__((error(message)))
+#endif /* __CHECKER__ */
+#endif /* __GNUC_MINOR__ >= 3 */
+
#if __GNUC_MINOR__ >= 5
/*
* Mark a position in code as unreachable. This can be used to
@@ -46,8 +56,7 @@
/* Mark a function definition as prohibited from being cloned. */
#define __noclone __attribute__((__noclone__))
-#endif
-#endif
+#endif /* __GNUC_MINOR__ >= 5 */
#if __GNUC_MINOR__ >= 6
/*
@@ -56,10 +65,3 @@
#define __visible __attribute__((externally_visible))
#endif
-#if __GNUC_MINOR__ > 0
-#define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
-#endif
-#if __GNUC_MINOR__ >= 3 && !defined(__CHECKER__)
-#define __compiletime_warning(message) __attribute__((warning(message)))
-#define __compiletime_error(message) __attribute__((error(message)))
-#endif
--
1.7.3.4
Using GCC_VERSION reduces complexity, is easier to read and is GCC's
recommended mechanism for doing version checks. (Just don't ask me why
they didn't define it in the first place.) This also makes it easy to
merge compiler-gcc{,3,4}.h should somebody want to.
Signed-off-by: Daniel Santos <[email protected]>
Acked-by: David Rientjes <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
---
include/linux/compiler-gcc3.h | 8 ++++----
include/linux/compiler-gcc4.h | 14 +++++++-------
2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/linux/compiler-gcc3.h b/include/linux/compiler-gcc3.h
index 37d4124..7d89feb 100644
--- a/include/linux/compiler-gcc3.h
+++ b/include/linux/compiler-gcc3.h
@@ -2,22 +2,22 @@
#error "Please don't include <linux/compiler-gcc3.h> directly, include <linux/compiler.h> instead."
#endif
-#if __GNUC_MINOR__ < 2
+#if GCC_VERSION < 30200
# error Sorry, your compiler is too old - please upgrade it.
#endif
-#if __GNUC_MINOR__ >= 3
+#if GCC_VERSION >= 30300
# define __used __attribute__((__used__))
#else
# define __used __attribute__((__unused__))
#endif
-#if __GNUC_MINOR__ >= 4
+#if GCC_VERSION >= 30400
#define __must_check __attribute__((warn_unused_result))
#endif
#ifdef CONFIG_GCOV_KERNEL
-# if __GNUC_MINOR__ < 4
+# if GCC_VERSION < 30400
# error "GCOV profiling support for gcc versions below 3.4 not included"
# endif /* __GNUC_MINOR__ */
#endif /* CONFIG_GCOV_KERNEL */
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 8914293..9755029 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -4,7 +4,7 @@
/* GCC 4.1.[01] miscompiles __weak */
#ifdef __KERNEL__
-# if __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
+# if GCC_VERSION >= 40100 && GCC_VERSION <= 40101
# error Your version of gcc miscompiles the __weak directive
# endif
#endif
@@ -13,11 +13,11 @@
#define __must_check __attribute__((warn_unused_result))
#define __compiler_offsetof(a,b) __builtin_offsetof(a,b)
-#if __GNUC_MINOR__ > 0
+#if GCC_VERSION >= 40100
# define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
#endif
-#if __GNUC_MINOR__ >= 3
+#if GCC_VERSION >= 40300
/* Mark functions as cold. gcc will assume any path leading to a call
to them will be unlikely. This means a lot of manual unlikely()s
are unnecessary now for any paths leading to the usual suspects
@@ -39,9 +39,9 @@
# define __compiletime_warning(message) __attribute__((warning(message)))
# define __compiletime_error(message) __attribute__((error(message)))
#endif /* __CHECKER__ */
-#endif /* __GNUC_MINOR__ >= 3 */
+#endif /* GCC_VERSION >= 40300 */
-#if __GNUC_MINOR__ >= 5
+#if GCC_VERSION >= 40500
/*
* Mark a position in code as unreachable. This can be used to
* suppress control flow warnings after asm blocks that transfer
@@ -56,9 +56,9 @@
/* Mark a function definition as prohibited from being cloned. */
#define __noclone __attribute__((__noclone__))
-#endif /* __GNUC_MINOR__ >= 5 */
+#endif /* GCC_VERSION >= 40500 */
-#if __GNUC_MINOR__ >= 6
+#if GCC_VERSION >= 40600
/*
* Tell the optimizer that something else uses this function or variable.
*/
--
1.7.3.4
When calling BUILD_BUG_ON in an optimized build using gcc 4.3 and later,
the condition will be evaulated twice, possibily with side-effects.
This patch eliminates that error.
Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/bug.h | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 1b2465d..ccd44ce 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -58,8 +58,9 @@ struct pt_regs;
extern int __build_bug_on_failed;
#define BUILD_BUG_ON(condition) \
do { \
- ((void)sizeof(char[1 - 2*!!(condition)])); \
- if (condition) __build_bug_on_failed = 1; \
+ bool __cond = !!(condition); \
+ ((void)sizeof(char[1 - 2*!!(__cond)])); \
+ if (__cond) __build_bug_on_failed = 1; \
} while(0)
#endif
--
1.7.3.4
Negative sized arrays wont create a compile-time error in some cases
starting with gcc 4.4 (e.g., inlined functions), but gcc 4.3 introduced
the error function attribute that will. This patch modifies
BUILD_BUG_ON to behave like BUILD_BUG already does, using the error
function attribute so that you don't have to build the entire kernel to
discover that you have a problem, and then enjoy trying to track it down
from a link-time error.
Also, we are only including asm/bug.h and then expecting that
linux/compiler.h will eventually be included to define __linktime_error
(used in BUILD_BUG_ON). This patch includes it directly for clarity and
to avoid the possibility of changes in <arch>/*/include/asm/bug.h being
changed or not including linux/compiler.h for some reason.
Signed-off-by: Daniel Santos <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
---
include/linux/bug.h | 32 +++++++++++++++++++-------------
1 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index ccd44ce..dd4f506 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -2,6 +2,7 @@
#define _LINUX_BUG_H
#include <asm/bug.h>
+#include <linux/compiler.h>
enum bug_trap_type {
BUG_TRAP_TYPE_NONE = 0,
@@ -42,25 +43,30 @@ struct pt_regs;
* @condition: the condition which the compiler should know is false.
*
* If you have some code which relies on certain constants being equal, or
- * other compile-time-evaluated condition, you should use BUILD_BUG_ON to
+ * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
* detect if someone changes it.
*
- * The implementation uses gcc's reluctance to create a negative array, but
- * gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
- * to inline functions). So as a fallback we use the optimizer; if it can't
- * prove the condition is false, it will cause a link error on the undefined
- * "__build_bug_on_failed". This error message can be harder to track down
- * though, hence the two different methods.
+ * The implementation uses gcc's reluctance to create a negative array, but gcc
+ * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
+ * inline functions). Luckily, in 4.3 they added the "error" function
+ * attribute just for this type of case. Thus, we use a negative sized array
+ * (should always create an error on gcc versions older than 4.4) and then call
+ * an undefined function with the error attribute (should always create an
+ * error on gcc 4.3 and later). If for some reason, neither creates a
+ * compile-time error, we'll still have a link-time error, which is harder to
+ * track down.
*/
#ifndef __OPTIMIZE__
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
#else
-extern int __build_bug_on_failed;
-#define BUILD_BUG_ON(condition) \
- do { \
- bool __cond = !!(condition); \
- ((void)sizeof(char[1 - 2*!!(__cond)])); \
- if (__cond) __build_bug_on_failed = 1; \
+#define BUILD_BUG_ON(condition) \
+ do { \
+ bool __cond = !!(condition); \
+ extern void __build_bug_on_failed(void) \
+ __compiletime_error("BUILD_BUG_ON failed"); \
+ if (__cond) \
+ __build_bug_on_failed(); \
+ ((void)sizeof(char[1 - 2*!!(__cond)])); \
} while(0)
#endif
--
1.7.3.4
Introduce compiletime_assert to compiler.h, which moves the details of
how to break a build and emit an error message for a specific compiler
to the headers where these details should be. Following the tradition of
the POSIX assert macro, compiletime_assert creates a build-time error
when the supplied condition is *false*.
Next, we add BUILD_BUG_ON_MSG to bug.h which simply wraps
compiletime_assert, inverting the logic, so that it fails when the
condition is *true*, consistient with the language "build bug on." This
macro allows you to specify the error message you want emitted when the
supplied condition is true.
Finally, we remove all other code from bug.h that mucks with these
details (BUILD_BUG & BUILD_BUG_ON), and have them all call
BUILD_BUG_ON_MSG. This not only reduces source code bloat, but also
prevents the possibility of code being changed for one macro and not for
the other (which was previously the case for BUILD_BUG and
BUILD_BUG_ON).
Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/bug.h | 28 +++++++++++++---------------
include/linux/compiler.h | 24 ++++++++++++++++++++++++
2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 125e744..31a1310 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -16,6 +16,7 @@ struct pt_regs;
#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
#define BUILD_BUG_ON_ZERO(e) (0)
#define BUILD_BUG_ON_NULL(e) ((void*)0)
+#define BUILD_BUG_ON_MSG(cond, msg) (0)
#define BUILD_BUG_ON(condition) (0)
#define BUILD_BUG() (0)
#else /* __CHECKER__ */
@@ -39,6 +40,15 @@ struct pt_regs;
#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
/**
+ * BUILD_BUG_ON_MSG - break compile if a condition is true & emit supplied
+ * error message.
+ * @condition: the condition which the compiler should know is false.
+ *
+ * See BUILD_BUG_ON for description.
+ */
+#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
+
+/**
* BUILD_BUG_ON - break compile if a condition is true.
* @condition: the condition which the compiler should know is false.
*
@@ -59,15 +69,8 @@ struct pt_regs;
#ifndef __OPTIMIZE__
#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
#else
-#define BUILD_BUG_ON(condition) \
- do { \
- bool __cond = !!(condition); \
- extern void __build_bug_on_failed(void) \
- __compiletime_error("BUILD_BUG_ON failed"); \
- if (__cond) \
- __build_bug_on_failed(); \
- __compiletime_error_fallback(__cond); \
- } while(0)
+#define BUILD_BUG_ON(condition) \
+ BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
#endif
/**
@@ -77,12 +80,7 @@ struct pt_regs;
* build time, you should use BUILD_BUG to detect if it is
* unexpectedly used.
*/
-#define BUILD_BUG() \
- do { \
- extern void __build_bug_failed(void) \
- __compiletime_error("BUILD_BUG failed");\
- __build_bug_failed(); \
- } while (0)
+#define BUILD_BUG() BUILD_BUG_ON_MSG(1, "BUILD_BUG failed")
#endif /* __CHECKER__ */
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index 8e5b9d5..57878f3 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -304,6 +304,30 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
# define __compiletime_error_fallback(condition) do { } while (0)
#endif
+#define __compiletime_assert(condition, msg, __func) \
+ do { \
+ bool __cond = !(condition); \
+ extern void __func(void) __compiletime_error(msg); \
+ if (__cond) \
+ __func(); \
+ __compiletime_error_fallback(__cond); \
+ } while (0)
+
+#define _compiletime_assert(condition, msg, __func) \
+ __compiletime_assert(condition, msg, __func)
+
+/**
+ * compiletime_assert - break build and emit msg if condition is false
+ * @condition: a compile-time constant condition to check
+ * @msg: a message to emit if condition is false
+ *
+ * In tradition of POSIX assert, this macro will break the build if the
+ * supplied condition is *false*, emitting the supplied error message if the
+ * compiler has support to do so.
+ */
+#define compiletime_assert(condition, msg) \
+ _compiletime_assert(condition, msg, __compiletime_assert_ ## __LINE__)
+
/*
* Prevent the compiler from merging or refetching accesses. The compiler
* is also forbidden from reordering successive instances of ACCESS_ONCE(),
--
1.7.3.4
Prior to the introduction of __attribute__((error("msg"))) in gcc 4.3,
creating compile-time errors required a little trickery.
BUILD_BUG{,_ON} uses this attribute when available to generate
compile-time errors, but also uses the negative-sized array trick for
older compilers, resulting in two error messages in some cases. The
reason it's "some" cases is that as of gcc 4.4, the negative-sized array
will not create an error in some situations, like inline functions.
This patch replaces the negative-sized array code with the new
__compiletime_error_fallback() macro which expands to the same thing
unless the the error attribute is available, in which case it expands to
do{}while(0), resulting in exactly one compile-time error on all
versions of gcc.
Note that we are not changing the negative-sized array code for the
unoptimized version of BUILD_BUG_ON, since it has the potential to catch
problems that would be disabled in later versions of gcc were
__compiletime_error_fallback used. The reason is that that an
unoptimized build can't always remove calls to an error-attributed
function call (like we are using) that should effectively become dead
code if it were optimized. However, using a negative-sized array with a
similar value will not result in an false-positive (error). The only
caveat being that it will also fail to catch valid conditions, which we
should be expecting in an unoptimized build anyway.
Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/bug.h | 2 +-
include/linux/compiler.h | 5 +++++
2 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index dd4f506..125e744 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -66,7 +66,7 @@ struct pt_regs;
__compiletime_error("BUILD_BUG_ON failed"); \
if (__cond) \
__build_bug_on_failed(); \
- ((void)sizeof(char[1 - 2*!!(__cond)])); \
+ __compiletime_error_fallback(__cond); \
} while(0)
#endif
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index cbf6d9d..8e5b9d5 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -298,7 +298,12 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
#endif
#ifndef __compiletime_error
# define __compiletime_error(message)
+# define __compiletime_error_fallback(condition) \
+ do { ((void)sizeof(char[1 - 2*!!(condition)])); } while (0)
+#else
+# define __compiletime_error_fallback(condition) do { } while (0)
#endif
+
/*
* Prevent the compiler from merging or refetching accesses. The compiler
* is also forbidden from reordering successive instances of ACCESS_ONCE(),
--
1.7.3.4
When __CHECKER__ is defined, we disable all of the BUILD_BUG.* macros.
However, both BUILD_BUG_ON_NOT_POWER_OF_2 and BUILD_BUG_ON was
evaluating to nothing in this case, and we want (0) since this is a
function-like macro that will be followed by a semicolon.
Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/bug.h | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 298a916..1b2465d 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -12,10 +12,10 @@ enum bug_trap_type {
struct pt_regs;
#ifdef __CHECKER__
-#define BUILD_BUG_ON_NOT_POWER_OF_2(n)
+#define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
#define BUILD_BUG_ON_ZERO(e) (0)
#define BUILD_BUG_ON_NULL(e) ((void*)0)
-#define BUILD_BUG_ON(condition)
+#define BUILD_BUG_ON(condition) (0)
#define BUILD_BUG() (0)
#else /* __CHECKER__ */
--
1.7.3.4
__linktime_error() does the same thing as __compiletime_error() and is
only used in bug.h. Since the macro defines a function attribute that
will cause a failure at compile-time (not link-time), it makes more
sense to keep __compiletime_error(), which is also neatly mated with
__compiletime_warning().
Signed-off-by: Daniel Santos <[email protected]>
Acked-by: David Rientjes <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
---
include/linux/bug.h | 2 +-
include/linux/compiler-gcc4.h | 2 --
include/linux/compiler.h | 3 ---
3 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index aaac4bb..298a916 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -73,7 +73,7 @@ extern int __build_bug_on_failed;
#define BUILD_BUG() \
do { \
extern void __build_bug_failed(void) \
- __linktime_error("BUILD_BUG failed"); \
+ __compiletime_error("BUILD_BUG failed");\
__build_bug_failed(); \
} while (0)
diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 9755029..7f143ac 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -33,8 +33,6 @@
the kernel context */
#define __cold __attribute__((__cold__))
-#define __linktime_error(message) __attribute__((__error__(message)))
-
#ifndef __CHECKER__
# define __compiletime_warning(message) __attribute__((warning(message)))
# define __compiletime_error(message) __attribute__((error(message)))
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index b121554..cbf6d9d 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -299,9 +299,6 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
#ifndef __compiletime_error
# define __compiletime_error(message)
#endif
-#ifndef __linktime_error
-# define __linktime_error(message)
-#endif
/*
* Prevent the compiler from merging or refetching accesses. The compiler
* is also forbidden from reordering successive instances of ACCESS_ONCE(),
--
1.7.3.4
Throughout compiler*.h, many version checks are made. These can be
simplified by using the macro that gcc's documentation recommends.
However, my primary reason for adding this is that I need bug-check
macros that are enabled at certain gcc versions and it's cleaner to use
this macro than the tradition method:
if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ => 2)
If you add patch level, it gets this ugly:
if __GNUC__ > 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ > 2 || \
__GNUC_MINOR__ == 2 __GNUC_PATCHLEVEL__ >= 1))
As opposed to:
if GCC_VERSION >= 40201
While having separate headers for gcc 3 & 4 eliminates some of this
verbosity, they can still be cleaned up by this.
See also:
http://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
Signed-off-by: Daniel Santos <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
Acked-by: David Rientjes <[email protected]>
---
include/linux/compiler-gcc.h | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index 6a6d7ae..24545cd 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -5,6 +5,9 @@
/*
* Common definitions for all gcc versions go here.
*/
+#define GCC_VERSION (__GNUC__ * 10000 \
+ + __GNUC_MINOR__ * 100 \
+ + __GNUC_PATCHLEVEL__)
/* Optimization barrier */
--
1.7.3.4
Borislav,
Please note that this patch has changed slightly since you Acked it. I
have moved the location of the negative-size array code to the end of
the macro. I don't think this really matters honestly, but I figured
this was the best place to put that change.
Daniel
On 11/13/2012 04:13 PM, [email protected] wrote:
> Negative sized arrays wont create a compile-time error in some cases
> starting with gcc 4.4 (e.g., inlined functions), but gcc 4.3 introduced
> the error function attribute that will. This patch modifies
> BUILD_BUG_ON to behave like BUILD_BUG already does, using the error
> function attribute so that you don't have to build the entire kernel to
> discover that you have a problem, and then enjoy trying to track it down
> from a link-time error.
>
> Also, we are only including asm/bug.h and then expecting that
> linux/compiler.h will eventually be included to define __linktime_error
> (used in BUILD_BUG_ON). This patch includes it directly for clarity and
> to avoid the possibility of changes in <arch>/*/include/asm/bug.h being
> changed or not including linux/compiler.h for some reason.
>
> Signed-off-by: Daniel Santos <[email protected]>
> Acked-by: Borislav Petkov <[email protected]>
> ---
> include/linux/bug.h | 32 +++++++++++++++++++-------------
> 1 files changed, 19 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index ccd44ce..dd4f506 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -2,6 +2,7 @@
> #define _LINUX_BUG_H
>
> #include <asm/bug.h>
> +#include <linux/compiler.h>
>
> enum bug_trap_type {
> BUG_TRAP_TYPE_NONE = 0,
> @@ -42,25 +43,30 @@ struct pt_regs;
> * @condition: the condition which the compiler should know is false.
> *
> * If you have some code which relies on certain constants being equal, or
> - * other compile-time-evaluated condition, you should use BUILD_BUG_ON to
> + * some other compile-time-evaluated condition, you should use BUILD_BUG_ON to
> * detect if someone changes it.
> *
> - * The implementation uses gcc's reluctance to create a negative array, but
> - * gcc (as of 4.4) only emits that error for obvious cases (eg. not arguments
> - * to inline functions). So as a fallback we use the optimizer; if it can't
> - * prove the condition is false, it will cause a link error on the undefined
> - * "__build_bug_on_failed". This error message can be harder to track down
> - * though, hence the two different methods.
> + * The implementation uses gcc's reluctance to create a negative array, but gcc
> + * (as of 4.4) only emits that error for obvious cases (e.g. not arguments to
> + * inline functions). Luckily, in 4.3 they added the "error" function
> + * attribute just for this type of case. Thus, we use a negative sized array
> + * (should always create an error on gcc versions older than 4.4) and then call
> + * an undefined function with the error attribute (should always create an
> + * error on gcc 4.3 and later). If for some reason, neither creates a
> + * compile-time error, we'll still have a link-time error, which is harder to
> + * track down.
> */
> #ifndef __OPTIMIZE__
> #define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> #else
> -extern int __build_bug_on_failed;
> -#define BUILD_BUG_ON(condition) \
> - do { \
> - bool __cond = !!(condition); \
> - ((void)sizeof(char[1 - 2*!!(__cond)])); \
> - if (__cond) __build_bug_on_failed = 1; \
> +#define BUILD_BUG_ON(condition) \
> + do { \
> + bool __cond = !!(condition); \
> + extern void __build_bug_on_failed(void) \
> + __compiletime_error("BUILD_BUG_ON failed"); \
> + if (__cond) \
> + __build_bug_on_failed(); \
> + ((void)sizeof(char[1 - 2*!!(__cond)])); \
> } while(0)
> #endif
>
On Tue, Nov 13, 2012 at 04:24:15PM -0600, Daniel Santos wrote:
> Borislav,
>
> Please note that this patch has changed slightly since you Acked it. I
> have moved the location of the negative-size array code to the end of
> the macro. I don't think this really matters honestly, but I figured
> this was the best place to put that change.
Yes, that's fine.
Thanks.
--
Regards/Gruss,
Boris.
On Tue, Nov 13, 2012 at 04:13:38PM -0600, [email protected] wrote:
> When calling BUILD_BUG_ON in an optimized build using gcc 4.3 and later,
> the condition will be evaulated twice, possibily with side-effects.
> This patch eliminates that error.
>
> Signed-off-by: Daniel Santos <[email protected]>
> ---
> include/linux/bug.h | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index 1b2465d..ccd44ce 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -58,8 +58,9 @@ struct pt_regs;
> extern int __build_bug_on_failed;
> #define BUILD_BUG_ON(condition) \
> do { \
> - ((void)sizeof(char[1 - 2*!!(condition)])); \
> - if (condition) __build_bug_on_failed = 1; \
> + bool __cond = !!(condition); \
> + ((void)sizeof(char[1 - 2*!!(__cond)])); \
No need for the !!(__cond) here because you've done it in the line
above. IOW,
bool __cond = !!(condition);
((void)sizeof(char[1 - 2 * __cond]));
which is even a bit more readable, as a matter of fact.
> + if (__cond) __build_bug_on_failed = 1; \
> } while(0)
> #endif
Thanks.
--
Regards/Gruss,
Boris.
On Tue, Nov 13, 2012 at 04:13:37PM -0600, [email protected] wrote:
> When __CHECKER__ is defined, we disable all of the BUILD_BUG.* macros.
> However, both BUILD_BUG_ON_NOT_POWER_OF_2 and BUILD_BUG_ON was
> evaluating to nothing in this case, and we want (0) since this is a
> function-like macro that will be followed by a semicolon.
>
> Signed-off-by: Daniel Santos <[email protected]>
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
On Tue, Nov 13, 2012 at 04:13:40PM -0600, [email protected] wrote:
> Prior to the introduction of __attribute__((error("msg"))) in gcc 4.3,
> creating compile-time errors required a little trickery.
> BUILD_BUG{,_ON} uses this attribute when available to generate
> compile-time errors, but also uses the negative-sized array trick for
> older compilers, resulting in two error messages in some cases. The
> reason it's "some" cases is that as of gcc 4.4, the negative-sized array
> will not create an error in some situations, like inline functions.
>
> This patch replaces the negative-sized array code with the new
> __compiletime_error_fallback() macro which expands to the same thing
> unless the the error attribute is available, in which case it expands to
> do{}while(0), resulting in exactly one compile-time error on all
> versions of gcc.
>
> Note that we are not changing the negative-sized array code for the
> unoptimized version of BUILD_BUG_ON, since it has the potential to catch
> problems that would be disabled in later versions of gcc were
> __compiletime_error_fallback used. The reason is that that an
> unoptimized build can't always remove calls to an error-attributed
> function call (like we are using) that should effectively become dead
> code if it were optimized. However, using a negative-sized array with a
> similar value will not result in an false-positive (error). The only
> caveat being that it will also fail to catch valid conditions, which we
> should be expecting in an unoptimized build anyway.
>
> Signed-off-by: Daniel Santos <[email protected]>
> ---
> include/linux/bug.h | 2 +-
> include/linux/compiler.h | 5 +++++
> 2 files changed, 6 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index dd4f506..125e744 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -66,7 +66,7 @@ struct pt_regs;
> __compiletime_error("BUILD_BUG_ON failed"); \
> if (__cond) \
> __build_bug_on_failed(); \
> - ((void)sizeof(char[1 - 2*!!(__cond)])); \
> + __compiletime_error_fallback(__cond); \
We're passing an already evaluated __cond here which gets doubly-negated
again in __compiletime_error_fallback. If __compiletime_error_fallback
is going to be called only from BUILD_BUG_ON, then its definition should
be:
do { ((void)sizeof(char[1 - 2 * (condition)])); } while (0)
i.e., without the !!.
> } while(0)
> #endif
>
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index cbf6d9d..8e5b9d5 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -298,7 +298,12 @@ void ftrace_likely_update(struct ftrace_branch_data *f, int val, int expect);
> #endif
> #ifndef __compiletime_error
> # define __compiletime_error(message)
> +# define __compiletime_error_fallback(condition) \
> + do { ((void)sizeof(char[1 - 2*!!(condition)])); } while (0)
> +#else
> +# define __compiletime_error_fallback(condition) do { } while (0)
> #endif
> +
> /*
> * Prevent the compiler from merging or refetching accesses. The compiler
> * is also forbidden from reordering successive instances of ACCESS_ONCE(),
> --
> 1.7.3.4
Thanks.
--
Regards/Gruss,
Boris.
On Tue, Nov 13, 2012 at 04:13:41PM -0600, [email protected] wrote:
> Introduce compiletime_assert to compiler.h, which moves the details of
> how to break a build and emit an error message for a specific compiler
> to the headers where these details should be. Following the tradition of
> the POSIX assert macro, compiletime_assert creates a build-time error
> when the supplied condition is *false*.
>
> Next, we add BUILD_BUG_ON_MSG to bug.h which simply wraps
> compiletime_assert, inverting the logic, so that it fails when the
> condition is *true*, consistient with the language "build bug on." This
> macro allows you to specify the error message you want emitted when the
> supplied condition is true.
>
> Finally, we remove all other code from bug.h that mucks with these
> details (BUILD_BUG & BUILD_BUG_ON), and have them all call
> BUILD_BUG_ON_MSG. This not only reduces source code bloat, but also
> prevents the possibility of code being changed for one macro and not for
> the other (which was previously the case for BUILD_BUG and
> BUILD_BUG_ON).
>
> Signed-off-by: Daniel Santos <[email protected]>
Looks nice and clean.
Acked-by: Borislav Petkov <[email protected]>
--
Regards/Gruss,
Boris.
On 11/15/2012 09:07 AM, Borislav Petkov wrote:
> On Tue, Nov 13, 2012 at 04:13:38PM -0600, [email protected] wrote:
>> When calling BUILD_BUG_ON in an optimized build using gcc 4.3 and later,
>> the condition will be evaulated twice, possibily with side-effects.
>> This patch eliminates that error.
>>
>> Signed-off-by: Daniel Santos <[email protected]>
>> ---
>> include/linux/bug.h | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/bug.h b/include/linux/bug.h
>> index 1b2465d..ccd44ce 100644
>> --- a/include/linux/bug.h
>> +++ b/include/linux/bug.h
>> @@ -58,8 +58,9 @@ struct pt_regs;
>> extern int __build_bug_on_failed;
>> #define BUILD_BUG_ON(condition) \
>> do { \
>> - ((void)sizeof(char[1 - 2*!!(condition)])); \
>> - if (condition) __build_bug_on_failed = 1; \
>> + bool __cond = !!(condition); \
>> + ((void)sizeof(char[1 - 2*!!(__cond)])); \
> No need for the !!(__cond) here because you've done it in the line
> above. IOW,
>
> bool __cond = !!(condition);
> ((void)sizeof(char[1 - 2 * __cond]));
>
> which is even a bit more readable, as a matter of fact.
Ah yes. I did notice that at one point, but I think it slipped my mind.
Also, the kernel has introduced me to the usage of the !! construct, of
which I'm well versed in its affects in various situations and how gcc's
optimizer ends up treating its usage, so probably another reason I
didn't change it immediately. But it's basically shorthand for the
expression (condition ? 1 : 0), correct? Or are there other details in
the C standard that makes it different in some cases?
Thanks,
Daniel
On 11/15/2012 09:08 AM, Borislav Petkov wrote:
> On Tue, Nov 13, 2012 at 04:13:40PM -0600, [email protected] wrote:
>> Prior to the introduction of __attribute__((error("msg"))) in gcc 4.3,
>> creating compile-time errors required a little trickery.
>> BUILD_BUG{,_ON} uses this attribute when available to generate
>> compile-time errors, but also uses the negative-sized array trick for
>> older compilers, resulting in two error messages in some cases. The
>> reason it's "some" cases is that as of gcc 4.4, the negative-sized array
>> will not create an error in some situations, like inline functions.
>>
>> This patch replaces the negative-sized array code with the new
>> __compiletime_error_fallback() macro which expands to the same thing
>> unless the the error attribute is available, in which case it expands to
>> do{}while(0), resulting in exactly one compile-time error on all
>> versions of gcc.
>>
>> Note that we are not changing the negative-sized array code for the
>> unoptimized version of BUILD_BUG_ON, since it has the potential to catch
>> problems that would be disabled in later versions of gcc were
>> __compiletime_error_fallback used. The reason is that that an
>> unoptimized build can't always remove calls to an error-attributed
>> function call (like we are using) that should effectively become dead
>> code if it were optimized. However, using a negative-sized array with a
>> similar value will not result in an false-positive (error). The only
>> caveat being that it will also fail to catch valid conditions, which we
>> should be expecting in an unoptimized build anyway.
>>
>> Signed-off-by: Daniel Santos <[email protected]>
>> ---
>> include/linux/bug.h | 2 +-
>> include/linux/compiler.h | 5 +++++
>> 2 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/include/linux/bug.h b/include/linux/bug.h
>> index dd4f506..125e744 100644
>> --- a/include/linux/bug.h
>> +++ b/include/linux/bug.h
>> @@ -66,7 +66,7 @@ struct pt_regs;
>> __compiletime_error("BUILD_BUG_ON failed"); \
>> if (__cond) \
>> __build_bug_on_failed(); \
>> - ((void)sizeof(char[1 - 2*!!(__cond)])); \
>> + __compiletime_error_fallback(__cond); \
> We're passing an already evaluated __cond here which gets doubly-negated
> again in __compiletime_error_fallback. If __compiletime_error_fallback
> is going to be called only from BUILD_BUG_ON, then its definition should
> be:
>
> do { ((void)sizeof(char[1 - 2 * (condition)])); } while (0)
>
> i.e., without the !!.
Yeah, I agree. Also, with the complexity, I think a few more comments
can be helpful in compiler.h to clarify what these macros are for more
specifically.
On another note, I have a "part two" set of patches for bug.h &
compiler*.h that does some other stuff (more cleanup & restructuring)
and this is making me think about that more. My thought about
__compiletime_error_fallback is that it should be called only from
within compiler.h (as in the following patch) and basically just be a
private macro. However, we still use the use the negative sized array
trick for the unoptimized version of BUILD_BUG_ON (which may have
limited meaning), and we also use a negative bit specifier on a bitfield
in BUILD_BUG_ON_ZERO and BUILD_BUG_ON_NULL (which I treat some in my
other patches as well). But my thought is that it may be helpful to
encapsulate these tricks into (public) macros in compiler*.h, such as
"compiletime_assert_negarray" and "compiletime_assert_negbitfiled" and
then have __compiletime_error_fallback expand to
compiletime_assert_negarray when it's needed and no-op when it's not.
This doesn't have to be decided now, but it's just a thought you gave me.
And in case you're wondering about the negative bit field,
BUILD_BUG_ON_{ZERO,NULL} can't call BUILD_BUG_ON in a complex expression
({ exp; exp; }) because it is evaluated outside of a function body and
gcc doesn't like that. Thus, the following macro would, sadly, result
in errors:
#define BUILD_BUG_ON_ZERO(exp) ({BUILD_BUG_ON(exp); 0;})
However, it would not be an error to call an __alwaysinline function
that uses BUILD_BUG_ON, so I still have to explore that and make sure it
works in all scenarios (and compilers).
But back to *this* patch, I'll make that change now.
Thanks!
Daniel
On 11/13/2012 04:13 PM, [email protected] wrote:
> +#define __compiletime_assert(condition, msg, __func) \
> + do { \
> + bool __cond = !(condition); \
> + extern void __func(void) __compiletime_error(msg); \
> + if (__cond) \
> + __func(); \
> + __compiletime_error_fallback(__cond); \
> + } while (0)
> +
> +#define _compiletime_assert(condition, msg, __func) \
> + __compiletime_assert(condition, msg, __func)
> +
> +/**
> + * compiletime_assert - break build and emit msg if condition is false
> + * @condition: a compile-time constant condition to check
> + * @msg: a message to emit if condition is false
> + *
> + * In tradition of POSIX assert, this macro will break the build if the
> + * supplied condition is *false*, emitting the supplied error message if the
> + * compiler has support to do so.
> + */
> +#define compiletime_assert(condition, msg) \
> + _compiletime_assert(condition, msg, __compiletime_assert_ ## __LINE__)
> +
Whoops, this implementation pastes the tokens before allowing them to
expand, it should be something like this:
#define __compiletime_assert(condition, msg, __func) .... stuff
#define _compiletime_assert(condition, msg, prefix, suffix) \
__compiletime_assert(condition, msg, prefix ## suffix)
#define compiletime_assert(condition, msg) \
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
I'll fix that as well.
Daniel
On Thu, Nov 15, 2012 at 01:11:15PM -0600, Daniel Santos wrote:
> Ah yes. I did notice that at one point, but I think it slipped
> my mind. Also, the kernel has introduced me to the usage of the
> !! construct, of which I'm well versed in its affects in various
> situations and how gcc's optimizer ends up treating its usage, so
> probably another reason I didn't change it immediately. But it's
> basically shorthand for the expression (condition ? 1 : 0), correct?
I don't think so: "!!" is simply a double negation which turns the
whatever wild construct you have into either 0 or 1, depending on what
it evaluates to.
But I don't know what the standard says so you'll have to check :-)
Thanks.
--
Regards/Gruss,
Boris.
On Thu, Nov 15, 2012 at 01:44:45PM -0600, Daniel Santos wrote:
> Yeah, I agree. Also, with the complexity, I think a few more comments
> can be helpful in compiler.h to clarify what these macros are for more
> specifically.
>From what I've read so far the comments are fine but if you want to be
more descriptive then sure, by all means. Just don't let them grow out
of proportion - keep them simple and succinct.
> On another note, I have a "part two" set of patches for bug.h &
> compiler*.h that does some other stuff (more cleanup & restructuring)
> and this is making me think about that more. My thought about
> __compiletime_error_fallback is that it should be called only from
> within compiler.h (as in the following patch) and basically just be a
> private macro. However, we still use the use the negative sized array
> trick for the unoptimized version of BUILD_BUG_ON (which may have
> limited meaning), and we also use a negative bit specifier on a bitfield
> in BUILD_BUG_ON_ZERO and BUILD_BUG_ON_NULL (which I treat some in my
> other patches as well). But my thought is that it may be helpful to
> encapsulate these tricks into (public) macros in compiler*.h, such as
> "compiletime_assert_negarray" and "compiletime_assert_negbitfiled" and
> then have __compiletime_error_fallback expand to
> compiletime_assert_negarray when it's needed and no-op when it's not.
>
> This doesn't have to be decided now, but it's just a thought you gave me.
I dunno. I'm thinking that maybe making them more unreadable for no
apparent reason might be simply too much. They're fine the way they are
now, if you ask me.
> And in case you're wondering about the negative bit field,
> BUILD_BUG_ON_{ZERO,NULL} can't call BUILD_BUG_ON in a complex expression
> ({ exp; exp; }) because it is evaluated outside of a function body and
> gcc doesn't like that. Thus, the following macro would, sadly, result
> in errors:
>
> #define BUILD_BUG_ON_ZERO(exp) ({BUILD_BUG_ON(exp); 0;})
>
> However, it would not be an error to call an __alwaysinline function
> that uses BUILD_BUG_ON, so I still have to explore that and make sure it
> works in all scenarios (and compilers).
Ok, I see your point. Think of it this way: if unifying those macros can
only happen at the expense of readability or performance then maybe it
is not worth the trouble. If, OTOH, you can think of something slick and
pretty, then go ahead :).
HTH.
--
Regards/Gruss,
Boris.
On 11/17/2012 08:38 AM, Borislav Petkov wrote:
> On Thu, Nov 15, 2012 at 01:11:15PM -0600, Daniel Santos wrote:
>> Ah yes. I did notice that at one point, but I think it slipped
>> my mind. Also, the kernel has introduced me to the usage of the
>> !! construct, of which I'm well versed in its affects in various
>> situations and how gcc's optimizer ends up treating its usage, so
>> probably another reason I didn't change it immediately. But it's
>> basically shorthand for the expression (condition ? 1 : 0), correct?
Oh, this is embarrassing, The sentence above should have read:
... the kernel has introduced me to the usage of the !! construct, of which I'm *not* well versed in ...
hah! oh well
> I don't think so: "!!" is simply a double negation which turns the
> whatever wild construct you have into either 0 or 1, depending on what
> it evaluates to.
>
> But I don't know what the standard says so you'll have to check :-)
Here we go, from section ยง6.5.3.3:
The result of the logical negation operator ! is 0 if the value of its
operand compares unequal to 0, 1 if the value of its operand compares
equal to 0. The result has type int. The expression !E is equivalent to
(0==E).
I simply wasn't aware that it promised a "true" value to be one
exactly. Hurray for expanding horizons! :)
Daniel