linux-kernel - Re: [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANiq72nJemek_uo16P2KhyfKNgrUQOmi+495SGGWW+QzLDfTsw@mail.gmail.com>
Date:   Tue, 28 Aug 2018 22:41:27 +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,

Actually, to acknowledge the comments to the other email...

On Tue, Aug 28, 2018 at 6:53 PM, Nick Desaulniers
<ndesaulniers@...gle.com> wrote:
> On Tue, Aug 28, 2018 at 8:04 AM Miguel Ojeda
> <miguel.ojeda.sandonis@...il.com> wrote:
>>
>> Hi Nick,
>>
>> On Mon, Aug 27, 2018 at 7:43 PM, 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).
>>
>> 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.
>
> 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.

>>
>> >
>> >>
>> >> 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.
>>
>> 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.
>

Great then! Thanks for confirming!

Cheers,
Miguel