Message-ID: <CAKwvOdk3PjNtjMcsXxRVvKLNcT+O_=5D1B7DXNWcwxwcMqdV2w@mail.gmail.com>
Date:   Mon, 27 Aug 2018 10:48:52 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     miguel.ojeda.sandonis@...il.com
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        efriedma@...eaurora.org, sparse@...isli.org,
        Kees Cook <keescook@...omium.org>,
        Ingo Molnar <mingo@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Arnd Bergmann <arnd@...db.de>,
        Greg KH <gregkh@...uxfoundation.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        joe@...ches.com, asmadeus@...ewreck.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] include/linux/compiler*.h: Use feature checking instead
 of version checks for attributes

On Mon, Aug 27, 2018 at 10:43 AM Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
>
> On Sun, Aug 26, 2018 at 10:58 AM Miguel Ojeda
> <miguel.ojeda.sandonis@...il.com> wrote:
> >
> > 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".
>
> 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).
>
> >
> > 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.
>
> >
> > 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.
>
> >   * __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.
>
> >     (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.
>
> >
> > 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).
>
> >   * __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.
>
> It's important to test changes to compiler-clang.h with clang. ;)
>
> >
> > Cc: Eli Friedman <efriedma@...eaurora.org>
> > Cc: Christopher Li <sparse@...isli.org>
> > Cc: Kees Cook <keescook@...omium.org>
> > Cc: Ingo Molnar <mingo@...nel.org>
> > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Cc: Arnd Bergmann <arnd@...db.de>
> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Cc: Masahiro Yamada <yamada.masahiro@...ionext.com>
> > Cc: Joe Perches <joe@...ches.com>
> > Cc: Dominique Martinet <asmadeus@...ewreck.org>
> > Cc: Nick Desaulniers <ndesaulniers@...gle.com>
> > Cc: Linus Torvalds <torvalds@...ux-foundation.org>
> > Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
> > ---
> > *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+
>
> >
> >  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
> >
> > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> > index b1ce500fe8b3..3e7dafb3ea80 100644
> > --- a/include/linux/compiler-clang.h
> > +++ b/include/linux/compiler-clang.h
> > @@ -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__)))
> > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> > index 763bbad1e258..dde3daae6287 100644
> > --- a/include/linux/compiler-gcc.h
> > +++ b/include/linux/compiler-gcc.h
> > @@ -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.
> > diff --git a/include/linux/compiler-intel.h b/include/linux/compiler-intel.h
> > index 4c7f9befa9f6..fb9e77fc65ec 100644
> > --- a/include/linux/compiler-intel.h
> > +++ b/include/linux/compiler-intel.h
> > @@ -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))
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 681d866efb1e..7c0157d50964 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -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
> > diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
> > new file mode 100644
> > index 000000000000..af8c8413d136
> > --- /dev/null
> > +++ b/include/linux/compiler_attributes.h
> > @@ -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

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.

> > +
> > +/*
> > + * __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 */
> > diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> > index 3525c179698c..8cd622bedec4 100644
> > --- a/include/linux/compiler_types.h
> > +++ b/include/linux/compiler_types.h
> > @@ -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
> >
> >  #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.
>
> >
> >  /*
> >   * 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 */
> > --
> > 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

Powered by blists - more mailing lists