Re: [PATCH 7/7] Compiler Attributes: use feature checks instead of version checks

[Miguel Ojeda]

Hi Dominique,

On Sat, Sep 1, 2018 at 12:14 PM, Dominique Martinet
<asmadeus@xxxxxxxxxxxxx> wrote:
> Miguel Ojeda wrote on Fri, Aug 31, 2018:
>> 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.
>>
>> In an effort to make the file as regular as possible, comments
>> stating the purpose of attributes have been removed. Instead,
>> links to the compiler docs have been added (i.e. to gcc and,
>> if available, to clang as well). In addition, they have been sorted.
>>
>> Finally, if an attribute is optional (i.e. if it is guarded
>> by __has_attribute), the reason has been stated for future reference.
>>
>> Cc: Eli Friedman <efriedma@xxxxxxxxxxxxxx>
>> Cc: Christopher Li <sparse@xxxxxxxxxxx>
>> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
>> Cc: Arnd Bergmann <arnd@xxxxxxxx>
>> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> Cc: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> Cc: Joe Perches <joe@xxxxxxxxxxx>
>> Cc: Dominique Martinet <asmadeus@xxxxxxxxxxxxx>
>> Cc: Nick Desaulniers <ndesaulniers@xxxxxxxxxx>
>> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Miguel Ojeda <miguel.ojeda.sandonis@xxxxxxxxx>
>
> Nice work!
> Since I'm being Cc'd I took the time to test this as well, and have no
> problem with libbcc-building-with-clang (or native x86 gcc build)

Thanks a lot for testing! :-)

>
> Nick already made many comments so I only have one more.
>
>> [...]
>> 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
>> [...]
>> +/*
>> + * 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
>
> Hmm, if this is in this file and not compiler-gcc, I am not sure about
> using GNUC_MINOR without checking the major -- I have no idea what kind
> of versions e.g. icc will use (or what attributes ancients version of
> clang or old icc support, actually)

The idea here is that all non-gcc recent compilers that we may care
about support __has_attribute,so if we are inside the #ifndef, we
*have* to be gcc < 5 (see https://godbolt.org/z/NQFdsK).

>
>
> It's a bit of research work but I think it'd be cleaner to define
> similar macros for all three compilers, if we care about the old
> versions... Or actually..
>
> For clang you've implicitely required clang >= 3.0 in patch 3 of this
> serie, so presumabely it wouldn't need this compat macro at all.
>
> For icc I think icc 17 is still fairly recent... But I just abused work
> to test and linux fails to compile with icc 15/17/18 for other reasons
> (unrelated to this patch), so unless anyone helps with this I'm tempted

Thanks a lot for testing it! I agree that icc 17 is fairly recent, but
is there anybody using it on a regular basis to compile the kernel --
I guess your tests confirm that there isn't, but you never know... :-)
As for clang < 2.9, I doubt anybody is using it for anything
meaningful.

Also, we should note that this is about compiling the latest greatest
kernel release, which someone with older compilers is less likely to
be doing with clang and icc than gcc, I guess.

As for the failures: indeed, if anyone is actually doing it, they
probably need to apply quite some third-party patches anyway, so they
can also patch this file as well. Otherwise, they can always speak up
and ask for support. If nobody says anything though, then we have
proof nobody cares about it...

> to suggest leaving it at it, and whoever that is will probably have a
> better idea of how far back they want to make icc work / what attributes
> are defined there.
> It's a bit of a shame there's no linux-compilers list to reach out to :)
>
> (this would need to move the include of this file after the
> compiler-specific headers, but from what I can see there is no problem
> with that)

I included it as soon as possible so that other code (e.g. the
compiler-specific headers) can depend on the common attributes for
anything they need (in this way, the compiler_attributes.h can be
considered like it defines some extensions to the C language that we
allow all code in the kernel to use -- as if it was not there to begin
with).

By the way, do you know if there are some public docs on the
attributes supported by icc that we can link to? I couldn't find
anything in a quick search, both in google and in the icc's
"Compatibility" pages...

Cheers,
Miguel