Message-ID: <CANiq72mfuJ2=v8U9vUs1vJVEjY2sp7f-WbTRZRHNvgh+VbAgnA@mail.gmail.com>
Date:   Sat, 1 Sep 2018 15:38:13 +0200
From:   Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.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>
Subject: Re: [PATCH 7/7] Compiler Attributes: use feature checks instead of
 version checks

Hi Nick,

On Sat, Sep 1, 2018 at 1:07 AM, Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
> Overall, pretty happy with this patch.  Still some thoughts for a v3,
>> -#define __visible      __attribute__((externally_visible))
>> diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
>> new file mode 100644
>> index 000000000000..a9dfafc8fd19
>> --- /dev/null
>> +++ b/include/linux/compiler_attributes.h
>> @@ -0,0 +1,226 @@
>
> New file needs an SPDX license identifier right?

Yeah, but I wasn't sure of adding it, since the code I moved (even if
rearranged) from _types.h does not have it either. Any legal expert
here? Is _types.h it implicitly GPL 2? We should add the identifier to
both if so.

>
>> +#ifndef __LINUX_COMPILER_ATTRIBUTES_H
>> +#define __LINUX_COMPILER_ATTRIBUTES_H
>> +
>> +/*
>> + * This file is meant to be sorted (by actual attribute name,
>> + * not by #define identifier).
>> + *
>> + * Do not add here attributes which depend on others or require extra logic.
>> + *
>> + * If an attribute is optional, state the reason in the comment.
>
> A lot of newlines in this comment. Can be condensed?
>

I tried to write it as "bullet points", but as you guys prefer!

>> + */
>> +
>> +/*
>> + * To check for optional attributes, 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 __has_attribute by hand.
>> + */
>> +#ifndef __has_attribute
>> +#define __has_attribute(x) __GCC4_has_attribute_##x
>> +#define __GCC4_has_attribute_externally_visible 1
>> +#define __GCC4_has_attribute_noclone 1
>> +#define __GCC4_has_attribute_optimize 1
>> +#if __GNUC_MINOR__ >= 8
>> +#define __GCC4_has_attribute_no_sanitize_address 1
>> +#endif
>> +#if __GNUC_MINOR__ >= 9
>> +#define __GCC4_has_attribute_assume_aligned 1
>> +#endif
>> +#endif
>
> I'm quite happy with this. Kudos.

Thanks! :-)

>
>> +
>> +/*
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-alias-function-attribute
>> + */
>> +#define __alias(symbol)         __attribute__((alias(#symbol)))
>> +
>> +/*
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-aligned-function-attribute
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-aligned-type-attribute
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-aligned-variable-attribute
>> + */
>> +#define __aligned(x)            __attribute__((aligned(x)))
>> +#define __aligned_largest       __attribute__((aligned))
>> +
>> +/*
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-always_005finline-function-attribute
>> + * clang: mentioned but no proper documentation
>
> What?! Ok, I'll add to my TODO list, but I think we can leave out that
> clang doesn't have docs (yet!).
>

If you are going to do that -- please also check noinline (same thing
happens: you can find it mentioned in an example, but it is not
documented). Anyway, the clang docs state:

"""
Clang supports GCC’s gnu attribute namespace. All GCC attributes which
are accepted with the __attribute__((foo)) syntax are also accepted
as[[gnu::foo]]. This only extends to attributes which are specified by
GCC (see the list of GCC function attributes, GCC variable attributes,
and GCC type attributes). As with the GCC implementation, these
attributes must appertain to the declarator-id in a declaration, which
means they must go either at the start of the declaration or
immediately after the name being declared.
"""

in https://clang.llvm.org/docs/LanguageExtensions.html, so I guess it
is fair ;-) In any case, the GNU/C++/C2x/declspec/... compatibility
tables provided in https://clang.llvm.org/docs/AttributeReference.html
are great, and it would be *very* nice to complete that document to
have a reference for all developers accross all platforms, not just
kernel ones.

>> +
>> +/*
>> + * Note the long name.
>> + *
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-const-function-attribute
>> + */
>> +#define __attribute_const__     __attribute__((const))
>
> So I manually wrote down the list of attributes removed from one file,
> and added to another, to make sure they balance out and that none were
> missed.  I'm quite confident that nothing was missed moving from one
> file to another.  Except this.  You've renamed __const to
> __attribute_const__.  But you renamed __attribute_const_ to __const in
> patch 3/7 in this series.  Surely one of them is a mistake?

D'oh! It seems I confused myself when rebasing stuff to split the
patches. Good catch! 3/7 has the typo, which then triggers a second
rename. The end result of the series is the good state (i.e. no change
should happen to the identifiers).

>> +/*
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Type-Attributes.html#index-mode-type-attribute
>> + *   gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-mode-variable-attribute
>> + */
>> +#define __mode(x)               __attribute__((mode(x)))
>> +
>> +/*
>> + * Optional: not supported by clang
>> + * Note: icc does not recognize gcc's no-tracer
>
> In that case, can you provide 3 definitions for __noclone? Something like:
>
> if has noclone
>   if has optimize
>     noclone = noclone optimize
>   else
>     noclone = noclone
> else
>   noclone =
>

So what happened here is that icc recognizes the attribute itself (not
sure if it honors it or not), so there would be no compiler using the
"noclone && !optimize" definition, which is why I left it at that.

But I see the point in your idea in case there is a 4th compiler around...

>>  #ifdef CONFIG_ENABLE_MUST_CHECK
>>  #define __must_check           __attribute__((warn_unused_result))
>
> Can this attribute be hoisted to your new header?  I guess there's a
> few other __attributes__ still in this file even after this change.
> Maybe not something that needs to be in this patch set, but what are
> your thoughts on them?

This is something I gave quite some thought... I am still not sure
what is the right idea.

The decision I took (for the moment, at least) was to leave
compiler_attributes.h as "regular" and "simple" as possible, defining
whatever attributes that are common enough (i.e. those that can be
defined simply with __has_attribute at most) -- which is why I added
the "Do not add here..." comment in the top of the file, and leaving
the CONFIG- / PLUGIN-dependent definitions out of it.

Because of that logic, I tried to simplify as much as possible all
existing things (e.g. assume_aligned by removing __CHECKER__) so that
they could be moved to compiler_attributes.h. In a way,
compiler_attributes.h defines some extensions to the C language that
all code could use anywhere (actually whether we can move them out of
__KERNEL__ etc. is another question); and I tried as well to only move
those attributes that have a direct mapping to a "standard" attribute
(e.g. "noclone" was borderline :-).

The other alternative is moving everything to compiler_attributes.h
and keeping all logic there for all attributes and pseudo-attributes,
but I thought we could risk ending up with a complex mess there as in
_types.h; and end up with the pseudo-attributes which do not map to
real attributes and that are disabled in many configurations. Again,
the "C programming language extensions" idea could be a nice one (I
actually wrote a Documentation/ file to guide newcomers on this which
I didn't include in the series yet, see below [*]).

I would like to hear opinions on this!

>>  /*
>>   * Force always-inline if the user requests it so via the .config.
>>   * GCC does not warn about unused static inline functions for
>> @@ -240,17 +184,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 __always_inline __gnu_inline __maybe_unused notrace
>>  #else
>> -#define inline inline  __attribute__((unused)) notrace __gnu_inline
>> +#define inline          inline __gnu_inline __maybe_unused notrace
>
> This seems to have different spacing after the first `inline` than the
> line 2 lines above.
>

Take a look at it in fixed-sized font (the idea is that
__always_inline contains inline, so it replaces the inline below in
that case to avoid warnings about duplicated specifiers). Does that
explain it?

>>  #endif
>>
>>  #define __inline__ inline
>> -#define __inline inline
>> -#define noinline       __attribute__((noinline))
>> -
>> -#define __always_inline inline __attribute__((always_inline))
>> +#define __inline   inline
>>
>>  /*
>>   * Rather then using noinline to prevent stack consumption, use
>> --
>> 2.17.1
>>
>
> Thank you again for this patch.

You are welcome! I am glad these series is getting a warm welcome.

Cheers,
Miguel

[*]

Documentation/process/programming-language.rst:

.. _programming_language:

Programming Language
====================

The kernel is written in the C programming language [c-language]_.
More precisely, the kernel is typically compiled with ``gcc`` [gcc]_
under ``-std=gnu89`` [gcc-c-dialect-options]_: the GNU dialect of ISO C90
(including some C99 features).

This dialect contains many extensions to the language [gnu-extensions]_,
and many of them are used within the kernel as a matter of course.

There is some support for compiling the kernel with ``clang`` [clang]_
and ``icc`` [icc]_, although at the time of writing it is not completed,
requiring third-party patches.

Attributes
----------

One of the common extensions used throughout the kernel are attributes
[gcc-attribute-syntax]_. Attributes allow to introduce
implementation-defined semantics to language entities (like variables,
functions or types) without having to make significant syntactic changes
to the language (e.g. adding a new keyword) [n2049]_.

In many cases, attributes are optional (i.e. a compiler not supporting them
should still produce proper code, even if it is slower or does not perform
as many compile-time checks/diagnostics).

The kernel defines pseudo-keywords (e.g. ``__pure``) instead of using
directly the GNU attribute syntax (e.g. ``__attribute__((pure))``).

Please refer to ``include/linux/compiler_attributes.h`` to see a reference
of all the common supported attributes.

.. [c-language] http://www.open-std.org/jtc1/sc22/wg14/www/standards
.. [gcc] https://gcc.gnu.org
.. [clang] https://clang.llvm.org
.. [icc] https://software.intel.com/en-us/c-compilers
.. [gcc-c-dialect-options]
https://gcc.gnu.org/onlinedocs/gcc/C-Dialect-Options.html
.. [gnu-extensions]: https://gcc.gnu.org/onlinedocs/gcc/C-Extensions.html
.. [gcc-attribute-syntax]
https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
.. [n2049] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2049.pdf

Powered by blists - more mailing lists