Message-ID: <CANiq72nV_sVGgQuZra2gqOXHnbtCYWZDBMRW=EeeLK3qTL99ew@mail.gmail.com>
Date:   Tue, 28 Aug 2018 17:10:31 +0200
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Eli Friedman <efriedma@...eaurora.org>,
        Christopher Li <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 Perches <joe@...ches.com>,
        Dominique Martinet <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

Hi Nick,

On Mon, Aug 27, 2018 at 7:48 PM, Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
> On Mon, Aug 27, 2018 at 10:43 AM Nick Desaulniers
>> > +
>> > +/*
>> > + * 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.
>

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

>> > +
>> > +/*
>> > + * __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