2012-10-05 19:36:19

by Daniel Santos

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

This patch set is a dependency of the generic red-black tree patch set, which
I have now split up into three smaller sets.

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.), and to cleanup & add some needed features to bug.h.

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) - Will now generate a compile-time error and in
gcc-4.4+, also emits an error message containing the expression that failed.
o Add BUILD_BUG_ON_MSG(expr, msg) - Like BUILD_BUG_ON, except that you can
specify the error message that is emitted (again, gcc-4.4+).
o Add BUILD_BUG_ON_INTERNAL(expr, fn, msg) - contains a generic implementation
of BUILG_BUG_ON{_MSG}.
o Finally, the implementations of BUILD_BUG{,_ON,_ON_MSG} is consolidated,
eliminating duplicate code.

2012-10-05 19:43:01

by Daniel Santos

Subject: [PATCH v2 01/10] 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 8721704..4506d65 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-05 19:43:18

by Daniel Santos

Subject: [PATCH v2 03/10] 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]>
---
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 4506d65..bbfeb13 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-05 19:43:21

by Daniel Santos

Subject: [PATCH v2 02/10] 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-05 19:43:29

by Daniel Santos

Subject: [PATCH v2 04/10] bug.h: directly include linux/compiler.h

We are just including asm/bug.h and 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. (Later patches will
in this set use more macros defined in compiler*.h.)

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

diff --git a/include/linux/bug.h b/include/linux/bug.h
index aaac4bb..4bd74d8 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,
--
1.7.3.4

2012-10-05 19:43:42

by Daniel Santos

Subject: [PATCH v2 06/10] 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.

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

diff --git a/include/linux/bug.h b/include/linux/bug.h
index a03c3ef..3d4b564 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -43,24 +43,28 @@ 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.
+ * 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 pre-gcc-4.4) and then call an undefined
+ * function with the error attribute (should always creates an error 4.3+). 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-05 19:43:48

by Daniel Santos

Subject: [PATCH v2 08/10] 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 f8eae31..1b43ea2 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-05 19:43:57

by Daniel Santos

Subject: [PATCH v2 09/10] bug.h: Add BUILD_BUG_ON_MSG & BUILD_BUG_INTERNAL{,2}

Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
turned 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{,2}, 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 | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 1b43ea2..91bd9d5 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,31 @@ struct pt_regs;
*/
#define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))

+#define _CONCAT1(a, b) a##b
+#define CONCAT(a, b) _CONCAT1(a, b)
+#define UNIQUIFY(prefix) CONCAT(prefix, __LINE__)
+
+#define BUILD_BUG_INTERNAL2(condition, msg, fn) \
+ do { \
+ extern void fn (void) __compiletime_error(msg); \
+ __compiletime_error_fallback(condition); \
+ if (condition) \
+ fn(); \
+ } while (0)
+
+#define BUILD_BUG_INTERNAL(condition, msg, fn) \
+ BUILD_BUG_INTERNAL2(condition, msg, fn)
+
+/**
+ * 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, UNIQUIFY(__build_bug_on_failed_))
+
/**
* 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-05 19:44:04

by Daniel Santos

Subject: [PATCH v2 10/10] 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 91bd9d5..ee880e5 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -84,14 +84,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

/**
@@ -101,12 +95,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-05 19:44:37

by Daniel Santos

Subject: [PATCH v2 07/10] 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
(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 | 5 +++++
2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 3d4b564..f8eae31 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -56,13 +56,13 @@ struct pt_regs;
* link-time error, which is harder to 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 fd455aa..88ba201 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -296,6 +296,11 @@ 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) \
+ ((void)sizeof(char[1 - 2*!!(condition)]))
+#endif
+#ifndef __compiletime_error_fallback
+# define __compiletime_error_fallback(condition) (void)(0)
#endif
/*
* Prevent the compiler from merging or refetching accesses. The compiler
--
1.7.3.4

2012-10-05 19:45:00

by Daniel Santos

Subject: [PATCH v2 05/10] 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]>
---
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 4bd74d8..a03c3ef 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -74,7 +74,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 bbfeb13..e15985b 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 f430e41..fd455aa 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -297,9 +297,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-05 20:27:12

by Steven Rostedt

Subject: Re: [PATCH v2 0/10] Cleanup & new features for compiler*.h and bug.h

On Fri, 2012-10-05 at 14:35 -0500, [email protected] wrote:
> This patch set is a dependency of the generic red-black tree patch set, which
> I have now split up into three smaller sets.
>
> 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.), and to cleanup & add some needed features to bug.h.
>

A quick review of the patches look fine to me.

Acked-by: Steven Rostedt <[email protected]>

-- Steve

2012-10-05 20:59:07

by Borislav Petkov

Subject: Re: [PATCH v2 09/10] bug.h: Add BUILD_BUG_ON_MSG & BUILD_BUG_INTERNAL{,2}

On Fri, Oct 05, 2012 at 02:42:48PM -0500, [email protected] wrote:
> Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
> turned 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{,2}, 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 | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index 1b43ea2..91bd9d5 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,31 @@ struct pt_regs;
> */
> #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
>
> +#define _CONCAT1(a, b) a##b
> +#define CONCAT(a, b) _CONCAT1(a, b)

Let's call the indirection _CONCAT without the "1".

> +#define UNIQUIFY(prefix) CONCAT(prefix, __LINE__)
> +
> +#define BUILD_BUG_INTERNAL2(condition, msg, fn) \
> + do { \
> + extern void fn (void) __compiletime_error(msg); \
> + __compiletime_error_fallback(condition); \
> + if (condition) \
> + fn(); \
> + } while (0)
> +
> +#define BUILD_BUG_INTERNAL(condition, msg, fn) \
> + BUILD_BUG_INTERNAL2(condition, msg, fn)

Ditto. BUILD_BUG_INTERNAL2 should be __BUILD_BUG_INTERNAL and the one
calling it _BUILD_BUG_INTERNAL (with one underscore).

> +
> +/**
> + * 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, UNIQUIFY(__build_bug_on_failed_))

Btw, why are we adding the line at all? It is issued by gcc anyway:

cc -Wall macros.c -o macros
macros.c: In function ‘main’:
macros.c:22:1: error: ‘__build_bug_on_failed_22’ undeclared (first use in this function)
^^^^

It is in front of the filename here.

macros.c:22:1: note: each undeclared identifier is reported only once for each function it appears in
make: *** [macros] Error 1

Thanks.

--
Regards/Gruss,
Boris.

2012-10-05 20:59:36

by Josh Triplett

Subject: Re: [PATCH v2 07/10] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On Fri, Oct 05, 2012 at 02:42:46PM -0500, [email protected] wrote:
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -296,6 +296,11 @@ 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) \
> + ((void)sizeof(char[1 - 2*!!(condition)]))
> +#endif
> +#ifndef __compiletime_error_fallback
> +# define __compiletime_error_fallback(condition) (void)(0)

Might want to use do { } while (0) here, to force the use of a
semicolon and avoid the use of __compiletime_error_fallback in an
expression.

- Josh Triplett

2012-10-05 21:03:14

by Josh Triplett

Subject: Re: [PATCH v2 09/10] bug.h: Add BUILD_BUG_ON_MSG & BUILD_BUG_INTERNAL{,2}

On Fri, Oct 05, 2012 at 10:58:58PM +0200, Borislav Petkov wrote:
> On Fri, Oct 05, 2012 at 02:42:48PM -0500, [email protected] wrote:
> > Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
> > turned 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{,2}, 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 | 26 ++++++++++++++++++++++++++
> > 1 files changed, 26 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/bug.h b/include/linux/bug.h
> > index 1b43ea2..91bd9d5 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,31 @@ struct pt_regs;
> > */
> > #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
> >
> > +#define _CONCAT1(a, b) a##b
> > +#define CONCAT(a, b) _CONCAT1(a, b)
>
> Let's call the indirection _CONCAT without the "1".
>
> > +#define UNIQUIFY(prefix) CONCAT(prefix, __LINE__)
> > +
> > +#define BUILD_BUG_INTERNAL2(condition, msg, fn) \
> > + do { \
> > + extern void fn (void) __compiletime_error(msg); \
> > + __compiletime_error_fallback(condition); \
> > + if (condition) \
> > + fn(); \
> > + } while (0)
> > +
> > +#define BUILD_BUG_INTERNAL(condition, msg, fn) \
> > + BUILD_BUG_INTERNAL2(condition, msg, fn)
>
> Ditto. BUILD_BUG_INTERNAL2 should be __BUILD_BUG_INTERNAL and the one
> calling it _BUILD_BUG_INTERNAL (with one underscore).

Also, you don't need both the BUILD_BUG_INTERNAL and CONCAT/UNIQUIFY
macros. My original implementation just used the BUILD_BUG_INTERNAL
family of macros; if you'd rather rename them, by all means do so, but I
don't think you need two separate families of multiply-indirect macros.

> > +
> > +/**
> > + * 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, UNIQUIFY(__build_bug_on_failed_))
>
> Btw, why are we adding the line at all? It is issued by gcc anyway:
>
> cc -Wall macros.c -o macros
> macros.c: In function ‘main’:
> macros.c:22:1: error: ‘__build_bug_on_failed_22’ undeclared (first use in this function)

Because without that, you end up writing multiple prototypes for the
same function (__build_bug_on_failed) with different error attributes,
and GCC will ignore all but the last error attribute it sees, even with
a scoped prototype.

- Josh Triplett

2012-10-05 21:04:14

by Steven Rostedt

Subject: Re: [PATCH v2 09/10] bug.h: Add BUILD_BUG_ON_MSG & BUILD_BUG_INTERNAL{,2}

On Fri, 2012-10-05 at 22:58 +0200, Borislav Petkov wrote:
> On Fri, Oct 05, 2012 at 02:42:48PM -0500, [email protected] wrote:
> > Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
> > turned 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{,2}, 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 | 26 ++++++++++++++++++++++++++
> > 1 files changed, 26 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/bug.h b/include/linux/bug.h
> > index 1b43ea2..91bd9d5 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,31 @@ struct pt_regs;
> > */
> > #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
> >
> > +#define _CONCAT1(a, b) a##b
> > +#define CONCAT(a, b) _CONCAT1(a, b)
>
> Let's call the indirection _CONCAT without the "1".

You're stricter than I ;-)

>
> > +#define UNIQUIFY(prefix) CONCAT(prefix, __LINE__)
> > +
> > +#define BUILD_BUG_INTERNAL2(condition, msg, fn) \
> > + do { \
> > + extern void fn (void) __compiletime_error(msg); \
> > + __compiletime_error_fallback(condition); \
> > + if (condition) \
> > + fn(); \
> > + } while (0)
> > +
> > +#define BUILD_BUG_INTERNAL(condition, msg, fn) \
> > + BUILD_BUG_INTERNAL2(condition, msg, fn)
>
> Ditto. BUILD_BUG_INTERNAL2 should be __BUILD_BUG_INTERNAL and the one
> calling it _BUILD_BUG_INTERNAL (with one underscore).

I thought about commenting about the '2' too, but figured it's only used
internally by BUILD_BUG_INTERNAL and was really not too concerned about
such a trivial thing. It could have been 42 for all I care ;-)

>
> > +
> > +/**
> > + * 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, UNIQUIFY(__build_bug_on_failed_))
>
> Btw, why are we adding the line at all? It is issued by gcc anyway:
>
> cc -Wall macros.c -o macros
> macros.c: In function ‘main’:
> macros.c:22:1: error: ‘__build_bug_on_failed_22’ undeclared (first use in this function)
> ^^^^
>
> It is in front of the filename here.
>
> macros.c:22:1: note: each undeclared identifier is reported only once for each function it appears in
> make: *** [macros] Error 1

I was thinking that the number was added as a safety measure that the
line number would be shown for all versions of the compiler. I'm not
sure it is.

-- Steve

2012-10-06 04:28:57

by Daniel Santos

Subject: Re: [PATCH v2 07/10] compiler.h, bug.h: Prevent double error messages with BUILD_BUG{,_ON}

On 10/05/2012 03:59 PM, Josh Triplett wrote:
> On Fri, Oct 05, 2012 at 02:42:46PM -0500, [email protected] wrote:
>> --- a/include/linux/compiler.h
>> +++ b/include/linux/compiler.h
>> @@ -296,6 +296,11 @@ 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) \
>> + ((void)sizeof(char[1 - 2*!!(condition)]))
>> +#endif
>> +#ifndef __compiletime_error_fallback
>> +# define __compiletime_error_fallback(condition) (void)(0)
>
> Might want to use do { } while (0) here, to force the use of a
> semicolon and avoid the use of __compiletime_error_fallback in an
> expression.
Sure! But while we're here, we may want to consider a few other macros
in bug.h. These two are intended to be used as an expression:

#define BUILD_BUG_ON_ZERO(e) (sizeof(struct { int:-!!(e); }))
#define BUILD_BUG_ON_NULL(e) ((void *)sizeof(struct { int:-!!(e); }))

They are using a different technique to generate the compile-time error,
perhaps because the negative sized array wasn't always working past gcc
4.4? Either way, perhaps these can become

#define BUILD_BUG_ON_ZERO(e) ({BUILD_BUG_ON(e); 0;})
#define BUILD_BUG_ON_NULL(e) ({BUILD_BUG_ON(e); (void*)0;})

This would again give us our cute error message. However, I don't know
when this style of expression began to be supported (I know it's a gcc
extension), but I'm guessing it's pre gcc 3.2 because it's used in
kernel.h. Also:

#define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))

can become:

#define BUILD_BUG_ON_NOT_POWER_OF_2(n) \
BUILD_BUG_ON_MSG((n) == 0 || (((n) & ((n) - 1)) != 0), \
#n " not a power of two")

I think the only thing that would leave unfinished is the __OPTIMIZE__ check
in the BUILD_BUG_ON definition. This is a throw-back to the days before
BUILD_BUG_ON_NON_CONST (oops, that's still in another patch set). Well, if
you look at version 1 of this patch set, you'll see that it has that check,
since __builtin_constant_p never returns one in an unoptimized build.
However, that's a bit more work because we will need to examine every use of
BUILD_BUG_ON and __builtin_constant_p. I only found 2-3 last time I looked,
one of which was commented outwith the remark that it "breaks in funny
ways",
which we certainly already know about __builtin_constant_p. Another was a
pretty complicated expression, but I'll have to look them up again.

Please let me know what you think.

Daniel

2012-10-06 04:41:10

by Daniel Santos

Subject: Re: [PATCH v2 09/10] bug.h: Add BUILD_BUG_ON_MSG & BUILD_BUG_INTERNAL{,2}

On 10/05/2012 04:02 PM, Josh Triplett wrote:
> On Fri, Oct 05, 2012 at 10:58:58PM +0200, Borislav Petkov wrote:
>> On Fri, Oct 05, 2012 at 02:42:48PM -0500, [email protected] wrote:
>>> Add BUILD_BUG_ON_MSG which behaves like BUILD_BUG_ON (with optimizations
>>> turned 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{,2}, 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 | 26 ++++++++++++++++++++++++++
>>> 1 files changed, 26 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/linux/bug.h b/include/linux/bug.h
>>> index 1b43ea2..91bd9d5 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,31 @@ struct pt_regs;
>>> */
>>> #define BUILD_BUG_ON_INVALID(e) ((void)(sizeof((__force long)(e))))
>>>
>>> +#define _CONCAT1(a, b) a##b
>>> +#define CONCAT(a, b) _CONCAT1(a, b)
>>
>> Let's call the indirection _CONCAT without the "1".

No problem, naming conventions are good! :)

>>
>>> +#define UNIQUIFY(prefix) CONCAT(prefix, __LINE__)
>>> +
>>> +#define BUILD_BUG_INTERNAL2(condition, msg, fn) \
>>> + do { \
>>> + extern void fn (void) __compiletime_error(msg); \
>>> + __compiletime_error_fallback(condition); \
>>> + if (condition) \
>>> + fn(); \
>>> + } while (0)
>>> +
>>> +#define BUILD_BUG_INTERNAL(condition, msg, fn) \
>>> + BUILD_BUG_INTERNAL2(condition, msg, fn)
>>
>> Ditto. BUILD_BUG_INTERNAL2 should be __BUILD_BUG_INTERNAL and the one
>> calling it _BUILD_BUG_INTERNAL (with one underscore).
>
> Also, you don't need both the BUILD_BUG_INTERNAL and CONCAT/UNIQUIFY
> macros. My original implementation just used the BUILD_BUG_INTERNAL
> family of macros; if you'd rather rename them, by all means do so, but I
> don't think you need two separate families of multiply-indirect macros.

Yeah, I was thinking in terms of reusable macros. I'm kinda thinking the
kernel needs a header just for handy little macros, like concat,
uniquify, the
IS_EMPTY and IF_EMPTY macros of mine in rbtree.h, etc. There is a
stringify.h
that just contains a __stringify macro. But here, it's just verbose, so
I'll
change it back to how you had it.

>>> +
>>> +/**
>>> + * 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, UNIQUIFY(__build_bug_on_failed_))
>>
>> Btw, why are we adding the line at all? It is issued by gcc anyway:
>>
>> cc -Wall macros.c -o macros
>> macros.c: In function ‘main’:
>> macros.c:22:1: error: ‘__build_bug_on_failed_22’ undeclared (first use in this function)
>
> Because without that, you end up writing multiple prototypes for the
> same function (__build_bug_on_failed) with different error attributes,
> and GCC will ignore all but the last error attribute it sees, even with
> a scoped prototype.

Yeah, this is part of the trick to get non-existent functions with different
messages on their error attributes, so that each BUILD_BUG_ON-type macro can
have more helpful text in its error message. I don't know what's in your
macros.c, but it should have given you a much more shiny error message.

Daniel

2012-10-06 17:42:26

by Borislav Petkov

Subject: Re: [PATCH v2 01/10] compiler-gcc4.h: Reorder macros based upon gcc ver

On Fri, Oct 05, 2012 at 02:42:40PM -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]>
> ---
> 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 8721704..4506d65 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__)

This last hunk doesn't apply since mainline has a "4" above instead of
"3". And it has had a "4" since it got added by 4a3127693001c so unless
I'm missing something, how did the 3 appear in your patches?

> -#define __compiletime_warning(message) __attribute__((warning(message)))
> -#define __compiletime_error(message) __attribute__((error(message)))
> -#endif
> --
> 1.7.3.4

Thanks.

--
Regards/Gruss,
Boris.

2012-10-06 18:00:12

by Daniel Santos

Subject: Re: [PATCH v2 01/10] compiler-gcc4.h: Reorder macros based upon gcc ver

On 10/06/2012 12:42 PM, Borislav Petkov wrote:
>> @@ -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__)
>
> This last hunk doesn't apply since mainline has a "4" above instead of
> "3". And it has had a "4" since it got added by 4a3127693001c so unless
> I'm missing something, how did the 3 appear in your patches?
These are based against -mm, where another commit is already in that
changes it. I was told that since that commit was already in -mm, to
not include it in the patch set:

6c620cf1536a0ce6a83ecaaaf05298dcc0f7d440 (committed 2012-09-27)

This was probably (at least partially) the result of the fiasco I had
with my email.

2012-10-06 23:06:06

by Borislav Petkov

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On Fri, Oct 05, 2012 at 02:42:42PM -0500, [email protected] wrote:
> 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]>

Looks correct to me:

Acked-by: Borislav Petkov <[email protected]>

Thanks.

--
Regards/Gruss,
Boris.

2012-10-06 23:10:45

by Borislav Petkov

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On Fri, Oct 05, 2012 at 02:42:42PM -0500, [email protected] wrote:
> 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]>
> ---

[ … ]

> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> index 4506d65..bbfeb13 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

Did I miss something again? This "error" preprocessor function is
commented out here? Why?

--
Regards/Gruss,
Boris.

2012-10-07 18:28:09

by Daniel Santos

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On 10/06/2012 06:10 PM, Borislav Petkov wrote:
> On Fri, Oct 05, 2012 at 02:42:42PM -0500, [email protected] wrote:
>> 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]>
>> ---
>
> [ … ]
>
>> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
>> index 4506d65..bbfeb13 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
>
> Did I miss something again? This "error" preprocessor function is
> commented out here? Why?
We'll have to ask Andrew. Maybe so he can test on those versions of gcc?

commit d3ffe64a1dbcfe18b57f90f7c01c40c93d0a8b92
Author: Andrew Morton <[email protected]>
Date: Fri Sep 28 00:02:42 2012 +0000

a

Signed-off-by: Andrew Morton <[email protected]>

diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
index 934bc34..997fd8a 100644
--- a/include/linux/compiler-gcc4.h
+++ b/include/linux/compiler-gcc4.h
@@ -5,7 +5,7 @@
/* GCC 4.1.[01] miscompiles __weak */
#ifdef __KERNEL__
# if __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
-# error Your version of gcc miscompiles the __weak directive
+//# error Your version of gcc miscompiles the __weak directive
# endif
#endif


I can provide you a version of these patches rebased against Linus if
you like, which I am using to test since the -mm & -next trees aren't
working on my machine (hardware, .config and/or LVM/RAID setup). I
haven't put Walken's patches underneath them however.

2012-10-07 18:36:15

by Daniel Santos

Subject: Re: [PATCH v2 0/10] Cleanup & new features for compiler*.h and bug.h

I just realized that in my patch set header I didn't specify that this
version of the patches is based against -mm. However, I did not test
against -mm, I rebased them to mainline, merged some conflicts and made
some changes and tested there and I'm running that kernel now (that is,
with all of my patches, including fair scheduler using the generic
red-black tree).

This patch set builds fine against -mm & -next, but I just can't get
either of those to run correctly on my machine, even without my
patches. (Sorry, that I don't have an actual test machine at the moment.)

On 10/05/2012 02:35 PM, [email protected] wrote:
> This patch set is a dependency of the generic red-black tree patch set, which
> I have now split up into three smaller sets.
>
> 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.), and to cleanup & add some needed features to bug.h.
>
> 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) - Will now generate a compile-time error and in
> gcc-4.4+, also emits an error message containing the expression that failed.
> o Add BUILD_BUG_ON_MSG(expr, msg) - Like BUILD_BUG_ON, except that you can
> specify the error message that is emitted (again, gcc-4.4+).
> o Add BUILD_BUG_ON_INTERNAL(expr, fn, msg) - contains a generic implementation
> of BUILG_BUG_ON{_MSG}.
> o Finally, the implementations of BUILD_BUG{,_ON,_ON_MSG} is consolidated,
> eliminating duplicate code.

2012-10-07 19:43:14

by Borislav Petkov

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On Sun, Oct 07, 2012 at 01:27:58PM -0500, Daniel Santos wrote:
> > Did I miss something again? This "error" preprocessor function is
> > commented out here? Why?
> We'll have to ask Andrew. Maybe so he can test on those versions of gcc?
>
> commit d3ffe64a1dbcfe18b57f90f7c01c40c93d0a8b92
> Author: Andrew Morton <[email protected]>
> Date: Fri Sep 28 00:02:42 2012 +0000
>
> a
>
> Signed-off-by: Andrew Morton <[email protected]>
>
> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> index 934bc34..997fd8a 100644
> --- a/include/linux/compiler-gcc4.h
> +++ b/include/linux/compiler-gcc4.h
> @@ -5,7 +5,7 @@
> /* GCC 4.1.[01] miscompiles __weak */
> #ifdef __KERNEL__
> # if __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
> -# error Your version of gcc miscompiles the __weak directive
> +//# error Your version of gcc miscompiles the __weak directive
> # endif
> #endif

Ah, interesting. I think akpm has been redoing -mm couple times recently
so you probably caught a temporary thing.

> I can provide you a version of these patches rebased against Linus if
> you like, which I am using to test since the -mm & -next trees aren't
> working on my machine (hardware, .config and/or LVM/RAID setup). I
> haven't put Walken's patches underneath them however.

Nah, not necessary. I'd simply wait after the merge window closes and
everything settles down and then crank out a patchset against one of
the major trees (say -mm, linus or -next) so we can agree on the final
versions. AFAICT, the general design is fine - it's just the details
that need to be hammered out with precision.

Thanks.

--
Regards/Gruss,
Boris.

2012-10-07 20:21:38

by Daniel Santos

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On 10/07/2012 02:42 PM, Borislav Petkov wrote:
> On Sun, Oct 07, 2012 at 01:27:58PM -0500, Daniel Santos wrote:
>>> Did I miss something again? This "error" preprocessor function is
>>> commented out here? Why?
>> We'll have to ask Andrew. Maybe so he can test on those versions of gcc?
>>
>> commit d3ffe64a1dbcfe18b57f90f7c01c40c93d0a8b92
>> Author: Andrew Morton <[email protected]>
>> Date: Fri Sep 28 00:02:42 2012 +0000
>>
>> a
>>
>> Signed-off-by: Andrew Morton <[email protected]>
>>
>> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
>> index 934bc34..997fd8a 100644
>> --- a/include/linux/compiler-gcc4.h
>> +++ b/include/linux/compiler-gcc4.h
>> @@ -5,7 +5,7 @@
>> /* GCC 4.1.[01] miscompiles __weak */
>> #ifdef __KERNEL__
>> # if __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
>> -# error Your version of gcc miscompiles the __weak directive
>> +//# error Your version of gcc miscompiles the __weak directive
>> # endif
>> #endif
>
> Ah, interesting. I think akpm has been redoing -mm couple times recently
> so you probably caught a temporary thing.

I was guessing something like that, but I figured he had a reason for
doing it so I didn't dare ask the master! :)

>
>> I can provide you a version of these patches rebased against Linus if
>> you like, which I am using to test since the -mm & -next trees aren't
>> working on my machine (hardware, .config and/or LVM/RAID setup). I
>> haven't put Walken's patches underneath them however.
>
> Nah, not necessary. I'd simply wait after the merge window closes and
> everything settles down and then crank out a patchset against one of
> the major trees (say -mm, linus or -next) so we can agree on the final
> versions. AFAICT, the general design is fine - it's just the details
> that need to be hammered out with precision.
>
> Thanks.
If I had more time & energy for this part, I would try to find all uses
of BUILD_BUG_ON with __builtin_const_p, remove the __OPTIMIZE__ check
from BUILD_BUG_ON, put my BUILD_BUG_ON_NON_CONST macros back into this
patch set and adjust any other code in the kernel to not fail
un-optimized. Also, I would figure out the reason for this commented
out check (arch/powerpc/kvm/timing.h:52):

/* The BUILD_BUG_ON below breaks in funny ways, commented out
* for now ... -BenH
BUILD_BUG_ON(!__builtin_constant_p(type));
*/

and correct it with the appropriate new macro. However, one of my
biggest problems is staying on task, perhaps partially due to having
ADHD. In fact, *this* project (the generic red-black trees) got started
when I was exploring the possibilities of running parts of Wine in the
kernel, when I was dismayed by the lack of genericity in the kernel's
red-black tree code, so it's actually a tangent! :) Don't get me wrong,
I don't regret it. It has been a marvelous adventure in the
possibilities of the C language on modern compilers. But after I finish
this up, I plan on writing up a paper on the "C metaprogramming"
techniques I seem to have discovered and then getting back to my Wine
project. There are many other nify compile-time tricks that I
discovered in the exploration process that I didn't need to use here,
but which are worth documenting.

So basically, I know that I need to try to get this thing wrapped up.

Daniel

2012-10-09 18:42:03

by Andrew Morton

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On Sun, 07 Oct 2012 13:27:58 -0500
Daniel Santos <[email protected]> wrote:

> We'll have to ask Andrew. Maybe so he can test on those versions of gcc?
>
> commit d3ffe64a1dbcfe18b57f90f7c01c40c93d0a8b92
> Author: Andrew Morton <[email protected]>
> Date: Fri Sep 28 00:02:42 2012 +0000
>
> a
>
> Signed-off-by: Andrew Morton <[email protected]>
>
> diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> index 934bc34..997fd8a 100644
> --- a/include/linux/compiler-gcc4.h
> +++ b/include/linux/compiler-gcc4.h
> @@ -5,7 +5,7 @@
> /* GCC 4.1.[01] miscompiles __weak */
> #ifdef __KERNEL__
> # if __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
> -# error Your version of gcc miscompiles the __weak directive
> +//# error Your version of gcc miscompiles the __weak directive

hm, yeah, sorry, I use various old crufty cross-compilers.

There are quite a number of patches in -mm which aren't included in
linux-next and that's one of them. The
NEXT_PATCHES_START/NEXT_PATCHES_END markers in the series file identify
the patches which Stephen selects.

2012-10-09 19:46:06

by Josh Triplett

Subject: Re: [PATCH v2 03/10] compiler-gcc{3,4}.h: Use GCC_VERSION macro

On Tue, Oct 09, 2012 at 11:41:58AM -0700, Andrew Morton wrote:
> On Sun, 07 Oct 2012 13:27:58 -0500
> Daniel Santos <[email protected]> wrote:
>
> > We'll have to ask Andrew. Maybe so he can test on those versions of gcc?
> >
> > commit d3ffe64a1dbcfe18b57f90f7c01c40c93d0a8b92
> > Author: Andrew Morton <[email protected]>
> > Date: Fri Sep 28 00:02:42 2012 +0000
> >
> > a
> >
> > Signed-off-by: Andrew Morton <[email protected]>
> >
> > diff --git a/include/linux/compiler-gcc4.h b/include/linux/compiler-gcc4.h
> > index 934bc34..997fd8a 100644
> > --- a/include/linux/compiler-gcc4.h
> > +++ b/include/linux/compiler-gcc4.h
> > @@ -5,7 +5,7 @@
> > /* GCC 4.1.[01] miscompiles __weak */
> > #ifdef __KERNEL__
> > # if __GNUC_MINOR__ == 1 && __GNUC_PATCHLEVEL__ <= 1
> > -# error Your version of gcc miscompiles the __weak directive
> > +//# error Your version of gcc miscompiles the __weak directive
>
> hm, yeah, sorry, I use various old crufty cross-compilers.

Ah, for build testing where you don't actually care if the resulting
kernel runs?

- Josh Triplett

2012-10-18 02:27:05

by David Rientjes

Subject: Re: [PATCH v2 01/10] compiler-gcc4.h: Reorder macros based upon gcc ver

On Sat, 6 Oct 2012, Daniel Santos wrote:

> These are based against -mm, where another commit is already in that
> changes it. I was told that since that commit was already in -mm, to
> not include it in the patch set:
>
> 6c620cf1536a0ce6a83ecaaaf05298dcc0f7d440 (committed 2012-09-27)
>
> This was probably (at least partially) the result of the fiasco I had
> with my email.
>

Yeah, this patch applies fine on linux-next, which is the correct thing to
do if you're building upon already queued patches.