2012-10-28 20:54:03

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 0/10] Cleanup & new features for compiler*.h and bug.h


include/linux/bug.h | 59 ++++++++++++++++++++++++++--------------
include/linux/compiler-gcc.h | 3 ++
include/linux/compiler-gcc3.h | 8 +++---
include/linux/compiler-gcc4.h | 28 +++++++++---------
include/linux/compiler.h | 8 ++++-
5 files changed, 65 insertions(+), 41 deletions(-)

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

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 Introduce BUILD_BUG_ON_MSG and clean up the implementations of the
BUILD_BUG* macros.


2012-10-28 20:57:09

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 1/9] compiler-gcc4.h: Reorder macros based upon gcc ver

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]>
---
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

2012-10-28 20:57:14

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 3/9] compiler-gcc{3,4}.h: Use GCC_VERSION macro

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

2012-10-28 20:57:18

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

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.

Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/bug.h | 4 ++--
include/linux/compiler.h | 7 +++++++
2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 03259d7..da03dc1 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -57,13 +57,13 @@ struct pt_regs;
* track down.
*/
#ifndef __OPTIMIZE__
-#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+#define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
#else
#define BUILD_BUG_ON(condition) \
do { \
extern void __build_bug_on_failed(void) \
__compiletime_error("BUILD_BUG_ON failed"); \
- ((void)sizeof(char[1 - 2*!!(condition)])); \
+ __compiletime_error_fallback(condition); \
if (condition) \
__build_bug_on_failed(); \
} while(0)
diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index cbf6d9d..84926f2 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -298,6 +298,13 @@ 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)
+#endif
+#ifndef __compiletime_error_fallback
+# define __compiletime_error_fallback(condition) do { } while (0)
#endif
/*
* Prevent the compiler from merging or refetching accesses. The compiler
--
1.7.3.4

2012-10-28 20:57:26

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 8/9] bug.h: Add BUILD_BUG_ON_MSG & _BUILD_BUG_INTERNAL

Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
enabled), except that it allows you to specify the error message you
want emitted as the third parameter. Under the hood, this relies on
_BUILD_BUG_INTERNAL, which does the actual work and is pretty-much
identical to BUILD_BUG_ON.

Signed-off-by: Daniel Santos <[email protected]>
---
include/linux/bug.h | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 6c38988..3bc1ddf 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)
#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__ */
@@ -38,6 +39,27 @@ struct pt_regs;
*/
#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))

+#define __BUILD_BUG_INTERNAL(condition, msg, line) \
+ do { \
+ extern void __build_bug_on_failed_ ## line \
+ (void) __compiletime_error(msg); \
+ __compiletime_error_fallback(condition); \
+ if (condition) \
+ __build_bug_on_failed_ ## line(); \
+ } while (0)
+
+#define _BUILD_BUG_INTERNAL(condition, msg, line) \
+ __BUILD_BUG_INTERNAL(condition, msg, line)
+
+/**
+ * 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) _BUILD_BUG_INTERNAL(cond, msg, __LINE__)
+
/**
* BUILD_BUG_ON - break compile if a condition is true.
* @condition: the condition which the compiler should know is false.
--
1.7.3.4

2012-10-28 20:57:24

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 9/9] bug.h: Convert BUILD_BUG{,_ON} to use BUILD_BUG_ON_MSG

Remove duplicate code by converting BUILD_BUG and BUILD_BUG_ON to just
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 | 17 +++--------------
1 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 3bc1ddf..b58ba51 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -81,14 +81,8 @@ struct pt_regs;
#ifndef __OPTIMIZE__
#define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
#else
-#define BUILD_BUG_ON(condition) \
- do { \
- extern void __build_bug_on_failed(void) \
- __compiletime_error("BUILD_BUG_ON failed"); \
- __compiletime_error_fallback(condition); \
- if (condition) \
- __build_bug_on_failed(); \
- } while(0)
+#define BUILD_BUG_ON(condition) \
+ BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
#endif

/**
@@ -98,12 +92,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__ */

--
1.7.3.4

2012-10-28 20:57:59

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 7/9] bug.h: Fix BUILD_BUG_ON macro in __CHECKER__

When __CHECKER__ is defined, we disable all of the BUILD_BUG.* macros.
However, 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 | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index da03dc1..6c38988 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -16,7 +16,7 @@ struct pt_regs;
#define BUILD_BUG_ON_NOT_POWER_OF_2(n)
#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

2012-10-28 20:58:17

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 5/9] bug.h: Make BUILD_BUG_ON generate compile-time error

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 | 30 ++++++++++++++++++------------
1 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 298a916..03259d7 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,24 +43,29 @@ 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 { \
- ((void)sizeof(char[1 - 2*!!(condition)])); \
- if (condition) __build_bug_on_failed = 1; \
+#define BUILD_BUG_ON(condition) \
+ do { \
+ extern void __build_bug_on_failed(void) \
+ __compiletime_error("BUILD_BUG_ON failed"); \
+ ((void)sizeof(char[1 - 2*!!(condition)])); \
+ if (condition) \
+ __build_bug_on_failed(); \
} while(0)
#endif

--
1.7.3.4

2012-10-28 20:58:41

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 4/9] compiler{,-gcc4}.h, bug.h: Remove duplicate macros

__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

2012-10-28 20:59:00

by Daniel Santos

[permalink] [raw]
Subject: [PATCH v4 2/9] compiler-gcc.h: Add gcc-recommended GCC_VERSION macro

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

2012-10-30 12:02:15

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 1/9] compiler-gcc4.h: Reorder macros based upon gcc ver

On Sun, Oct 28, 2012 at 03:57:07PM -0500, [email protected] wrote:
> 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]>

--
Regards/Gruss,
Boris.

2012-10-30 16:19:42

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On Sun, Oct 28, 2012 at 03:57:12PM -0500, [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.
>
> Signed-off-by: Daniel Santos <[email protected]>
> ---
> include/linux/bug.h | 4 ++--
> include/linux/compiler.h | 7 +++++++
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index 03259d7..da03dc1 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -57,13 +57,13 @@ struct pt_regs;
> * track down.
> */
> #ifndef __OPTIMIZE__
> -#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +#define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
> #else
> #define BUILD_BUG_ON(condition) \
> do { \
> extern void __build_bug_on_failed(void) \
> __compiletime_error("BUILD_BUG_ON failed"); \
> - ((void)sizeof(char[1 - 2*!!(condition)])); \
> + __compiletime_error_fallback(condition); \
> if (condition) \
> __build_bug_on_failed(); \

If we're defining a fallback, shouldn't it come second? I.e.:

if (condition)
__build_bug_on_failed();
__compiletime_error_fallback(condition);

Also, the error message from __build_bug_on_failed is much more
informative:

arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
arch/x86/kernel/cpu/amd.c:486:2: error: call to ‘__build_bug_on_failed’ declared with attribute error: BUILD_BUG_ON failed
make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [arch/x86/kernel/cpu/] Error 2

than

arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
arch/x86/kernel/cpu/amd.c:486:2: error: size of unnamed array is negative
make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [arch/x86/kernel/cpu/] Error 2

Finally, you need to do:

bool __cond = !!(condition);

and use __cond so that condition doesn't get evaluated multiple times
(possibly with side effects).

Thanks.

--
Regards/Gruss,
Boris.

2012-10-30 16:44:25

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 7/9] bug.h: Fix BUILD_BUG_ON macro in __CHECKER__

On Sun, Oct 28, 2012 at 03:57:13PM -0500, [email protected] wrote:
> When __CHECKER__ is defined, we disable all of the BUILD_BUG.* macros.
> However, 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.

2012-10-30 17:17:55

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 8/9] bug.h: Add BUILD_BUG_ON_MSG & _BUILD_BUG_INTERNAL

On Sun, Oct 28, 2012 at 03:57:14PM -0500, [email protected] wrote:
> Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
> enabled), except that it allows you to specify the error message you
> want emitted as the third parameter. Under the hood, this relies on
> _BUILD_BUG_INTERNAL, which does the actual work and is pretty-much
> identical to BUILD_BUG_ON.
>
> Signed-off-by: Daniel Santos <[email protected]>
> ---
> include/linux/bug.h | 22 ++++++++++++++++++++++
> 1 files changed, 22 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index 6c38988..3bc1ddf 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)
> #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__ */
> @@ -38,6 +39,27 @@ struct pt_regs;
> */
> #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
>
> +#define __BUILD_BUG_INTERNAL(condition, msg, line) \
> + do { \
> + extern void __build_bug_on_failed_ ## line \
> + (void) __compiletime_error(msg); \
> + __compiletime_error_fallback(condition); \
> + if (condition) \
> + __build_bug_on_failed_ ## line(); \
> + } while (0)
> +
> +#define _BUILD_BUG_INTERNAL(condition, msg, line) \
> + __BUILD_BUG_INTERNAL(condition, msg, line)

Stupid question:

can BUILD_BUG_ON and BUILD_BUG_ON_MSG both use __BUILD_BUG_INTERNAL?

In the BUILD_BUG_ON msg will be "BUILD_BUG_ON failed" and line empty.
Can that even work?

Thanks.

--
Regards/Gruss,
Boris.

2012-10-30 19:19:14

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 9/9] bug.h: Convert BUILD_BUG{,_ON} to use BUILD_BUG_ON_MSG

On Sun, Oct 28, 2012 at 03:57:15PM -0500, [email protected] wrote:
> Remove duplicate code by converting BUILD_BUG and BUILD_BUG_ON to just
> 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 | 17 +++--------------
> 1 files changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index 3bc1ddf..b58ba51 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -81,14 +81,8 @@ struct pt_regs;
> #ifndef __OPTIMIZE__
> #define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
> #else
> -#define BUILD_BUG_ON(condition) \
> - do { \
> - extern void __build_bug_on_failed(void) \
> - __compiletime_error("BUILD_BUG_ON failed"); \
> - __compiletime_error_fallback(condition); \
> - if (condition) \
> - __build_bug_on_failed(); \
> - } while(0)
> +#define BUILD_BUG_ON(condition) \
> + BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)

Concatenating "condition" might not be very informative in all cases.
For example:

BUILD_BUG_ON(1);

Having __LINE__ is good enough IMHO.

Thanks.

--
Regards/Gruss,
Boris.

2012-10-30 21:57:37

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 8/9] bug.h: Add BUILD_BUG_ON_MSG & _BUILD_BUG_INTERNAL

On Tue, Oct 30, 2012 at 06:17:47PM +0100, Borislav Petkov wrote:
> can BUILD_BUG_ON and BUILD_BUG_ON_MSG both use __BUILD_BUG_INTERNAL?
>
> In the BUILD_BUG_ON msg will be "BUILD_BUG_ON failed" and line empty.
> Can that even work?

Yes it can, I should simply look at patch 9/9 first :-).

--
Regards/Gruss,
Boris.

2012-10-31 01:02:45

by Josh Triplett

[permalink] [raw]
Subject: Re: [PATCH v4 9/9] bug.h: Convert BUILD_BUG{,_ON} to use BUILD_BUG_ON_MSG

On Tue, Oct 30, 2012 at 08:19:05PM +0100, Borislav Petkov wrote:
> On Sun, Oct 28, 2012 at 03:57:15PM -0500, [email protected] wrote:
> > Remove duplicate code by converting BUILD_BUG and BUILD_BUG_ON to just
> > 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 | 17 +++--------------
> > 1 files changed, 3 insertions(+), 14 deletions(-)
> >
> > diff --git a/include/linux/bug.h b/include/linux/bug.h
> > index 3bc1ddf..b58ba51 100644
> > --- a/include/linux/bug.h
> > +++ b/include/linux/bug.h
> > @@ -81,14 +81,8 @@ struct pt_regs;
> > #ifndef __OPTIMIZE__
> > #define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
> > #else
> > -#define BUILD_BUG_ON(condition) \
> > - do { \
> > - extern void __build_bug_on_failed(void) \
> > - __compiletime_error("BUILD_BUG_ON failed"); \
> > - __compiletime_error_fallback(condition); \
> > - if (condition) \
> > - __build_bug_on_failed(); \
> > - } while(0)
> > +#define BUILD_BUG_ON(condition) \
> > + BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>
> Concatenating "condition" might not be very informative in all cases.
> For example:
>
> BUILD_BUG_ON(1);
>
> Having __LINE__ is good enough IMHO.

While it doesn't always help, it may help sometimes. Worst case,
BUILD_BUG_ON(1) gives you no less information than it did before; best
case, it gives you useful data.

- Josh Triplett

2012-10-31 05:34:32

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On 10/30/2012 11:19 AM, Borislav Petkov wrote:
> On Sun, Oct 28, 2012 at 03:57:12PM -0500, [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.
>>
>> Signed-off-by: Daniel Santos <[email protected]>
>> ---
>> include/linux/bug.h | 4 ++--
>> include/linux/compiler.h | 7 +++++++
>> 2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/bug.h b/include/linux/bug.h
>> index 03259d7..da03dc1 100644
>> --- a/include/linux/bug.h
>> +++ b/include/linux/bug.h
>> @@ -57,13 +57,13 @@ struct pt_regs;
>> * track down.
>> */
>> #ifndef __OPTIMIZE__
>> -#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
>> +#define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
>> #else
>> #define BUILD_BUG_ON(condition) \
>> do { \
>> extern void __build_bug_on_failed(void) \
>> __compiletime_error("BUILD_BUG_ON failed"); \
>> - ((void)sizeof(char[1 - 2*!!(condition)])); \
>> + __compiletime_error_fallback(condition); \
>> if (condition) \
>> __build_bug_on_failed(); \
> If we're defining a fallback, shouldn't it come second? I.e.:
>
> if (condition)
> __build_bug_on_failed();
> __compiletime_error_fallback(condition);
>
> Also, the error message from __build_bug_on_failed is much more
> informative:
>
> arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
> arch/x86/kernel/cpu/amd.c:486:2: error: call to ‘__build_bug_on_failed’ declared with attribute error: BUILD_BUG_ON failed
> make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [arch/x86/kernel/cpu/] Error 2
>
> than
>
> arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
> arch/x86/kernel/cpu/amd.c:486:2: error: size of unnamed array is negative
> make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> make: *** [arch/x86/kernel/cpu/] Error 2
Yes, the __build_bug_on_failed message is much more informative. This
will only increase with these patches. For example, the line

BUILD_BUG_ON(sizeof(*c) != 4);

emits this error:

arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
arch/x86/kernel/cpu/amd.c:486:2: error: call to
‘__build_bug_on_failed_486’ declared with attribute error: BUILD_BUG_ON
failed: sizeof(*c) != 4
make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
make: *** [arch/x86/kernel/cpu/amd.o] Error 2

It's true that there is some redundancy in there as well as the
gibberish line number embedded in the function name, but the end of the
line spits out the exact statement that failed.

But as far as rather the fallback is first or the __compiletime_error
function is a matter of asthetics, since it's really an either/or
situation. Either the __build_bug_on_failedxxx function will be
declared with __attribute__((error(message))) and the fallback will
expand to a no-op, or the fallback will produce code that (presumably
always?) breaks the build. For insurance, a link-time error will occur
if the fallback code fails to break the build.

Realistically, a single macro could be defined in compiler*.h that
encapsulates the entirety of this mechanism and only exposes a "black
box" macro, that will simply expand to something that breaks the build
in the most appropriate fashion based upon the version of gcc. In
essence, the new BUILD_BUG_ON_MSG macro attempts to fill that roll.
>
> Finally, you need to do:
>
> bool __cond = !!(condition);
>
> and use __cond so that condition doesn't get evaluated multiple times
> (possibly with side effects).
>
> Thanks.
Big problem! Very good catch, thank you! All good programmers know not
use expressions that can have side effects in an assert-type macro, but
this it should certainly be as dummy proof as possible. That will force
others to get a really *really* good dummy if they want to break it!

Thank you for this! I suppose another justification for having a single
"black box" macro that does this in one place and addresses all of these
tricky issues.

I guess I'll fix it up (and address the emails on the other patches) and
do a v5 then for the whole set? (is that the right way to resubmit with
these corrections?)

Thanks
Daniel

2012-10-31 05:48:23

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH v4 9/9] bug.h: Convert BUILD_BUG{,_ON} to use BUILD_BUG_ON_MSG

On 10/30/2012 08:02 PM, Josh Triplett wrote:
> On Tue, Oct 30, 2012 at 08:19:05PM +0100, Borislav Petkov wrote:
>> On Sun, Oct 28, 2012 at 03:57:15PM -0500, [email protected] wrote:
>>> Remove duplicate code by converting BUILD_BUG and BUILD_BUG_ON to just
>>> 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 | 17 +++--------------
>>> 1 files changed, 3 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/include/linux/bug.h b/include/linux/bug.h
>>> index 3bc1ddf..b58ba51 100644
>>> --- a/include/linux/bug.h
>>> +++ b/include/linux/bug.h
>>> @@ -81,14 +81,8 @@ struct pt_regs;
>>> #ifndef __OPTIMIZE__
>>> #define BUILD_BUG_ON(condition) __compiletime_error_fallback(condition)
>>> #else
>>> -#define BUILD_BUG_ON(condition) \
>>> - do { \
>>> - extern void __build_bug_on_failed(void) \
>>> - __compiletime_error("BUILD_BUG_ON failed"); \
>>> - __compiletime_error_fallback(condition); \
>>> - if (condition) \
>>> - __build_bug_on_failed(); \
>>> - } while(0)
>>> +#define BUILD_BUG_ON(condition) \
>>> + BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
>> Concatenating "condition" might not be very informative in all cases.
>> For example:
>>
>> BUILD_BUG_ON(1);
>>
>> Having __LINE__ is good enough IMHO.

Honestly, __LINE__ is only used to keep the function name unique. If
anything, I think that having it creates more confusion rather than adds
clarity since the error message will indicate the file and line number
anyway. So in other words, it is redundant without it being apparent
why. Of course, it's a very simple and portable mechanism to keep the
symbols unique. IMO, using __COUNTER__would be better for clarity
(since the number wouldn't relate to the anything real), but it is not
portable across versions of gcc (introduced in 4.4 or some such), so we
are using __LINE__.

> While it doesn't always help, it may help sometimes. Worst case,
> BUILD_BUG_ON(1) gives you no less information than it did before; best
> case, it gives you useful data.

Yeah, and depending upon what it's fed (and how much pre-processing had
been done on that) it can actually prove helpful because stringifying
condition can reveal pre-processing errors as well. So if you passed
some macro as the condition and it didn't expand the way you expected,
this error message will print out exactly how it expanded, up to the
point of having been passed to BUILD_BUG_ON. I don't know how much that
could potentially help, but for troubleshooting, I find extra
information helpful, more often than harmful.

Daniel

2012-10-31 11:06:18

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On Wed, Oct 31, 2012 at 12:34:45AM -0500, Daniel Santos wrote:
> Yes, the __build_bug_on_failed message is much more informative. This
> will only increase with these patches. For example, the line
>
> BUILD_BUG_ON(sizeof(*c) != 4);
>
> emits this error:
>
> arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
> arch/x86/kernel/cpu/amd.c:486:2: error: call to
> ‘__build_bug_on_failed_486’ declared with attribute error: BUILD_BUG_ON
> failed: sizeof(*c) != 4
> make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
> make: *** [arch/x86/kernel/cpu/amd.o] Error 2
>
> It's true that there is some redundancy in there as well as the
> gibberish line number embedded in the function name, but the end of the
> line spits out the exact statement that failed.

I guess that's as good as it gets. But it's fine IMO, it tells you
exactly what you need to know.

> But as far as rather the fallback is first or the __compiletime_error
> function is a matter of asthetics, since it's really an either/or
> situation. Either the __build_bug_on_failedxxx function will be
> declared with __attribute__((error(message))) and the fallback will
> expand to a no-op, or the fallback will produce code that (presumably
> always?) breaks the build. For insurance, a link-time error will occur
> if the fallback code fails to break the build.

Right, but my suggestion was to have the more informative message always
trigger first, if possible and if gcc supports it (practically, more
and more systems will be upgrading gcc which has the error attribute
with time) and have the less informative one be the more seldom one. The
"fallback" naming is just a minor issue.

This way, the error message would be precise on most modern toolchains.
Older toolchains will issue something about negative array size, which
is not really helpful so one would have to fire up an editor and
actually look at the code :).

> Realistically, a single macro could be defined in compiler*.h that
> encapsulates the entirety of this mechanism and only exposes a "black
> box" macro, that will simply expand to something that breaks the build
> in the most appropriate fashion based upon the version of gcc. In
> essence, the new BUILD_BUG_ON_MSG macro attempts to fill that roll.

Yes.

> I guess I'll fix it up (and address the emails on the other patches)
> and do a v5 then for the whole set? (is that the right way to resubmit
> with these corrections?)

Well, you could wait a couple of days first to gather feedback from
other people and then resend. This way you give chance to people to take
a look without them seeing too many versions of the patchset and getting
confused.

What I always do is send out the patchset, collect and discuss changes,
add in the required changes and test it while the discussions go on.
After they settle down (and they do in a couple of days, in most cases)
I then send out the newly tested version out.

That whole exercise takes more or less a week if you're doing other
stuff in between :)

--
Regards/Gruss,
Boris.

2012-10-31 16:38:34

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On 10/31/2012 06:06 AM, Borislav Petkov wrote:
> On Wed, Oct 31, 2012 at 12:34:45AM -0500, Daniel Santos wrote:
>> Yes, the __build_bug_on_failed message is much more informative. This
>> will only increase with these patches. For example, the line
>>
>> BUILD_BUG_ON(sizeof(*c) != 4);
>>
>> emits this error:
>>
>> arch/x86/kernel/cpu/amd.c: In function ‘early_init_amd’:
>> arch/x86/kernel/cpu/amd.c:486:2: error: call to
>> ‘__build_bug_on_failed_486’ declared with attribute error: BUILD_BUG_ON
>> failed: sizeof(*c) != 4
>> make[1]: *** [arch/x86/kernel/cpu/amd.o] Error 1
>> make: *** [arch/x86/kernel/cpu/amd.o] Error 2
>>
>> It's true that there is some redundancy in there as well as the
>> gibberish line number embedded in the function name, but the end of the
>> line spits out the exact statement that failed.
> I guess that's as good as it gets. But it's fine IMO, it tells you
> exactly what you need to know.
Yeah, that's what I'm thinking as well. Of course, I'm *always* happy
for somebody to come up with a superior solution! :)
>
>> But as far as rather the fallback is first or the __compiletime_error
>> function is a matter of asthetics, since it's really an either/or
>> situation. Either the __build_bug_on_failedxxx function will be
>> declared with __attribute__((error(message))) and the fallback will
>> expand to a no-op, or the fallback will produce code that (presumably
>> always?) breaks the build. For insurance, a link-time error will occur
>> if the fallback code fails to break the build.
> Right, but my suggestion was to have the more informative message always
> trigger first, if possible and if gcc supports it (practically, more
> and more systems will be upgrading gcc which has the error attribute
> with time) and have the less informative one be the more seldom one. The
> "fallback" naming is just a minor issue.
>
> This way, the error message would be precise on most modern toolchains.
> Older toolchains will issue something about negative array size, which
> is not really helpful so one would have to fire up an editor and
> actually look at the code :).

lol! :) Yeah, this is exactly how it should be behaving at this point,
although it's not too clear with the "fallback" macro being defined
elsewhere that it's doing nothing when the error attribute is
available. I suppose this is another reason to move the whole mechanism
to compiler*.h or add some comments to clarify what's going on.
>> Realistically, a single macro could be defined in compiler*.h that
>> encapsulates the entirety of this mechanism and only exposes a "black
>> box" macro, that will simply expand to something that breaks the build
>> in the most appropriate fashion based upon the version of gcc. In
>> essence, the new BUILD_BUG_ON_MSG macro attempts to fill that roll.
> Yes.

Hmmm, this gets tricky. So I think you are talking about a single
function-like macro that will create the built-time error. As I see it,
we'll need to move the entirety of __BUILD_BUG_INTERNAL (with the double
evaluation of condition fixed) into compiler*.h, even if we change the
name of the macro. The alternatives are to a.) further spitting out the
pieces of it into separate little macros (like
__compiletime_error_fallback), which I'm not fond of or b.) allow bug.h
to hold details about compiler functionality, which doesn't seem right
at all. Let me play with this some and see what I can figure out.

>
>> I guess I'll fix it up (and address the emails on the other patches)
>> and do a v5 then for the whole set? (is that the right way to resubmit
>> with these corrections?)
> Well, you could wait a couple of days first to gather feedback from
> other people and then resend. This way you give chance to people to take
> a look without them seeing too many versions of the patchset and getting
> confused.
>
> What I always do is send out the patchset, collect and discuss changes,
> add in the required changes and test it while the discussions go on.
> After they settle down (and they do in a couple of days, in most cases)
> I then send out the newly tested version out.
>
> That whole exercise takes more or less a week if you're doing other
> stuff in between :)

Ahh, helpful guidelines, thanks so much! :)

2012-11-03 18:10:30

by Daniel Santos

[permalink] [raw]
Subject: Re: [PATCH v4 6/9] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On 10/31/2012 06:06 AM, Borislav Petkov wrote:
>> Realistically, a single macro could be defined in compiler*.h that
>> encapsulates the entirety of this mechanism and only exposes a "black
>> box" macro, that will simply expand to something that breaks the build
>> in the most appropriate fashion based upon the version of gcc. In
>> essence, the new BUILD_BUG_ON_MSG macro attempts to fill that roll.
>
> Yes.

OK, so I have two solutions, one I like and one that I don't. Both of
these solutions will use the __compiletime_error_fallback macro, but
cleaned up slightly and it will only be used from within compiler.h (so
it becomes a private macro).

#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

And now let me present the solution that I do not like.

// compiler.h (after the above snippet)

#define __compiletime_error_invoke(condition, func) \
do { \
bool __cond = !!(condition); \
if (__cond) \
func(); \
__compiletime_error_fallback(__cond); \
} while (0)
#endif


// bug.h
...
#define __BUILD_BUG_INTERNAL(condition, msg, line) \
do { \
extern void __build_bug_on_failed_ ## line(void)\
__compiletime_error(msg); \
__compiletime_error_invoke(condition, \
__build_bug_on_failed_ ## line) \
} while (0)

#define _BUILD_BUG_INTERNAL(condition, msg, line) \
__BUILD_BUG_INTERNAL(condition, msg, line)

Rather than using __compiletime_error_fallback externally anywhere, we
just use it in another macro in compiler.h. Then, this
__compiletime_error_invoke macro is actually called to invoke the
error. This problem is that this is highly disjointed; we end up with
large parts of the implementation in both compiler.h and in bug.h.
(also, I think we would need another macro to expand the concatenation
of __build_bug_on_failed_ ## line, I didn't test the above).

If we are going to move this level of detail out of bug.h, I think we
should move *all* of it.

// compiler.h
...

#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 - some documentation...
*/
#define compiletime_assert(condition, msg) \
__compiletime_assert(condition, msg, __compiletime_assert_ ##
__LINE__)


// bug.h -- remove *BUILD_BUG_INTERNAL macros entirely and just chance this:

#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(cond, msg)

To me, this is much cleaner. It will give all compile-time assertions
the same base function name, but this shouldn't be much of a problem
since the new error attribute will spit out a nice detailed error
message at the point the error occurs and few people (if any?) should be
having to track this down from the link-time error. Even if you are
having to track it down just from a link-time error, it will tell you
the object file that the function was called from and the line number is
embedded in the function name, so it wouldn't be too hard. For example,
it's possible that the link-time error may be the only thing that breaks
on Intel or some other compilers (clang?). Having the full
implementation in compiler*.h allows for easier tweaking to suit various
compilers as well. (I may need to load up intel's latest compiler and
see how it is there).

Anyway, please let me know if you like it.

Daniel