include/linux/compiler*.h: Use feature checking instead of version checks for attributes
Subsystems (3)
PATCH 1/1 include/linux/compiler*.h: Use feature checking instead of version checks for attributes
Instead of using version checks per-compiler to define (or not) each attribute, use __has_attribute to test for them, following the cleanup started with commit 815f0ddb346c ("include/linux/compiler*.h: make compiler-*.h mutually exclusive").
All the attributes that are fairly common/standard (i.e. those that do not require extra logic to define them) have been moved to a new file include/linux/compiler_attributes.h. The attributes have been sorted and divided between "required" and "optional".
Further, attributes that are already supported in gcc >= 4.6 and recent clang were simply made to be required (instead of testing for them):
* always_inline
* const (pure was already "required", by the way)
* gnu_inline
Finally, some other bits were cleaned up in the process:
* __optimize: removed (unused in the whole kernel tree)
* __must_be_array: removed from -gcc and -clang (identical), moved to _types (it depends on the unconditionally used __builtin_types_compatible_p
* Removes unneeded underscores on the attributes' names
There are some things that can be further cleaned up afterwards:
* __attribute_const__: rename to __const
* __noretpoline: avoid checking for defined(__notrepoline)
* __compiletime_warning/error: they are in two different places, -gcc and compiler.h.
* sparse' attributes could potentially go into the end of attributes.h too (as another separate section).
Compile-tested an x86 allmodconfig for a while with gcc 8.2.0 and 4.6.4.
---
*Seems* to work, but note that I did not finish the entire allmodconfig :)
A few things could be splitted into their own patch, but I kept it as one for simplicity for a first review.
These files are becoming no-headaches-readable again, yay.
include/linux/compiler-clang.h | 5 -- include/linux/compiler-gcc.h | 60 ---------------- include/linux/compiler-intel.h | 6 -- include/linux/compiler.h | 4 -- include/linux/compiler_attributes.h | 105 ++++++++++++++++++++++++++++ include/linux/compiler_types.h | 91 ++++-------------------- 6 files changed, 118 insertions(+), 153 deletions(-) create mode 100644 include/linux/compiler_attributes.h
include/linux/compiler-clang.h +0 -5
@@ -21,8 +21,6 @@#define __SANITIZE_ADDRESS__#endif -#define __no_sanitize_address __attribute__((no_sanitize("address"))) -/* * Not all versions of clang implement the the type-generic versions * of the builtin overflow checkers. Fortunately, clang implements
@@ -41,6 +39,3 @@* compilers, like ICC. */ #define barrier () __asm__ __volatile__("" : : : "memory") -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#define __assume_aligned (a, ...) \ - __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))
include/linux/compiler-gcc.h +0 -60
94 lines not on this page · raw message
include/linux/compiler-intel.h +0 -6
@@ -37,9 +37,3 @@/* icc has this, but it's called _bswap16 */ #define __HAVE_BUILTIN_BSWAP16__#define __builtin_bswap16 _bswap16 - -/* The following are for compatibility with GCC, from compiler-gcc.h, -* and may be redefined here because they should not be shared with other -* compilers, like clang. -*/ -#define __visible __attribute__((externally_visible))
include/linux/compiler.h +0 -4
@@ -301,10 +301,6 @@ static inline void *offset_to_ptr(const int *off)#endif /* __ASSEMBLY__ */ -#ifndef __optimize -# define __optimize (level) -#endif -/* Compile time object size, -1 for unknown */ #ifndef __compiletime_object_size# define __compiletime_object_size (obj) -1
include/linux/compiler_attributes.h new file +105 -0
105 lines not on this page · raw message
include/linux/compiler_types.h +13 -78
147 lines not on this page · raw message
Signature · 1 line
2.17.1
Hi,
A couple of extra notes on my patch.
Quoted 5 lines · Miguel Ojeda
include/linux/compiler_attributes.h new file +5
-0+/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5,
include/linux/compiler_attributes.h line 44
Quoted 10 lines
include/linux/compiler_attributes.h new file +10
-0+* we implement it by hand. +*/ +#ifndef __has_attribute +#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1 +#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0
Note that:
- assume_aligned came with gcc 4.9
- no_sanitize_address came with gcc 4.8
So if we feel it is important to have them there (before gcc 5), we would need here a quick version check here.
Quoted 9 lines
include/linux/compiler_attributes.h new file +9
-0+#endif + +/* +* __assume_aligned(n, k): Tell the optimizer that the returned +* pointer can be assumed to be k modulo n. The second argument is +* optional (default 0), so we use a variadic macro to make the +* shorthand. +* +* Beware: Do not apply this to functions which may return
include/linux/compiler_attributes.h line 63
Quoted 10 lines
include/linux/compiler_attributes.h new file +10
-0+* ERR_PTRs. Also, it is probably unwise to apply it to functions +* returning extra information in the low bits (but in that case the +* compiler should see some alignment anyway, when the return value is +* massaged by 'flags = ptr & 3; ptr &= ~3;'). +*/ +#if __has_attribute (assume_aligned) +#define __assume_aligned (a, ...) __attribute__((assume_aligned(a, ## __VA_ARGS__))) +#else +#define __assume_aligned (a, ...) +#endif
Beforehand, these were !defined(__CHECKER__). I am not sure if sparse complains too much about it. Maybe sparse could learn __has_attribute.
Cheers,
Miguel
Quoted 3 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").
Very nice. Thank you Miguel.
trivia:
I believe the alphabetic sorting of the required attributes makes reading by use a bit difficult and I would prefer that various required attributes are sorted by logical use instead.
ie: keep noinline and __always_inline together,
keep __used and __always_unused together,
etc...
Hi Joe,
Quoted 6 lines · Joe Perches
Quoted 3 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").Very nice. Thank you Miguel.
Thanks!
Quoted 12 lines
trivia:
I believe the alphabetic sorting of the required attributes makes reading by use a bit difficult and I would prefer that various required attributes are sorted by logical use instead.
ie: keep noinline and
__always_inlinetogether,
keep__usedand__always_unusedtogether,
etc...
Both ways are fine with me --- I sorted them as an attempt to avoid the file evolving into a mess again in the upcoming years :-)
Half-joking: it may also be a good way to avoid people "guessing" what the attributes do by name and, instead, consulting the docs (either the compiler's, or a Doc/ file maybe).
Cheers,
Miguel
Nick Desaulniers
reviewerQuoted 9 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").All the attributes that are fairly common/standard (i.e. those that do not require extra logic to define them) have been moved to a new file
include/linux/compiler_attributes.h. The attributes have been sorted and divided between "required" and "optional".
Nice! Thanks Miguel. Regarding sorting, I'm happy with that. In fact, some of the comments can be removed IMO, as the attributes have common definitions in the docs (maybe an added link to the gcc and clang attribute docs at the top of the file rather than per attribute comments).
Quoted 6 lines
Further, attributes that are already supported in gcc >= 4.6 and recent clang were simply made to be required (instead of testing for them):
*always_inline
* const (pure was already "required", by the way)
*gnu_inline
There's an important test for gnu_inline that isn't checking that it's supported, but rather what the implicit behavior is depending on which C standard is being used. It's important not to remove that.
Quoted 3 lines
Finally, some other bits were cleaned up in the process:
*__optimize: removed (unused in the whole kernel tree)
A+ for removing dead code. I also don't see it used anywhere.
Quoted 1 line
*
__must_be_array: removed from -gcc and -clang (identical), moved to_types
Yep, uses a builtin (we should add guards for that, later, in a similar style change that guards the use of builtins). Looks good.
Quoted 2 lines
(it depends on the unconditionally used
__builtin_types_compatible_p
* Removes unneeded underscores on the attributes' names
That doesn't sound right, but lets see what you mean by that.
Quoted 3 lines
There are some things that can be further cleaned up afterwards:
*__attribute_const__: rename to__const
This doesn't look correct to me; the kernel is full of call sites for __attribute_const__. You can't rename the definition without renaming all of the call sites (and that would be too big a change for this patch, IMO). Skip the rename, and it also looks like you just removed it outright (Oops).
Quoted 7 lines
*
__noretpoline: avoid checking fordefined(__notrepoline)
* __compiletime_warning/error: they are in two different places, -gcc andcompiler.h.
* sparse' attributes could potentially go into the end ofattributes.htoo (as another separate section).Compile-tested an x86 allmodconfig for a while with gcc 8.2.0 and 4.6.4.
It's important to test changes to compiler-clang.h with clang. ;)
Quoted 14 lines
Cc Eli Friedman <[email protected]>Cc Christopher Li <[email protected]>Cc Kees Cook <[email protected]>Cc Ingo Molnar <[email protected]>Cc Geert Uytterhoeven <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Joe Perches <[email protected]>Cc Dominique Martinet <[email protected]>Cc Nick Desaulniers <[email protected]>Cc Linus Torvalds <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>
Quoted 7 lines
---
*Seems* to work, but note that I did not finish the entire allmodconfig :)A few things could be splitted into their own patch, but I kept it as one for simplicity for a first review.
These files are becoming no-headaches-readable again, yay.
A+
Quoted 408 lines
include/linux/compiler-clang.h | 5 -- include/linux/compiler-gcc.h | 60 ---------------- include/linux/compiler-intel.h | 6 -- include/linux/compiler.h | 4 -- include/linux/compiler_attributes.h | 105 ++++++++++++++++++++++++++++ include/linux/compiler_types.h | 91 ++++-------------------- 6 files changed, 118 insertions(+), 153 deletions(-) create mode 100644 include/linux/compiler_attributes.hinclude/linux/compiler-clang.h +0
-5@@ -21,8 +21,6 @@#define __SANITIZE_ADDRESS__#endif -#define __no_sanitize_address __attribute__((no_sanitize("address"))) -/* * Not all versions of clang implement the the type-generic versions * of the builtin overflow checkers. Fortunately, clang implements @@ -41,6 +39,3 @@* compilers, like ICC. */ #define barrier () __asm__ __volatile__("" : : : "memory") -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#define __assume_aligned (a, ...) \ - __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))include/linux/compiler-gcc.h +0
-60@@ -68,13 +68,6 @@*/ #define uninitialized_var (x) x = x -#ifdef __CHECKER__ -#define __must_be_array (a) 0 -#else -/* &a[0] degrades to a pointer: a different type from an array */ -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#endif -#ifdef RETPOLINE #define __noretpoline __attribute__((indirect_branch("keep")))#endif @@ -95,8 +88,6 @@#define __UNIQUE_ID (prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) -#define __optimize (level) __attribute__((__optimize__(level))) -#define __compiletime_object_size (obj) __builtin_object_size(obj, 0)#ifndef __CHECKER__@@ -133,9 +124,6 @@__builtin_unreachable (); \ } while (0) -/* Mark a function definition as prohibited from being cloned. */ -#define __noclone __attribute__((__noclone__, __optimize__("no-tracer"))) -#if defined(RANDSTRUCT_PLUGIN ) && !defined(__CHECKER__)#define __randomize_layout __attribute__((randomize_layout))#define __no_randomize_layout __attribute__((no_randomize_layout))@@ -144,32 +132,6 @@#define randomized_struct_fields_end } __randomize_layout;#endif -/* -* When used with Link Time Optimization, gcc can optimize away C functions or -* variables which are referenced only from assembly code. __visible tells the -* optimizer that something else uses this function or variable, thus preventing -* this. -*/ -#define __visible __attribute__((externally_visible)) - -/* gcc version specific checks */ - -#if GCC_VERSION >=40900 && !defined(__CHECKER__) -/* -* __assume_aligned(n, k): Tell the optimizer that the returned -* pointer can be assumed to be k modulo n. The second argument is -* optional (default 0), so we use a variadic macro to make the -* shorthand. -* -* Beware: Do not apply this to functions which may return -* ERR_PTRs. Also, it is probably unwise to apply it to functions -* returning extra information in the low bits (but in that case the -* compiler should see some alignment anyway, when the return value is -* massaged by 'flags = ptr & 3; ptr &= ~3;'). -*/ -#define __assume_aligned (a, ...) __attribute__((__assume_aligned__(a, ## __VA_ARGS__))) -#endif -/* * GCC 'asm goto' miscompiles certain code sequences: * @@ -201,32 +163,10 @@#define KASAN_ABI_VERSION 3#endif -#if GCC_VERSION >=40902 -/* -* Tell the compiler that address safety instrumentation (KASAN) -* should not be applied to that function. -* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 -*/ -#define __no_sanitize_address __attribute__((no_sanitize_address)) -#endif -#if GCC_VERSION >=50100 -/* -* Mark structures as requiring designated initializers. -* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html -*/ -#define __designated_init __attribute__((designated_init))#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1#endif -#if !defined(__noclone) -#define __noclone/* not needed */ -#endif - -#if !defined(__no_sanitize_address) -#define __no_sanitize_address -#endif -/* * Turn individual warnings and errors on and off locally, depending * on version. include/linux/compiler-intel.h +0
-6@@ -37,9 +37,3 @@/* icc has this, but it's called _bswap16 */ #define __HAVE_BUILTIN_BSWAP16__#define __builtin_bswap16 _bswap16 - -/* The following are for compatibility with GCC, from compiler-gcc.h, -* and may be redefined here because they should not be shared with other -* compilers, like clang. -*/ -#define __visible __attribute__((externally_visible))include/linux/compiler.h +0
-4@@ -301,10 +301,6 @@ static inline void *offset_to_ptr(const int *off)#endif /* __ASSEMBLY__ */ -#ifndef __optimize -# define __optimize (level) -#endif -/* Compile time object size, -1 for unknown */ #ifndef __compiletime_object_size# define __compiletime_object_size (obj) -1include/linux/compiler_attributes.h new file +105
-0@@ -0,0 +1,105 @@ +#ifndef __LINUX_COMPILER_ATTRIBUTES_H +#define __LINUX_COMPILER_ATTRIBUTES_H + +/* This file is meant to be sorted. */ + +/* +* Required attributes: your compiler must support these. +*/ +#define __alias (symbol) __attribute__((alias(#symbol))) +#define __aligned (x) __attribute__((aligned(x))) +#define __aligned_largest __attribute__((aligned)) +#define __always_inline inline __attribute__((always_inline)) +#define __always_unused __attribute__((unused)) +#define __attribute_const__ __attribute__((const)) +#define __cold __attribute__((cold)) +#define __gnu_inline __attribute__((gnu_inline)) +#define __malloc __attribute__((malloc)) +#define __maybe_unused __attribute__((unused)) +#define __mode (x) __attribute__((mode(x))) +#define noinline __attribute__((noinline)) +#define __noreturn __attribute__((noreturn)) +#define __packed __attribute__((packed)) +#define __printf (a, b) __attribute__((format(printf, a, b))) +#define __pure __attribute__((pure)) +#define __scanf (a, b) __attribute__((format(scanf, a, b))) +#define __section (S ) __attribute__((section(#S))) +#define __used __attribute__((used)) +#define __weak __attribute__((weak)) + +/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5, +* we implement it by hand. +*/ +#ifndef __has_attribute +#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1 +#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0 +#endif + +/* +* __assume_aligned(n, k): Tell the optimizer that the returned +* pointer can be assumed to be k modulo n. The second argument is +* optional (default 0), so we use a variadic macro to make the +* shorthand. +* +* Beware: Do not apply this to functions which may return +* ERR_PTRs. Also, it is probably unwise to apply it to functions +* returning extra information in the low bits (but in that case the +* compiler should see some alignment anyway, when the return value is +* massaged by 'flags = ptr & 3; ptr &= ~3;'). +*/ +#if __has_attribute (assume_aligned) +#define __assume_aligned (a, ...) __attribute__((assume_aligned(a, ## __VA_ARGS__))) +#else +#define __assume_aligned (a, ...) +#endif + +/* +* Mark structures as requiring designated initializers. +* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html +*/ +#if __has_attribute (designated_init) +#define __designated_init __attribute__((designated_init)) +#else +#define __designated_init +#endif + +/* +* When used with Link Time Optimization, gcc can optimize away C functions or +* variables which are referenced only from assembly code. __visible tells the +* optimizer that something else uses this function or variable, thus preventing +* this. +*/ +#if __has_attribute (externally_visible) +#define __visible __attribute__((externally_visible)) +#else +#define __visible +#endif + +/* Mark a function definition as prohibited from being cloned. */ +#if __has_attribute (noclone) &&__has_attribute (optimize) +#define __noclone __attribute__((noclone, optimize("no-tracer"))) +#else +#define __noclone +#endif + +/* +* Tell the compiler that address safety instrumentation (KASAN) +* should not be applied to that function. +* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 +*/ +#if __has_attribute (no_sanitize_address) +#define __no_sanitize_address __attribute__((no_sanitize_address)) +#else +#define __no_sanitize_address +#endif + +#endif /* __LINUX_COMPILER_ATTRIBUTES_H */ include/linux/compiler_types.h +5
-72@@ -54,6 +54,9 @@ extern void __chk_io_ptr(const volatile void __iomem *);#ifdef __KERNEL__ +/* Attributes */ +#include <linux/compiler_attributes.h> +/* Compiler specific macros. */ #ifdef __clang__#include <linux/compiler-clang.h> @@ -78,12 +81,6 @@ extern void __chk_io_ptr(const volatile void __iomem *);#include <asm/compiler.h> #endif -/* -* Generic compiler-independent macros required for kernel -* build go below this comment. Actual compiler/compiler version -* specific implementations come from the above header files -*/ -struct ftrace_branch_data {const char *func;const char *file;@@ -119,10 +116,6 @@ struct ftrace_likely_data {* compilers. We don't consider that to be an error, so set them to nothing. * For example, some of them are for compiler specific plugins. */ -#ifndef __designated_init -# define __designated_init -#endif -#ifndef __latent_entropy# define __latent_entropy#endif @@ -140,17 +133,6 @@ struct ftrace_likely_data {# define randomized_struct_fields_end#endif -#ifndef __visible -#define __visible -#endif - -/* -* Assume alignment of return value. -*/ -#ifndef __assume_aligned -#define __assume_aligned (a, ...) -#endif -/* Are two types/vars the same type (ignoring qualifiers)? */ #define __same_type (a, b) __builtin_types_compatible_p(typeof(a), typeof(b))@@ -159,14 +141,6 @@ struct ftrace_likely_data { (sizeof (t) ==sizeof (char ) ||sizeof (t) ==sizeof (short ) || \sizeof (t) ==sizeof (int ) ||sizeof (t) ==sizeof (long )) -#ifndef __attribute_const__ -#define __attribute_const__ __attribute__((__const__)) -#endif - -#ifndef __noclone -#define __noclone -#endif - /* Helpers for emitting diagnostics in pragmas. */#ifndef __diag#define __diag (string)@@ -186,34 +160,6 @@ struct ftrace_likely_data {#define __diag_error (compiler, version, option, comment) \ __diag_ ## compiler(version, error, option) -/* -* From the GCC manual: -* -* Many functions have no effects except the return value and their -* return value depends only on the parameters and/or global -* variables. Such a function can be subject to common subexpression -* elimination and loop optimization just as an arithmetic operator -* would be. -* [...] -*/ -#define __pure __attribute__((pure)) -#define __aligned (x) __attribute__((aligned(x))) -#define __aligned_largest __attribute__((aligned)) -#define __printf (a, b) __attribute__((format(printf, a, b))) -#define __scanf (a, b) __attribute__((format(scanf, a, b))) -#define __maybe_unused __attribute__((unused)) -#define __always_unused __attribute__((unused)) -#define __mode (x) __attribute__((mode(x))) -#define __malloc __attribute__((__malloc__)) -#define __used __attribute__((__used__)) -#define __noreturn __attribute__((noreturn)) -#define __packed __attribute__((packed)) -#define __weak __attribute__((weak)) -#define __alias (symbol) __attribute__((alias(#symbol))) -#define __cold __attribute__((cold)) -#define __section (S ) __attribute__((__section__(#S))) - -#ifdef CONFIG_ENABLE_MUST_CHECK #define __must_check __attribute__((warn_unused_result))#else @@ -228,18 +174,6 @@ struct ftrace_likely_data {#define __compiler_offsetof (a, b) __builtin_offsetof(a, b) -/* -* Feature detection for gnu_inline (gnu89 extern inline semantics). Either -* __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, -* and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not -* defined so the gnu89 semantics are the default. -*/ -#ifdef __GNUC_STDC_INLINE__ -# define __gnu_inline __attribute__((gnu_inline)) -#else -# define __gnu_inline -#endif -/* * Force always-inline if the user requests it so via the .config. * GCC does not warn about unused static inline functions for @@ -254,19 +188,13 @@ struct ftrace_likely_data {*/ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING ) || \ !defined(CONFIG_OPTIMIZE_INLINING ) -#define inline \ - inline __attribute__((always_inline, unused)) notrace __gnu_inline +#define inline inline __attribute__((always_inline, unused)) notrace __gnu_inline#else -#define inline inline __attribute__((unused)) notrace __gnu_inline +#define inline inline __attribute__((unused)) notrace __gnu_inline
include/linux/compiler_types.h struct ftrace_likely_data { · line 197
Quoted 10 lines
include/linux/compiler_types.h +1
-6#endif #define __inline__ inline -#define __inline inline -#define noinline __attribute__((noinline)) - -#ifndef __always_inline -#define __always_inline inline __attribute__((always_inline)) -#endif +#define __inline inline
All of the changes to inline should not be removed, see above. It's important to make this work correctly regardless of C standard used.
Quoted 16 lines
include/linux/compiler_types.h +7
-1/* * Rather then using noinline to prevent stack consumption, use @@ -274,4 +202,11 @@ struct ftrace_likely_data {*/ #define noinline_for_stack noinline +#ifdef __CHECKER__ +#define __must_be_array (a) 0 +#else +/* &a[0] degrades to a pointer: a different type from an array */ +#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +#endif +#endif /* __LINUX_COMPILER_TYPES_H */ --
Quoted 2 lines
2.17.1
With the above changes requested, I'm super happy with the spirit of this patch, and look forward to a v2. Thanks again Miguel!
Signature · 2 lines
Thanks, ~Nick Desaulniers
Nick Desaulniers
reviewerQuoted 295 lines · Nick Desaulniers
Quoted 9 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").All the attributes that are fairly common/standard (i.e. those that do not require extra logic to define them) have been moved to a new file
include/linux/compiler_attributes.h. The attributes have been sorted and divided between "required" and "optional".Nice! Thanks Miguel. Regarding sorting, I'm happy with that. In fact, some of the comments can be removed IMO, as the attributes have common definitions in the docs (maybe an added link to the gcc and clang attribute docs at the top of the file rather than per attribute comments).
Quoted 6 lines
Further, attributes that are already supported in gcc >= 4.6 and recent clang were simply made to be required (instead of testing for them):
*always_inline
* const (pure was already "required", by the way)
*gnu_inlineThere's an important test for
gnu_inlinethat isn't checking that it's supported, but rather what the implicit behavior is depending on which C standard is being used. It's important not to remove that.Quoted 3 lines
Finally, some other bits were cleaned up in the process:
*__optimize: removed (unused in the whole kernel tree)A+ for removing dead code. I also don't see it used anywhere.
Quoted 1 line
*
__must_be_array: removed from -gcc and -clang (identical), moved to_typesYep, uses a builtin (we should add guards for that, later, in a similar style change that guards the use of builtins). Looks good.
Quoted 2 lines
(it depends on the unconditionally used
__builtin_types_compatible_p
* Removes unneeded underscores on the attributes' namesThat doesn't sound right, but lets see what you mean by that.
Quoted 3 lines
There are some things that can be further cleaned up afterwards:
*__attribute_const__: rename to__constThis doesn't look correct to me; the kernel is full of call sites for
__attribute_const__. You can't rename the definition without renaming all of the call sites (and that would be too big a change for this patch, IMO). Skip the rename, and it also looks like you just removed it outright (Oops).Quoted 7 lines
*
__noretpoline: avoid checking fordefined(__notrepoline)
* __compiletime_warning/error: they are in two different places, -gcc andcompiler.h.
* sparse' attributes could potentially go into the end ofattributes.htoo (as another separate section).Compile-tested an x86 allmodconfig for a while with gcc 8.2.0 and 4.6.4.
It's important to test changes to
compiler-clang.hwith clang. ;)Quoted 14 lines
Cc Eli Friedman <[email protected]>Cc Christopher Li <[email protected]>Cc Kees Cook <[email protected]>Cc Ingo Molnar <[email protected]>Cc Geert Uytterhoeven <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Joe Perches <[email protected]>Cc Dominique Martinet <[email protected]>Cc Nick Desaulniers <[email protected]>Cc Linus Torvalds <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>Quoted 7 lines
---
*Seems* to work, but note that I did not finish the entire allmodconfig :)A few things could be splitted into their own patch, but I kept it as one for simplicity for a first review.
These files are becoming no-headaches-readable again, yay.
A+
Quoted 205 lines
include/linux/compiler-clang.h | 5 -- include/linux/compiler-gcc.h | 60 ---------------- include/linux/compiler-intel.h | 6 -- include/linux/compiler.h | 4 -- include/linux/compiler_attributes.h | 105 ++++++++++++++++++++++++++++ include/linux/compiler_types.h | 91 ++++-------------------- 6 files changed, 118 insertions(+), 153 deletions(-) create mode 100644 include/linux/compiler_attributes.hinclude/linux/compiler-clang.h +0
-5@@ -21,8 +21,6 @@#define __SANITIZE_ADDRESS__#endif -#define __no_sanitize_address __attribute__((no_sanitize("address"))) -/* * Not all versions of clang implement the the type-generic versions * of the builtin overflow checkers. Fortunately, clang implements @@ -41,6 +39,3 @@* compilers, like ICC. */ #define barrier () __asm__ __volatile__("" : : : "memory") -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#define __assume_aligned (a, ...) \ - __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))include/linux/compiler-gcc.h +0
-60@@ -68,13 +68,6 @@*/ #define uninitialized_var (x) x = x -#ifdef __CHECKER__ -#define __must_be_array (a) 0 -#else -/* &a[0] degrades to a pointer: a different type from an array */ -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#endif -#ifdef RETPOLINE #define __noretpoline __attribute__((indirect_branch("keep")))#endif @@ -95,8 +88,6 @@#define __UNIQUE_ID (prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) -#define __optimize (level) __attribute__((__optimize__(level))) -#define __compiletime_object_size (obj) __builtin_object_size(obj, 0)#ifndef __CHECKER__@@ -133,9 +124,6 @@__builtin_unreachable (); \ } while (0) -/* Mark a function definition as prohibited from being cloned. */ -#define __noclone __attribute__((__noclone__, __optimize__("no-tracer"))) -#if defined(RANDSTRUCT_PLUGIN ) && !defined(__CHECKER__)#define __randomize_layout __attribute__((randomize_layout))#define __no_randomize_layout __attribute__((no_randomize_layout))@@ -144,32 +132,6 @@#define randomized_struct_fields_end } __randomize_layout;#endif -/* -* When used with Link Time Optimization, gcc can optimize away C functions or -* variables which are referenced only from assembly code. __visible tells the -* optimizer that something else uses this function or variable, thus preventing -* this. -*/ -#define __visible __attribute__((externally_visible)) - -/* gcc version specific checks */ - -#if GCC_VERSION >=40900 && !defined(__CHECKER__) -/* -* __assume_aligned(n, k): Tell the optimizer that the returned -* pointer can be assumed to be k modulo n. The second argument is -* optional (default 0), so we use a variadic macro to make the -* shorthand. -* -* Beware: Do not apply this to functions which may return -* ERR_PTRs. Also, it is probably unwise to apply it to functions -* returning extra information in the low bits (but in that case the -* compiler should see some alignment anyway, when the return value is -* massaged by 'flags = ptr & 3; ptr &= ~3;'). -*/ -#define __assume_aligned (a, ...) __attribute__((__assume_aligned__(a, ## __VA_ARGS__))) -#endif -/* * GCC 'asm goto' miscompiles certain code sequences: * @@ -201,32 +163,10 @@#define KASAN_ABI_VERSION 3#endif -#if GCC_VERSION >=40902 -/* -* Tell the compiler that address safety instrumentation (KASAN) -* should not be applied to that function. -* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 -*/ -#define __no_sanitize_address __attribute__((no_sanitize_address)) -#endif -#if GCC_VERSION >=50100 -/* -* Mark structures as requiring designated initializers. -* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html -*/ -#define __designated_init __attribute__((designated_init))#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1#endif -#if !defined(__noclone) -#define __noclone/* not needed */ -#endif - -#if !defined(__no_sanitize_address) -#define __no_sanitize_address -#endif -/* * Turn individual warnings and errors on and off locally, depending * on version. include/linux/compiler-intel.h +0
-6@@ -37,9 +37,3 @@/* icc has this, but it's called _bswap16 */ #define __HAVE_BUILTIN_BSWAP16__#define __builtin_bswap16 _bswap16 - -/* The following are for compatibility with GCC, from compiler-gcc.h, -* and may be redefined here because they should not be shared with other -* compilers, like clang. -*/ -#define __visible __attribute__((externally_visible))include/linux/compiler.h +0
-4@@ -301,10 +301,6 @@ static inline void *offset_to_ptr(const int *off)#endif /* __ASSEMBLY__ */ -#ifndef __optimize -# define __optimize (level) -#endif -/* Compile time object size, -1 for unknown */ #ifndef __compiletime_object_size# define __compiletime_object_size (obj) -1include/linux/compiler_attributes.h new file +37
-0@@ -0,0 +1,105 @@ +#ifndef __LINUX_COMPILER_ATTRIBUTES_H +#define __LINUX_COMPILER_ATTRIBUTES_H + +/* This file is meant to be sorted. */ + +/* +* Required attributes: your compiler must support these. +*/ +#define __alias (symbol) __attribute__((alias(#symbol))) +#define __aligned (x) __attribute__((aligned(x))) +#define __aligned_largest __attribute__((aligned)) +#define __always_inline inline __attribute__((always_inline)) +#define __always_unused __attribute__((unused)) +#define __attribute_const__ __attribute__((const)) +#define __cold __attribute__((cold)) +#define __gnu_inline __attribute__((gnu_inline)) +#define __malloc __attribute__((malloc)) +#define __maybe_unused __attribute__((unused)) +#define __mode (x) __attribute__((mode(x))) +#define noinline __attribute__((noinline)) +#define __noreturn __attribute__((noreturn)) +#define __packed __attribute__((packed)) +#define __printf (a, b) __attribute__((format(printf, a, b))) +#define __pure __attribute__((pure)) +#define __scanf (a, b) __attribute__((format(scanf, a, b))) +#define __section (S ) __attribute__((section(#S))) +#define __used __attribute__((used)) +#define __weak __attribute__((weak)) + +/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5, +* we implement it by hand. +*/ +#ifndef __has_attribute
Quoted 8 lines
include/linux/compiler_attributes.h line 45
Quoted 8 lines
include/linux/compiler_attributes.h new file +8
-0+#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1 +#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0 +#endif
And a follow up; I'm trying to understand what will happen in the case of say gcc 4.9 here. Were any of these supported between gcc 4.6 and 5.0? If so, then this code will not use them. It's simpler than explicit version checks, but it won't use features that are supported.
Quoted 233 lines
Quoted 195 lines
include/linux/compiler_attributes.h new file +60
-0+ +/* +* __assume_aligned(n, k): Tell the optimizer that the returned +* pointer can be assumed to be k modulo n. The second argument is +* optional (default 0), so we use a variadic macro to make the +* shorthand. +* +* Beware: Do not apply this to functions which may return +* ERR_PTRs. Also, it is probably unwise to apply it to functions +* returning extra information in the low bits (but in that case the +* compiler should see some alignment anyway, when the return value is +* massaged by 'flags = ptr & 3; ptr &= ~3;'). +*/ +#if __has_attribute (assume_aligned) +#define __assume_aligned (a, ...) __attribute__((assume_aligned(a, ## __VA_ARGS__))) +#else +#define __assume_aligned (a, ...) +#endif + +/* +* Mark structures as requiring designated initializers. +* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html +*/ +#if __has_attribute (designated_init) +#define __designated_init __attribute__((designated_init)) +#else +#define __designated_init +#endif + +/* +* When used with Link Time Optimization, gcc can optimize away C functions or +* variables which are referenced only from assembly code. __visible tells the +* optimizer that something else uses this function or variable, thus preventing +* this. +*/ +#if __has_attribute (externally_visible) +#define __visible __attribute__((externally_visible)) +#else +#define __visible +#endif + +/* Mark a function definition as prohibited from being cloned. */ +#if __has_attribute (noclone) &&__has_attribute (optimize) +#define __noclone __attribute__((noclone, optimize("no-tracer"))) +#else +#define __noclone +#endif + +/* +* Tell the compiler that address safety instrumentation (KASAN) +* should not be applied to that function. +* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 +*/ +#if __has_attribute (no_sanitize_address) +#define __no_sanitize_address __attribute__((no_sanitize_address)) +#else +#define __no_sanitize_address +#endif + +#endif /* __LINUX_COMPILER_ATTRIBUTES_H */ include/linux/compiler_types.h +5
-72@@ -54,6 +54,9 @@ extern void __chk_io_ptr(const volatile void __iomem *);#ifdef __KERNEL__ +/* Attributes */ +#include <linux/compiler_attributes.h> +/* Compiler specific macros. */ #ifdef __clang__#include <linux/compiler-clang.h> @@ -78,12 +81,6 @@ extern void __chk_io_ptr(const volatile void __iomem *);#include <asm/compiler.h> #endif -/* -* Generic compiler-independent macros required for kernel -* build go below this comment. Actual compiler/compiler version -* specific implementations come from the above header files -*/ -struct ftrace_branch_data {const char *func;const char *file;@@ -119,10 +116,6 @@ struct ftrace_likely_data {* compilers. We don't consider that to be an error, so set them to nothing. * For example, some of them are for compiler specific plugins. */ -#ifndef __designated_init -# define __designated_init -#endif -#ifndef __latent_entropy# define __latent_entropy#endif @@ -140,17 +133,6 @@ struct ftrace_likely_data {# define randomized_struct_fields_end#endif -#ifndef __visible -#define __visible -#endif - -/* -* Assume alignment of return value. -*/ -#ifndef __assume_aligned -#define __assume_aligned (a, ...) -#endif -/* Are two types/vars the same type (ignoring qualifiers)? */ #define __same_type (a, b) __builtin_types_compatible_p(typeof(a), typeof(b))@@ -159,14 +141,6 @@ struct ftrace_likely_data { (sizeof (t) ==sizeof (char ) ||sizeof (t) ==sizeof (short ) || \sizeof (t) ==sizeof (int ) ||sizeof (t) ==sizeof (long )) -#ifndef __attribute_const__ -#define __attribute_const__ __attribute__((__const__)) -#endif - -#ifndef __noclone -#define __noclone -#endif - /* Helpers for emitting diagnostics in pragmas. */#ifndef __diag#define __diag (string)@@ -186,34 +160,6 @@ struct ftrace_likely_data {#define __diag_error (compiler, version, option, comment) \ __diag_ ## compiler(version, error, option) -/* -* From the GCC manual: -* -* Many functions have no effects except the return value and their -* return value depends only on the parameters and/or global -* variables. Such a function can be subject to common subexpression -* elimination and loop optimization just as an arithmetic operator -* would be. -* [...] -*/ -#define __pure __attribute__((pure)) -#define __aligned (x) __attribute__((aligned(x))) -#define __aligned_largest __attribute__((aligned)) -#define __printf (a, b) __attribute__((format(printf, a, b))) -#define __scanf (a, b) __attribute__((format(scanf, a, b))) -#define __maybe_unused __attribute__((unused)) -#define __always_unused __attribute__((unused)) -#define __mode (x) __attribute__((mode(x))) -#define __malloc __attribute__((__malloc__)) -#define __used __attribute__((__used__)) -#define __noreturn __attribute__((noreturn)) -#define __packed __attribute__((packed)) -#define __weak __attribute__((weak)) -#define __alias (symbol) __attribute__((alias(#symbol))) -#define __cold __attribute__((cold)) -#define __section (S ) __attribute__((__section__(#S))) - -#ifdef CONFIG_ENABLE_MUST_CHECK #define __must_check __attribute__((warn_unused_result))#else @@ -228,18 +174,6 @@ struct ftrace_likely_data {#define __compiler_offsetof (a, b) __builtin_offsetof(a, b) -/* -* Feature detection for gnu_inline (gnu89 extern inline semantics). Either -* __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, -* and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not -* defined so the gnu89 semantics are the default. -*/ -#ifdef __GNUC_STDC_INLINE__ -# define __gnu_inline __attribute__((gnu_inline)) -#else -# define __gnu_inline -#endif -/* * Force always-inline if the user requests it so via the .config. * GCC does not warn about unused static inline functions for @@ -254,19 +188,13 @@ struct ftrace_likely_data {*/ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING ) || \ !defined(CONFIG_OPTIMIZE_INLINING ) -#define inline \ - inline __attribute__((always_inline, unused)) notrace __gnu_inline +#define inline inline __attribute__((always_inline, unused)) notrace __gnu_inline#else -#define inline inline __attribute__((unused)) notrace __gnu_inline +#define inline inline __attribute__((unused)) notrace __gnu_inlineinclude/linux/compiler_types.h struct ftrace_likely_data { · line 197
Quoted 10 lines
include/linux/compiler_types.h +1
-6#endif #define __inline__ inline -#define __inline inline -#define noinline __attribute__((noinline)) - -#ifndef __always_inline -#define __always_inline inline __attribute__((always_inline)) -#endif +#define __inline inlineAll of the changes to inline should not be removed, see above. It's important to make this work correctly regardless of C standard used.
Quoted 16 lines
include/linux/compiler_types.h +7
-1/* * Rather then using noinline to prevent stack consumption, use @@ -274,4 +202,11 @@ struct ftrace_likely_data {*/ #define noinline_for_stack noinline +#ifdef __CHECKER__ +#define __must_be_array (a) 0 +#else +/* &a[0] degrades to a pointer: a different type from an array */ +#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +#endif +#endif /* __LINUX_COMPILER_TYPES_H */ --Quoted 2 lines
2.17.1
With the above changes requested, I'm super happy with the spirit of this patch, and look forward to a v2. Thanks again Miguel! --
Thanks,
~Nick Desaulniers
Signature · 2 lines
Thanks, ~Nick Desaulniers
Hi Nick,
Quoted 12 lines · Nick Desaulniers
Quoted 9 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").All the attributes that are fairly common/standard (i.e. those that do not require extra logic to define them) have been moved to a new file
include/linux/compiler_attributes.h. The attributes have been sorted and divided between "required" and "optional".
Quoted 5 lines
Nice! Thanks Miguel. Regarding sorting, I'm happy with that. In fact, some of the comments can be removed IMO, as the attributes have common definitions in the docs (maybe an added link to the gcc and clang attribute docs at the top of the file rather than per attribute comments).
Thanks for the review!
I thought about that, although there isn't a single page with them in GCC (we could group them by type though: function ones, variable ones... and then link to those). On the other hand, maybe writing a Doc/ file is better and allows us to write as much as one would like about each of them (and a link to each page compiler's page about it, etc.). I think in the end the Doc/ file might be the best, in order not to crowd the header.
Quoted 11 lines
Quoted 7 lines
Further, attributes that are already supported in gcc >= 4.6 and recent clang were simply made to be required (instead of testing for them):
*always_inline
* const (pure was already "required", by the way)
*gnu_inlineThere's an important test for
gnu_inlinethat isn't checking that it's supported, but rather what the implicit behavior is depending on which C standard is being used. It's important not to remove that.
Hm... I actually thought it was not available at some point before 4.6 and removed the #ifdef. The comment even says it is featuring detecting it so that the old GCC inlining is used; but it shouldn't matter if you always use it, no?
I just went looking for more info in d03db2bc2 ("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline declarations") and if I understood the commit message, the problem is compiling with implicit new standard in newer compilers which trigger the C90 behavior, while we need the old one --- but if we use gnu_inline, we are getting it regardless.
I am sure I am missing something, but I think a clarification is needed (and in the code comment as well) -- a bit off-topic, anyway.
[Also, I wouldn't define an attribute or not depending on some other condition. I would, instead, define something some other symbol with that logic (i.e. instead of using "__gnu_inline", because that is lying -- it is not using the attribute even if the compiler supports it).]
Quoted 9 lines
Quoted 4 lines
Finally, some other bits were cleaned up in the process:
*__optimize: removed (unused in the whole kernel tree)A+ for removing dead code. I also don't see it used anywhere.
Quoted 1 line
*
__must_be_array: removed from -gcc and -clang (identical), moved to_types
Quoted 7 lines
Yep, uses a builtin (we should add guards for that, later, in a similar style change that guards the use of builtins). Looks good.
Quoted 2 lines
(it depends on the unconditionally used
__builtin_types_compatible_p
* Removes unneeded underscores on the attributes' namesThat doesn't sound right, but lets see what you mean by that.
Some attributes used the __name__ syntax (i.e. inside the double parenthesis), others didn't. I simplified by removing all the extra underscores.
Quoted 7 lines
Quoted 4 lines
There are some things that can be further cleaned up afterwards:
*__attribute_const__: rename to__constThis doesn't look correct to me; the kernel is full of call sites for
__attribute_const__. You can't rename the definition without renaming
Of course it is full of use sites! That is why I said it is a possible cleanup for *afterwards* this patch :-)
Quoted 3 lines
all of the call sites (and that would be too big a change for this patch, IMO). Skip the rename, and it also looks like you just removed it outright (Oops).
Not sure what you mean by this (?). The attribute is still there unchanged.
Quoted 10 lines
Quoted 8 lines
*
__noretpoline: avoid checking fordefined(__notrepoline)
* __compiletime_warning/error: they are in two different places, -gcc andcompiler.h.
* sparse' attributes could potentially go into the end ofattributes.htoo (as another separate section).Compile-tested an x86 allmodconfig for a while with gcc 8.2.0 and 4.6.4.
It's important to test changes to
compiler-clang.hwith clang. ;)
I would agree if the clang build wasn't broken to begin with. ;)
Quoted 435 lines
Quoted 15 lines
Cc Eli Friedman <[email protected]>Cc Christopher Li <[email protected]>Cc Kees Cook <[email protected]>Cc Ingo Molnar <[email protected]>Cc Geert Uytterhoeven <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Joe Perches <[email protected]>Cc Dominique Martinet <[email protected]>Cc Nick Desaulniers <[email protected]>Cc Linus Torvalds <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>Quoted 7 lines
---
*Seems* to work, but note that I did not finish the entire allmodconfig :)A few things could be splitted into their own patch, but I kept it as one for simplicity for a first review.
These files are becoming no-headaches-readable again, yay.
A+
Quoted 410 lines
include/linux/compiler-clang.h | 5 -- include/linux/compiler-gcc.h | 60 ---------------- include/linux/compiler-intel.h | 6 -- include/linux/compiler.h | 4 -- include/linux/compiler_attributes.h | 105 ++++++++++++++++++++++++++++ include/linux/compiler_types.h | 91 ++++-------------------- 6 files changed, 118 insertions(+), 153 deletions(-) create mode 100644 include/linux/compiler_attributes.hinclude/linux/compiler-clang.h +0
-5@@ -21,8 +21,6 @@#define __SANITIZE_ADDRESS__#endif -#define __no_sanitize_address __attribute__((no_sanitize("address"))) -/* * Not all versions of clang implement the the type-generic versions * of the builtin overflow checkers. Fortunately, clang implements @@ -41,6 +39,3 @@* compilers, like ICC. */ #define barrier () __asm__ __volatile__("" : : : "memory") -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#define __assume_aligned (a, ...) \ - __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))include/linux/compiler-gcc.h +0
-60@@ -68,13 +68,6 @@*/ #define uninitialized_var (x) x = x -#ifdef __CHECKER__ -#define __must_be_array (a) 0 -#else -/* &a[0] degrades to a pointer: a different type from an array */ -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#endif -#ifdef RETPOLINE #define __noretpoline __attribute__((indirect_branch("keep")))#endif @@ -95,8 +88,6 @@#define __UNIQUE_ID (prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) -#define __optimize (level) __attribute__((__optimize__(level))) -#define __compiletime_object_size (obj) __builtin_object_size(obj, 0)#ifndef __CHECKER__@@ -133,9 +124,6 @@__builtin_unreachable (); \ } while (0) -/* Mark a function definition as prohibited from being cloned. */ -#define __noclone __attribute__((__noclone__, __optimize__("no-tracer"))) -#if defined(RANDSTRUCT_PLUGIN ) && !defined(__CHECKER__)#define __randomize_layout __attribute__((randomize_layout))#define __no_randomize_layout __attribute__((no_randomize_layout))@@ -144,32 +132,6 @@#define randomized_struct_fields_end } __randomize_layout;#endif -/* -* When used with Link Time Optimization, gcc can optimize away C functions or -* variables which are referenced only from assembly code. __visible tells the -* optimizer that something else uses this function or variable, thus preventing -* this. -*/ -#define __visible __attribute__((externally_visible)) - -/* gcc version specific checks */ - -#if GCC_VERSION >=40900 && !defined(__CHECKER__) -/* -* __assume_aligned(n, k): Tell the optimizer that the returned -* pointer can be assumed to be k modulo n. The second argument is -* optional (default 0), so we use a variadic macro to make the -* shorthand. -* -* Beware: Do not apply this to functions which may return -* ERR_PTRs. Also, it is probably unwise to apply it to functions -* returning extra information in the low bits (but in that case the -* compiler should see some alignment anyway, when the return value is -* massaged by 'flags = ptr & 3; ptr &= ~3;'). -*/ -#define __assume_aligned (a, ...) __attribute__((__assume_aligned__(a, ## __VA_ARGS__))) -#endif -/* * GCC 'asm goto' miscompiles certain code sequences: * @@ -201,32 +163,10 @@#define KASAN_ABI_VERSION 3#endif -#if GCC_VERSION >=40902 -/* -* Tell the compiler that address safety instrumentation (KASAN) -* should not be applied to that function. -* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 -*/ -#define __no_sanitize_address __attribute__((no_sanitize_address)) -#endif -#if GCC_VERSION >=50100 -/* -* Mark structures as requiring designated initializers. -* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html -*/ -#define __designated_init __attribute__((designated_init))#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1#endif -#if !defined(__noclone) -#define __noclone/* not needed */ -#endif - -#if !defined(__no_sanitize_address) -#define __no_sanitize_address -#endif -/* * Turn individual warnings and errors on and off locally, depending * on version. include/linux/compiler-intel.h +0
-6@@ -37,9 +37,3 @@/* icc has this, but it's called _bswap16 */ #define __HAVE_BUILTIN_BSWAP16__#define __builtin_bswap16 _bswap16 - -/* The following are for compatibility with GCC, from compiler-gcc.h, -* and may be redefined here because they should not be shared with other -* compilers, like clang. -*/ -#define __visible __attribute__((externally_visible))include/linux/compiler.h +0
-4@@ -301,10 +301,6 @@ static inline void *offset_to_ptr(const int *off)#endif /* __ASSEMBLY__ */ -#ifndef __optimize -# define __optimize (level) -#endif -/* Compile time object size, -1 for unknown */ #ifndef __compiletime_object_size# define __compiletime_object_size (obj) -1include/linux/compiler_attributes.h new file +105
-0@@ -0,0 +1,105 @@ +#ifndef __LINUX_COMPILER_ATTRIBUTES_H +#define __LINUX_COMPILER_ATTRIBUTES_H + +/* This file is meant to be sorted. */ + +/* +* Required attributes: your compiler must support these. +*/ +#define __alias (symbol) __attribute__((alias(#symbol))) +#define __aligned (x) __attribute__((aligned(x))) +#define __aligned_largest __attribute__((aligned)) +#define __always_inline inline __attribute__((always_inline)) +#define __always_unused __attribute__((unused)) +#define __attribute_const__ __attribute__((const)) +#define __cold __attribute__((cold)) +#define __gnu_inline __attribute__((gnu_inline)) +#define __malloc __attribute__((malloc)) +#define __maybe_unused __attribute__((unused)) +#define __mode (x) __attribute__((mode(x))) +#define noinline __attribute__((noinline)) +#define __noreturn __attribute__((noreturn)) +#define __packed __attribute__((packed)) +#define __printf (a, b) __attribute__((format(printf, a, b))) +#define __pure __attribute__((pure)) +#define __scanf (a, b) __attribute__((format(scanf, a, b))) +#define __section (S ) __attribute__((section(#S))) +#define __used __attribute__((used)) +#define __weak __attribute__((weak)) + +/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5, +* we implement it by hand. +*/ +#ifndef __has_attribute +#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1 +#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0 +#endif + +/* +* __assume_aligned(n, k): Tell the optimizer that the returned +* pointer can be assumed to be k modulo n. The second argument is +* optional (default 0), so we use a variadic macro to make the +* shorthand. +* +* Beware: Do not apply this to functions which may return +* ERR_PTRs. Also, it is probably unwise to apply it to functions +* returning extra information in the low bits (but in that case the +* compiler should see some alignment anyway, when the return value is +* massaged by 'flags = ptr & 3; ptr &= ~3;'). +*/ +#if __has_attribute (assume_aligned) +#define __assume_aligned (a, ...) __attribute__((assume_aligned(a, ## __VA_ARGS__))) +#else +#define __assume_aligned (a, ...) +#endif + +/* +* Mark structures as requiring designated initializers. +* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html +*/ +#if __has_attribute (designated_init) +#define __designated_init __attribute__((designated_init)) +#else +#define __designated_init +#endif + +/* +* When used with Link Time Optimization, gcc can optimize away C functions or +* variables which are referenced only from assembly code. __visible tells the +* optimizer that something else uses this function or variable, thus preventing +* this. +*/ +#if __has_attribute (externally_visible) +#define __visible __attribute__((externally_visible)) +#else +#define __visible +#endif + +/* Mark a function definition as prohibited from being cloned. */ +#if __has_attribute (noclone) &&__has_attribute (optimize) +#define __noclone __attribute__((noclone, optimize("no-tracer"))) +#else +#define __noclone +#endif + +/* +* Tell the compiler that address safety instrumentation (KASAN) +* should not be applied to that function. +* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 +*/ +#if __has_attribute (no_sanitize_address) +#define __no_sanitize_address __attribute__((no_sanitize_address)) +#else +#define __no_sanitize_address +#endif + +#endif /* __LINUX_COMPILER_ATTRIBUTES_H */ include/linux/compiler_types.h +5
-72@@ -54,6 +54,9 @@ extern void __chk_io_ptr(const volatile void __iomem *);#ifdef __KERNEL__ +/* Attributes */ +#include <linux/compiler_attributes.h> +/* Compiler specific macros. */ #ifdef __clang__#include <linux/compiler-clang.h> @@ -78,12 +81,6 @@ extern void __chk_io_ptr(const volatile void __iomem *);#include <asm/compiler.h> #endif -/* -* Generic compiler-independent macros required for kernel -* build go below this comment. Actual compiler/compiler version -* specific implementations come from the above header files -*/ -struct ftrace_branch_data {const char *func;const char *file;@@ -119,10 +116,6 @@ struct ftrace_likely_data {* compilers. We don't consider that to be an error, so set them to nothing. * For example, some of them are for compiler specific plugins. */ -#ifndef __designated_init -# define __designated_init -#endif -#ifndef __latent_entropy# define __latent_entropy#endif @@ -140,17 +133,6 @@ struct ftrace_likely_data {# define randomized_struct_fields_end#endif -#ifndef __visible -#define __visible -#endif - -/* -* Assume alignment of return value. -*/ -#ifndef __assume_aligned -#define __assume_aligned (a, ...) -#endif -/* Are two types/vars the same type (ignoring qualifiers)? */ #define __same_type (a, b) __builtin_types_compatible_p(typeof(a), typeof(b))@@ -159,14 +141,6 @@ struct ftrace_likely_data { (sizeof (t) ==sizeof (char ) ||sizeof (t) ==sizeof (short ) || \sizeof (t) ==sizeof (int ) ||sizeof (t) ==sizeof (long )) -#ifndef __attribute_const__ -#define __attribute_const__ __attribute__((__const__)) -#endif - -#ifndef __noclone -#define __noclone -#endif - /* Helpers for emitting diagnostics in pragmas. */#ifndef __diag#define __diag (string)@@ -186,34 +160,6 @@ struct ftrace_likely_data {#define __diag_error (compiler, version, option, comment) \ __diag_ ## compiler(version, error, option) -/* -* From the GCC manual: -* -* Many functions have no effects except the return value and their -* return value depends only on the parameters and/or global -* variables. Such a function can be subject to common subexpression -* elimination and loop optimization just as an arithmetic operator -* would be. -* [...] -*/ -#define __pure __attribute__((pure)) -#define __aligned (x) __attribute__((aligned(x))) -#define __aligned_largest __attribute__((aligned)) -#define __printf (a, b) __attribute__((format(printf, a, b))) -#define __scanf (a, b) __attribute__((format(scanf, a, b))) -#define __maybe_unused __attribute__((unused)) -#define __always_unused __attribute__((unused)) -#define __mode (x) __attribute__((mode(x))) -#define __malloc __attribute__((__malloc__)) -#define __used __attribute__((__used__)) -#define __noreturn __attribute__((noreturn)) -#define __packed __attribute__((packed)) -#define __weak __attribute__((weak)) -#define __alias (symbol) __attribute__((alias(#symbol))) -#define __cold __attribute__((cold)) -#define __section (S ) __attribute__((__section__(#S))) - -#ifdef CONFIG_ENABLE_MUST_CHECK #define __must_check __attribute__((warn_unused_result))#else @@ -228,18 +174,6 @@ struct ftrace_likely_data {#define __compiler_offsetof (a, b) __builtin_offsetof(a, b) -/* -* Feature detection for gnu_inline (gnu89 extern inline semantics). Either -* __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, -* and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not -* defined so the gnu89 semantics are the default. -*/ -#ifdef __GNUC_STDC_INLINE__ -# define __gnu_inline __attribute__((gnu_inline)) -#else -# define __gnu_inline -#endif -/* * Force always-inline if the user requests it so via the .config. * GCC does not warn about unused static inline functions for @@ -254,19 +188,13 @@ struct ftrace_likely_data {*/ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING ) || \ !defined(CONFIG_OPTIMIZE_INLINING ) -#define inline \ - inline __attribute__((always_inline, unused)) notrace __gnu_inline +#define inline inline __attribute__((always_inline, unused)) notrace __gnu_inline#else -#define inline inline __attribute__((unused)) notrace __gnu_inline +#define inline inline __attribute__((unused)) notrace __gnu_inline#endif
Quoted 12 lines
include/linux/compiler_types.h struct ftrace_likely_data { · line 197
Quoted 8 lines
include/linux/compiler_types.h +1
-6#define __inline__ inline -#define __inline inline -#define noinline __attribute__((noinline)) - -#ifndef __always_inline -#define __always_inline inline __attribute__((always_inline)) -#endif +#define __inline inlineAll of the changes to inline should not be removed, see above. It's important to make this work correctly regardless of C standard used.
See above.
Quoted 7 lines
Quoted 7 lines
include/linux/compiler_types.h +0
-0/* * Rather then using noinline to prevent stack consumption, use @@ -274,4 +202,11 @@ struct ftrace_likely_data {*/ #define noinline_for_stack noinline
Quoted 14 lines
Quoted 11 lines
include/linux/compiler_types.h +7
-1+#ifdef __CHECKER__ +#define __must_be_array (a) 0 +#else +/* &a[0] degrades to a pointer: a different type from an array */ +#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +#endif +#endif /* __LINUX_COMPILER_TYPES_H */ --2.17.1
With the above changes requested, I'm super happy with the spirit of this patch, and look forward to a v2. Thanks again Miguel!
Thanks again for the very thorough review!
Cheers,
Miguel
Quoted 3 lines
--
Thanks,
~Nick Desaulniers
Hi Nick,
Quoted 15 lines · Nick Desaulniers
On Mon, Aug 27, 2018 at 10:43 AM Nick Desaulniers
Quoted 14 lines
Quoted 14 lines
include/linux/compiler_attributes.h new file +14
-0+ +/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5, +* we implement it by hand. +*/ +#ifndef __has_attribute +#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1
Quoted 9 lines
Quoted 3 lines
include/linux/compiler_attributes.h line 45
Quoted 3 lines
include/linux/compiler_attributes.h new file +3
-0+#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0 +#endif And a follow up; I'm trying to understand what will happen in the case of say gcc 4.9 here. Were any of these supported between gcc 4.6 and 5.0? If so, then this code will not use them. It's simpler than explicit version checks, but it won't use features that are supported.
I addressed that in the email I sent afterwards:
"""
Note that:
- assume_aligned came with gcc 4.9
- no_sanitize_address came with gcc 4.8
So if we feel it is important to have them there (before gcc 5), we would need here a quick version check here.
"""
The idea is that, in the future, whenever gcc 5 or later is the minimum version, we just get rid of the #ifdef block without touching the rest of the code :-)
Cheers,
Miguel
Quoted 239 lines
Quoted 225 lines
Quoted 195 lines
include/linux/compiler_attributes.h new file +60
-0+ +/* +* __assume_aligned(n, k): Tell the optimizer that the returned +* pointer can be assumed to be k modulo n. The second argument is +* optional (default 0), so we use a variadic macro to make the +* shorthand. +* +* Beware: Do not apply this to functions which may return +* ERR_PTRs. Also, it is probably unwise to apply it to functions +* returning extra information in the low bits (but in that case the +* compiler should see some alignment anyway, when the return value is +* massaged by 'flags = ptr & 3; ptr &= ~3;'). +*/ +#if __has_attribute (assume_aligned) +#define __assume_aligned (a, ...) __attribute__((assume_aligned(a, ## __VA_ARGS__))) +#else +#define __assume_aligned (a, ...) +#endif + +/* +* Mark structures as requiring designated initializers. +* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html +*/ +#if __has_attribute (designated_init) +#define __designated_init __attribute__((designated_init)) +#else +#define __designated_init +#endif + +/* +* When used with Link Time Optimization, gcc can optimize away C functions or +* variables which are referenced only from assembly code. __visible tells the +* optimizer that something else uses this function or variable, thus preventing +* this. +*/ +#if __has_attribute (externally_visible) +#define __visible __attribute__((externally_visible)) +#else +#define __visible +#endif + +/* Mark a function definition as prohibited from being cloned. */ +#if __has_attribute (noclone) &&__has_attribute (optimize) +#define __noclone __attribute__((noclone, optimize("no-tracer"))) +#else +#define __noclone +#endif + +/* +* Tell the compiler that address safety instrumentation (KASAN) +* should not be applied to that function. +* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 +*/ +#if __has_attribute (no_sanitize_address) +#define __no_sanitize_address __attribute__((no_sanitize_address)) +#else +#define __no_sanitize_address +#endif + +#endif /* __LINUX_COMPILER_ATTRIBUTES_H */ include/linux/compiler_types.h +5
-72@@ -54,6 +54,9 @@ extern void __chk_io_ptr(const volatile void __iomem *);#ifdef __KERNEL__ +/* Attributes */ +#include <linux/compiler_attributes.h> +/* Compiler specific macros. */ #ifdef __clang__#include <linux/compiler-clang.h> @@ -78,12 +81,6 @@ extern void __chk_io_ptr(const volatile void __iomem *);#include <asm/compiler.h> #endif -/* -* Generic compiler-independent macros required for kernel -* build go below this comment. Actual compiler/compiler version -* specific implementations come from the above header files -*/ -struct ftrace_branch_data {const char *func;const char *file;@@ -119,10 +116,6 @@ struct ftrace_likely_data {* compilers. We don't consider that to be an error, so set them to nothing. * For example, some of them are for compiler specific plugins. */ -#ifndef __designated_init -# define __designated_init -#endif -#ifndef __latent_entropy# define __latent_entropy#endif @@ -140,17 +133,6 @@ struct ftrace_likely_data {# define randomized_struct_fields_end#endif -#ifndef __visible -#define __visible -#endif - -/* -* Assume alignment of return value. -*/ -#ifndef __assume_aligned -#define __assume_aligned (a, ...) -#endif -/* Are two types/vars the same type (ignoring qualifiers)? */ #define __same_type (a, b) __builtin_types_compatible_p(typeof(a), typeof(b))@@ -159,14 +141,6 @@ struct ftrace_likely_data { (sizeof (t) ==sizeof (char ) ||sizeof (t) ==sizeof (short ) || \sizeof (t) ==sizeof (int ) ||sizeof (t) ==sizeof (long )) -#ifndef __attribute_const__ -#define __attribute_const__ __attribute__((__const__)) -#endif - -#ifndef __noclone -#define __noclone -#endif - /* Helpers for emitting diagnostics in pragmas. */#ifndef __diag#define __diag (string)@@ -186,34 +160,6 @@ struct ftrace_likely_data {#define __diag_error (compiler, version, option, comment) \ __diag_ ## compiler(version, error, option) -/* -* From the GCC manual: -* -* Many functions have no effects except the return value and their -* return value depends only on the parameters and/or global -* variables. Such a function can be subject to common subexpression -* elimination and loop optimization just as an arithmetic operator -* would be. -* [...] -*/ -#define __pure __attribute__((pure)) -#define __aligned (x) __attribute__((aligned(x))) -#define __aligned_largest __attribute__((aligned)) -#define __printf (a, b) __attribute__((format(printf, a, b))) -#define __scanf (a, b) __attribute__((format(scanf, a, b))) -#define __maybe_unused __attribute__((unused)) -#define __always_unused __attribute__((unused)) -#define __mode (x) __attribute__((mode(x))) -#define __malloc __attribute__((__malloc__)) -#define __used __attribute__((__used__)) -#define __noreturn __attribute__((noreturn)) -#define __packed __attribute__((packed)) -#define __weak __attribute__((weak)) -#define __alias (symbol) __attribute__((alias(#symbol))) -#define __cold __attribute__((cold)) -#define __section (S ) __attribute__((__section__(#S))) - -#ifdef CONFIG_ENABLE_MUST_CHECK #define __must_check __attribute__((warn_unused_result))#else @@ -228,18 +174,6 @@ struct ftrace_likely_data {#define __compiler_offsetof (a, b) __builtin_offsetof(a, b) -/* -* Feature detection for gnu_inline (gnu89 extern inline semantics). Either -* __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, -* and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not -* defined so the gnu89 semantics are the default. -*/ -#ifdef __GNUC_STDC_INLINE__ -# define __gnu_inline __attribute__((gnu_inline)) -#else -# define __gnu_inline -#endif -/* * Force always-inline if the user requests it so via the .config. * GCC does not warn about unused static inline functions for @@ -254,19 +188,13 @@ struct ftrace_likely_data {*/ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING ) || \ !defined(CONFIG_OPTIMIZE_INLINING ) -#define inline \ - inline __attribute__((always_inline, unused)) notrace __gnu_inline +#define inline inline __attribute__((always_inline, unused)) notrace __gnu_inline#else -#define inline inline __attribute__((unused)) notrace __gnu_inline +#define inline inline __attribute__((unused)) notrace __gnu_inlineinclude/linux/compiler_types.h struct ftrace_likely_data { · line 197
Quoted 10 lines
include/linux/compiler_types.h +1
-6#endif #define __inline__ inline -#define __inline inline -#define noinline __attribute__((noinline)) - -#ifndef __always_inline -#define __always_inline inline __attribute__((always_inline)) -#endif +#define __inline inlineAll of the changes to inline should not be removed, see above. It's important to make this work correctly regardless of C standard used.
Quoted 16 lines
include/linux/compiler_types.h +7
-1/* * Rather then using noinline to prevent stack consumption, use @@ -274,4 +202,11 @@ struct ftrace_likely_data {*/ #define noinline_for_stack noinline +#ifdef __CHECKER__ +#define __must_be_array (a) 0 +#else +/* &a[0] degrades to a pointer: a different type from an array */ +#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +#endif +#endif /* __LINUX_COMPILER_TYPES_H */ --Quoted 8 lines
Quoted 2 lines
2.17.1
With the above changes requested, I'm super happy with the spirit of this patch, and look forward to a v2. Thanks again Miguel! --
Thanks,
~Nick Desaulniers--
Thanks,
~Nick Desaulniers
Nick Desaulniers
reviewerQuoted 25 lines · Miguel Ojeda
Hi Nick,
Quoted 12 lines · Nick Desaulniers
Quoted 9 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").All the attributes that are fairly common/standard (i.e. those that do not require extra logic to define them) have been moved to a new file
include/linux/compiler_attributes.h. The attributes have been sorted and divided between "required" and "optional".Quoted 5 lines
Nice! Thanks Miguel. Regarding sorting, I'm happy with that. In fact, some of the comments can be removed IMO, as the attributes have common definitions in the docs (maybe an added link to the gcc and clang attribute docs at the top of the file rather than per attribute comments).
Thanks for the review!
Quoted 8 lines
I thought about that, although there isn't a single page with them in GCC (we could group them by type though: function ones, variable ones... and then link to those).
On the other hand, maybe writing a
Doc/ file is better and allows us to write as much as one would like about each of them (and a link to each page compiler's page about it, etc.). I think in the end the Doc/ file might be the best, in order not to crowd the header.
A comment is closer to the source, but I guess that's bytes for each inclusion for every file. I don't feel passionate about this point one way or the other.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Further, attributes that are already supported in gcc >= 4.6 and recent clang were simply made to be required (instead of testing for them):
*always_inline
* const (pure was already "required", by the way)
*gnu_inline
Quoted 8 lines
Quoted 3 lines
There's an important test for
gnu_inlinethat isn't checking that it's supported, but rather what the implicit behavior is depending on which C standard is being used. It's important not to remove that.Hm... I actually thought it was not available at some point before 4.6 and removed the #ifdef. The comment even says it is featuring detecting it so that the old GCC inlining is used; but it shouldn't matter if you always use it, no?
Good point. Rather than defining it only if GNU inline is not the current behavior is a bit more verbose than just always defining it. This seems to confirm that that should work:
https://godbolt.org/z/igwh32.
Quoted 29 lines
I just went looking for more info in
d03db2bc2("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline declarations") and if I understood the commit message, the problem is compiling with implicit new standard in newer compilers which trigger the C90 behavior, while we need the old one --- but if we usegnu_inline, we are getting it regardless.I am sure I am missing something, but I think a clarification is needed (and in the code comment as well) -- a bit off-topic, anyway.
[Also, I wouldn't define an attribute or not depending on some other condition. I would, instead, define something some other symbol with that logic (i.e. instead of using "
__gnu_inline", because that is lying -- it is not using the attribute even if the compiler supports it).]Quoted 12 lines
Quoted 4 lines
Finally, some other bits were cleaned up in the process:
*__optimize: removed (unused in the whole kernel tree)A+ for removing dead code. I also don't see it used anywhere.
Quoted 1 line
*
__must_be_array: removed from -gcc and -clang (identical), moved to_typesYep, uses a builtin (we should add guards for that, later, in a similar style change that guards the use of builtins). Looks good.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
(it depends on the unconditionally used
__builtin_types_compatible_p
* Removes unneeded underscores on the attributes' namesThat doesn't sound right, but lets see what you mean by that.
Some attributes used the
__name__syntax (i.e. inside the double parenthesis), others didn't. I simplified by removing all the extra underscores.
A+
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
There are some things that can be further cleaned up afterwards:
*__attribute_const__: rename to__constThis doesn't look correct to me; the kernel is full of call sites for
__attribute_const__. You can't rename the definition without renaming
Quoted 8 lines
Of course it is full of use sites! That is why I said it is a possible cleanup for *afterwards* this patch :-)
Quoted 3 lines
all of the call sites (and that would be too big a change for this patch, IMO). Skip the rename, and it also looks like you just removed it outright (Oops).
Not sure what you mean by this (?). The attribute is still there unchanged.
Nevermind, I misinterpretered this part of the patch.
Quoted 486 lines
Quoted 11 lines
Quoted 9 lines
*
__noretpoline: avoid checking fordefined(__notrepoline)
* __compiletime_warning/error: they are in two different places, -gcc andcompiler.h.
* sparse' attributes could potentially go into the end ofattributes.htoo (as another separate section).Compile-tested an x86 allmodconfig for a while with gcc 8.2.0 and 4.6.4.
It's important to test changes to
compiler-clang.hwith clang. ;)I would agree if the clang build wasn't broken to begin with. ;)
Quoted 435 lines
Quoted 15 lines
Cc Eli Friedman <[email protected]>Cc Christopher Li <[email protected]>Cc Kees Cook <[email protected]>Cc Ingo Molnar <[email protected]>Cc Geert Uytterhoeven <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Greg Kroah-Hartman <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Joe Perches <[email protected]>Cc Dominique Martinet <[email protected]>Cc Nick Desaulniers <[email protected]>Cc Linus Torvalds <[email protected]>Signed-off-by Miguel Ojeda <[email protected]>Quoted 7 lines
---
*Seems* to work, but note that I did not finish the entire allmodconfig :)A few things could be splitted into their own patch, but I kept it as one for simplicity for a first review.
These files are becoming no-headaches-readable again, yay.
A+
Quoted 410 lines
include/linux/compiler-clang.h | 5 -- include/linux/compiler-gcc.h | 60 ---------------- include/linux/compiler-intel.h | 6 -- include/linux/compiler.h | 4 -- include/linux/compiler_attributes.h | 105 ++++++++++++++++++++++++++++ include/linux/compiler_types.h | 91 ++++-------------------- 6 files changed, 118 insertions(+), 153 deletions(-) create mode 100644 include/linux/compiler_attributes.hinclude/linux/compiler-clang.h +0
-5@@ -21,8 +21,6 @@#define __SANITIZE_ADDRESS__#endif -#define __no_sanitize_address __attribute__((no_sanitize("address"))) -/* * Not all versions of clang implement the the type-generic versions * of the builtin overflow checkers. Fortunately, clang implements @@ -41,6 +39,3 @@* compilers, like ICC. */ #define barrier () __asm__ __volatile__("" : : : "memory") -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#define __assume_aligned (a, ...) \ - __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))include/linux/compiler-gcc.h +0
-60@@ -68,13 +68,6 @@*/ #define uninitialized_var (x) x = x -#ifdef __CHECKER__ -#define __must_be_array (a) 0 -#else -/* &a[0] degrades to a pointer: a different type from an array */ -#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) -#endif -#ifdef RETPOLINE #define __noretpoline __attribute__((indirect_branch("keep")))#endif @@ -95,8 +88,6 @@#define __UNIQUE_ID (prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) -#define __optimize (level) __attribute__((__optimize__(level))) -#define __compiletime_object_size (obj) __builtin_object_size(obj, 0)#ifndef __CHECKER__@@ -133,9 +124,6 @@__builtin_unreachable (); \ } while (0) -/* Mark a function definition as prohibited from being cloned. */ -#define __noclone __attribute__((__noclone__, __optimize__("no-tracer"))) -#if defined(RANDSTRUCT_PLUGIN ) && !defined(__CHECKER__)#define __randomize_layout __attribute__((randomize_layout))#define __no_randomize_layout __attribute__((no_randomize_layout))@@ -144,32 +132,6 @@#define randomized_struct_fields_end } __randomize_layout;#endif -/* -* When used with Link Time Optimization, gcc can optimize away C functions or -* variables which are referenced only from assembly code. __visible tells the -* optimizer that something else uses this function or variable, thus preventing -* this. -*/ -#define __visible __attribute__((externally_visible)) - -/* gcc version specific checks */ - -#if GCC_VERSION >=40900 && !defined(__CHECKER__) -/* -* __assume_aligned(n, k): Tell the optimizer that the returned -* pointer can be assumed to be k modulo n. The second argument is -* optional (default 0), so we use a variadic macro to make the -* shorthand. -* -* Beware: Do not apply this to functions which may return -* ERR_PTRs. Also, it is probably unwise to apply it to functions -* returning extra information in the low bits (but in that case the -* compiler should see some alignment anyway, when the return value is -* massaged by 'flags = ptr & 3; ptr &= ~3;'). -*/ -#define __assume_aligned (a, ...) __attribute__((__assume_aligned__(a, ## __VA_ARGS__))) -#endif -/* * GCC 'asm goto' miscompiles certain code sequences: * @@ -201,32 +163,10 @@#define KASAN_ABI_VERSION 3#endif -#if GCC_VERSION >=40902 -/* -* Tell the compiler that address safety instrumentation (KASAN) -* should not be applied to that function. -* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 -*/ -#define __no_sanitize_address __attribute__((no_sanitize_address)) -#endif -#if GCC_VERSION >=50100 -/* -* Mark structures as requiring designated initializers. -* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html -*/ -#define __designated_init __attribute__((designated_init))#define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1#endif -#if !defined(__noclone) -#define __noclone/* not needed */ -#endif - -#if !defined(__no_sanitize_address) -#define __no_sanitize_address -#endif -/* * Turn individual warnings and errors on and off locally, depending * on version. include/linux/compiler-intel.h +0
-6@@ -37,9 +37,3 @@/* icc has this, but it's called _bswap16 */ #define __HAVE_BUILTIN_BSWAP16__#define __builtin_bswap16 _bswap16 - -/* The following are for compatibility with GCC, from compiler-gcc.h, -* and may be redefined here because they should not be shared with other -* compilers, like clang. -*/ -#define __visible __attribute__((externally_visible))include/linux/compiler.h +0
-4@@ -301,10 +301,6 @@ static inline void *offset_to_ptr(const int *off)#endif /* __ASSEMBLY__ */ -#ifndef __optimize -# define __optimize (level) -#endif -/* Compile time object size, -1 for unknown */ #ifndef __compiletime_object_size# define __compiletime_object_size (obj) -1include/linux/compiler_attributes.h new file +105
-0@@ -0,0 +1,105 @@ +#ifndef __LINUX_COMPILER_ATTRIBUTES_H +#define __LINUX_COMPILER_ATTRIBUTES_H + +/* This file is meant to be sorted. */ + +/* +* Required attributes: your compiler must support these. +*/ +#define __alias (symbol) __attribute__((alias(#symbol))) +#define __aligned (x) __attribute__((aligned(x))) +#define __aligned_largest __attribute__((aligned)) +#define __always_inline inline __attribute__((always_inline)) +#define __always_unused __attribute__((unused)) +#define __attribute_const__ __attribute__((const)) +#define __cold __attribute__((cold)) +#define __gnu_inline __attribute__((gnu_inline)) +#define __malloc __attribute__((malloc)) +#define __maybe_unused __attribute__((unused)) +#define __mode (x) __attribute__((mode(x))) +#define noinline __attribute__((noinline)) +#define __noreturn __attribute__((noreturn)) +#define __packed __attribute__((packed)) +#define __printf (a, b) __attribute__((format(printf, a, b))) +#define __pure __attribute__((pure)) +#define __scanf (a, b) __attribute__((format(scanf, a, b))) +#define __section (S ) __attribute__((section(#S))) +#define __used __attribute__((used)) +#define __weak __attribute__((weak)) + +/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5, +* we implement it by hand. +*/ +#ifndef __has_attribute +#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1 +#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0 +#endif + +/* +* __assume_aligned(n, k): Tell the optimizer that the returned +* pointer can be assumed to be k modulo n. The second argument is +* optional (default 0), so we use a variadic macro to make the +* shorthand. +* +* Beware: Do not apply this to functions which may return +* ERR_PTRs. Also, it is probably unwise to apply it to functions +* returning extra information in the low bits (but in that case the +* compiler should see some alignment anyway, when the return value is +* massaged by 'flags = ptr & 3; ptr &= ~3;'). +*/ +#if __has_attribute (assume_aligned) +#define __assume_aligned (a, ...) __attribute__((assume_aligned(a, ## __VA_ARGS__))) +#else +#define __assume_aligned (a, ...) +#endif + +/* +* Mark structures as requiring designated initializers. +* https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html +*/ +#if __has_attribute (designated_init) +#define __designated_init __attribute__((designated_init)) +#else +#define __designated_init +#endif + +/* +* When used with Link Time Optimization, gcc can optimize away C functions or +* variables which are referenced only from assembly code. __visible tells the +* optimizer that something else uses this function or variable, thus preventing +* this. +*/ +#if __has_attribute (externally_visible) +#define __visible __attribute__((externally_visible)) +#else +#define __visible +#endif + +/* Mark a function definition as prohibited from being cloned. */ +#if __has_attribute (noclone) &&__has_attribute (optimize) +#define __noclone __attribute__((noclone, optimize("no-tracer"))) +#else +#define __noclone +#endif + +/* +* Tell the compiler that address safety instrumentation (KASAN) +* should not be applied to that function. +* Conflicts with inlining: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67368 +*/ +#if __has_attribute (no_sanitize_address) +#define __no_sanitize_address __attribute__((no_sanitize_address)) +#else +#define __no_sanitize_address +#endif + +#endif /* __LINUX_COMPILER_ATTRIBUTES_H */ include/linux/compiler_types.h +5
-72@@ -54,6 +54,9 @@ extern void __chk_io_ptr(const volatile void __iomem *);#ifdef __KERNEL__ +/* Attributes */ +#include <linux/compiler_attributes.h> +/* Compiler specific macros. */ #ifdef __clang__#include <linux/compiler-clang.h> @@ -78,12 +81,6 @@ extern void __chk_io_ptr(const volatile void __iomem *);#include <asm/compiler.h> #endif -/* -* Generic compiler-independent macros required for kernel -* build go below this comment. Actual compiler/compiler version -* specific implementations come from the above header files -*/ -struct ftrace_branch_data {const char *func;const char *file;@@ -119,10 +116,6 @@ struct ftrace_likely_data {* compilers. We don't consider that to be an error, so set them to nothing. * For example, some of them are for compiler specific plugins. */ -#ifndef __designated_init -# define __designated_init -#endif -#ifndef __latent_entropy# define __latent_entropy#endif @@ -140,17 +133,6 @@ struct ftrace_likely_data {# define randomized_struct_fields_end#endif -#ifndef __visible -#define __visible -#endif - -/* -* Assume alignment of return value. -*/ -#ifndef __assume_aligned -#define __assume_aligned (a, ...) -#endif -/* Are two types/vars the same type (ignoring qualifiers)? */ #define __same_type (a, b) __builtin_types_compatible_p(typeof(a), typeof(b))@@ -159,14 +141,6 @@ struct ftrace_likely_data { (sizeof (t) ==sizeof (char ) ||sizeof (t) ==sizeof (short ) || \sizeof (t) ==sizeof (int ) ||sizeof (t) ==sizeof (long )) -#ifndef __attribute_const__ -#define __attribute_const__ __attribute__((__const__)) -#endif - -#ifndef __noclone -#define __noclone -#endif - /* Helpers for emitting diagnostics in pragmas. */#ifndef __diag#define __diag (string)@@ -186,34 +160,6 @@ struct ftrace_likely_data {#define __diag_error (compiler, version, option, comment) \ __diag_ ## compiler(version, error, option) -/* -* From the GCC manual: -* -* Many functions have no effects except the return value and their -* return value depends only on the parameters and/or global -* variables. Such a function can be subject to common subexpression -* elimination and loop optimization just as an arithmetic operator -* would be. -* [...] -*/ -#define __pure __attribute__((pure)) -#define __aligned (x) __attribute__((aligned(x))) -#define __aligned_largest __attribute__((aligned)) -#define __printf (a, b) __attribute__((format(printf, a, b))) -#define __scanf (a, b) __attribute__((format(scanf, a, b))) -#define __maybe_unused __attribute__((unused)) -#define __always_unused __attribute__((unused)) -#define __mode (x) __attribute__((mode(x))) -#define __malloc __attribute__((__malloc__)) -#define __used __attribute__((__used__)) -#define __noreturn __attribute__((noreturn)) -#define __packed __attribute__((packed)) -#define __weak __attribute__((weak)) -#define __alias (symbol) __attribute__((alias(#symbol))) -#define __cold __attribute__((cold)) -#define __section (S ) __attribute__((__section__(#S))) - -#ifdef CONFIG_ENABLE_MUST_CHECK #define __must_check __attribute__((warn_unused_result))#else @@ -228,18 +174,6 @@ struct ftrace_likely_data {#define __compiler_offsetof (a, b) __builtin_offsetof(a, b) -/* -* Feature detection for gnu_inline (gnu89 extern inline semantics). Either -* __GNUC_STDC_INLINE__ is defined (not using gnu89 extern inline semantics, -* and we opt in to the gnu89 semantics), or __GNUC_STDC_INLINE__ is not -* defined so the gnu89 semantics are the default. -*/ -#ifdef __GNUC_STDC_INLINE__ -# define __gnu_inline __attribute__((gnu_inline)) -#else -# define __gnu_inline -#endif -/* * Force always-inline if the user requests it so via the .config. * GCC does not warn about unused static inline functions for @@ -254,19 +188,13 @@ struct ftrace_likely_data {*/ #if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING ) || \ !defined(CONFIG_OPTIMIZE_INLINING ) -#define inline \ - inline __attribute__((always_inline, unused)) notrace __gnu_inline +#define inline inline __attribute__((always_inline, unused)) notrace __gnu_inline#else -#define inline inline __attribute__((unused)) notrace __gnu_inline +#define inline inline __attribute__((unused)) notrace __gnu_inline#endif Quoted 12 lines
include/linux/compiler_types.h struct ftrace_likely_data { · line 197
Quoted 8 lines
include/linux/compiler_types.h +1
-6#define __inline__ inline -#define __inline inline -#define noinline __attribute__((noinline)) - -#ifndef __always_inline -#define __always_inline inline __attribute__((always_inline)) -#endif +#define __inline inlineAll of the changes to inline should not be removed, see above. It's important to make this work correctly regardless of C standard used.
See above.
Quoted 7 lines
Quoted 7 lines
include/linux/compiler_types.h +0
-0/* * Rather then using noinline to prevent stack consumption, use @@ -274,4 +202,11 @@ struct ftrace_likely_data {*/ #define noinline_for_stack noinlineQuoted 14 lines
Quoted 11 lines
include/linux/compiler_types.h +7
-1+#ifdef __CHECKER__ +#define __must_be_array (a) 0 +#else +/* &a[0] degrades to a pointer: a different type from an array */ +#define __must_be_array (a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0])) +#endif +#endif /* __LINUX_COMPILER_TYPES_H */ --2.17.1
With the above changes requested, I'm super happy with the spirit of this patch, and look forward to a v2. Thanks again Miguel!
Quoted 1 line
Thanks again for the very thorough review!
Thanks for the patch! I'm almost ready to sign off, just a few more comments on the other thread.
Signature · 2 lines
Thanks, ~Nick Desaulniers
Nick Desaulniers
reviewerQuoted 37 lines · Miguel Ojeda
Hi Nick,
Quoted 15 lines · Nick Desaulniers
On Mon, Aug 27, 2018 at 10:43 AM Nick Desaulniers
Quoted 14 lines
Quoted 14 lines
include/linux/compiler_attributes.h new file +14
-0+ +/* +* Optional attributes: your compiler may or may not support them. +* +* To check for them, we use __has_attribute, which is supported on gcc >= 5, +* clang >= 2.9 and icc >= 17. In the meantime, to support 4.6 <= gcc < 5, +* we implement it by hand. +*/ +#ifndef __has_attribute +#define __has_attribute (x) __GCC46_has_attribute_##x +#define __GCC46_has_attribute_assume_aligned 0 +#define __GCC46_has_attribute_designated_init 0 +#define __GCC46_has_attribute_externally_visible 1 +#define __GCC46_has_attribute_noclone 1Quoted 9 lines
Quoted 3 lines
include/linux/compiler_attributes.h line 45
Quoted 3 lines
include/linux/compiler_attributes.h new file +3
-0+#define __GCC46_has_attribute_optimize 1 +#define __GCC46_has_attribute_no_sanitize_address 0 +#endif And a follow up; I'm trying to understand what will happen in the case of say gcc 4.9 here. Were any of these supported between gcc 4.6 and 5.0? If so, then this code will not use them. It's simpler than explicit version checks, but it won't use features that are supported.
I addressed that in the email I sent afterwards:
"""
Note that:
-assume_alignedcame with gcc 4.9
-no_sanitize_addresscame with gcc 4.8
Quoted 7 lines
So if we feel it is important to have them there (before gcc 5), we would need here a quick version check here.
"""The idea is that, in the future, whenever gcc 5 or later is the minimum version, we just get rid of the #ifdef block without touching the rest of the code :-)
So if __has_attribute came with gcc 5, then that means that this patch will break assume_aligned for gcc-4.9 users and no_sanitize_address for gcc-4.8 and gcc-4.9 users? The slab allocator uses assume_aligned, and no_sanitize_address for CONFIG_KASAN. Should this patch ever come back through stable, Android and ChromeOS gcc-4.9/KASAN builds will break.
I don't think we should leave that for a follow up; I would like to see it as part of this patch. It's ok to have explicit version checks for those 2 attributes since it's not possible to feature detect them for the versions of gcc that we support in this code base. I think you should add them in a v2 of this patch. Then we can point to this commit as the *shining example* of how to do proper feature detection, falling back to version checks only as a last resort.
Signature · 2 lines
Thanks, ~Nick Desaulniers
Hi Nick,
Quoted 18 lines · Nick Desaulniers
Quoted 8 lines · Miguel Ojeda
I addressed that in the email I sent afterwards:
"""
Note that:
-assume_alignedcame with gcc 4.9
-no_sanitize_addresscame with gcc 4.8Quoted 7 lines
So if we feel it is important to have them there (before gcc 5), we would need here a quick version check here.
"""The idea is that, in the future, whenever gcc 5 or later is the minimum version, we just get rid of the #ifdef block without touching the rest of the code :-)
Quoted 7 lines
So if
__has_attributecame with gcc 5, then that means that this patch will breakassume_alignedfor gcc-4.9 users andno_sanitize_addressfor gcc-4.8 and gcc-4.9 users? The slab allocator usesassume_aligned, andno_sanitize_addressforCONFIG_KASAN. Should this patch ever come back through stable, Android and ChromeOS gcc-4.9/KASAN builds will break.
Indeed, KASAN requires it:
This is strictly a debugging feature and it requires a gcc version of 4.9.2 or later. Detection of out of bounds accesses to stack or global variables requires gcc 5.0 or later.
So we should just support it. However, __no_sanitize_address is only used when CONFIG_KASAN is enabled (to define __no_kasan_or_inline). So I would say it is an attribute for a particular CONFIG (like those of sparse). Therefore, I think we should simply remove __no_sanitize_address for general use (let's see how it looks).
For __assume_aligned, it is "only" an optimization, but I think it is a general one, so we should keep it in attributes.h; I will simply add the gcc 4.9 support knowledge.
On that topic: actually, some of the attributes that we have that are "required" are not really "required" in the strict sense: we could test for them; but I wanted to minimize the amount of noise for gcc < 5 since we have to manually write the support table (and anyway most compilers support them). Whenever we are past gcc 5 in a few years, we could actually test for the non-strictly-required attribute if we want to be extra nice to new compilers :-)
Quoted 7 lines
I don't think we should leave that for a follow up; I would like to see it as part of this patch. It's ok to have explicit version checks for those 2 attributes since it's not possible to feature detect them for the versions of gcc that we support in this code base. I think you should add them in a v2 of this patch. Then we can point to this commit as the *shining example* of how to do proper feature detection, falling back to version checks only as a last resort.
Thanks for the kind words!
I also read your other comments in the previous email -- no comments on those. I will prepare v2.
Cheers,
Miguel
Hi Nick,
Actually, to acknowledge the comments to the other email...
Quoted 36 lines · Nick Desaulniers
Quoted 25 lines · Miguel Ojeda
Hi Nick,
Quoted 12 lines · Nick Desaulniers
Quoted 9 lines · Miguel Ojeda
Instead of using version checks per-compiler to define (or not) each attribute, use
__has_attributeto test for them, following the cleanup started with commit815f0ddb346c("include/linux/compiler*.h: make compiler-*.h mutually exclusive").All the attributes that are fairly common/standard (i.e. those that do not require extra logic to define them) have been moved to a new file
include/linux/compiler_attributes.h. The attributes have been sorted and divided between "required" and "optional".Quoted 5 lines
Nice! Thanks Miguel. Regarding sorting, I'm happy with that. In fact, some of the comments can be removed IMO, as the attributes have common definitions in the docs (maybe an added link to the gcc and clang attribute docs at the top of the file rather than per attribute comments).
Thanks for the review!
Quoted 8 lines
I thought about that, although there isn't a single page with them in GCC (we could group them by type though: function ones, variable ones... and then link to those).
On the other hand, maybe writing a
Doc/ file is better and allows us to write as much as one would like about each of them (and a link to each page compiler's page about it, etc.). I think in the end the Doc/ file might be the best, in order not to crowd the header.
Quoted 4 lines
A comment is closer to the source, but I guess that's bytes for each inclusion for every file. I don't feel passionate about this point one way or the other.
I think I will write a simple Doc/ file, link to it from the source, and see if people like it.
Quoted 18 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Further, attributes that are already supported in gcc >= 4.6 and recent clang were simply made to be required (instead of testing for them):
*always_inline
* const (pure was already "required", by the way)
*gnu_inlineQuoted 8 lines
Quoted 3 lines
There's an important test for
gnu_inlinethat isn't checking that it's supported, but rather what the implicit behavior is depending on which C standard is being used. It's important not to remove that.Hm... I actually thought it was not available at some point before 4.6 and removed the #ifdef. The comment even says it is featuring detecting it so that the old GCC inlining is used; but it shouldn't matter if you always use it, no?
Quoted 5 lines
Good point. Rather than defining it only if GNU inline is not the current behavior is a bit more verbose than just always defining it. This seems to confirm that that should work:
https://godbolt.org/z/igwh32.
Great then! Thanks for confirming!
Cheers,
Miguel