From: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
To: Nick Desaulniers <ndesaulniers@google.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Eli Friedman <efriedma@codeaurora.org>,
Christopher Li <sparse@chrisli.org>,
Kees Cook <keescook@chromium.org>, Ingo Molnar <mingo@kernel.org>,
Geert Uytterhoeven <geert@linux-m68k.org>,
Arnd Bergmann <arnd@arndb.de>,
Greg KH <gregkh@linuxfoundation.org>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
Joe Perches <joe@perches.com>,
Dominique Martinet <asmadeus@codewreck.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes
Date: Tue, 28 Aug 2018 22:33:10 +0200 [thread overview]
Message-ID: <CANiq72=0SPHyjQjKXyv1wojFG7R54Y3k7eqbk_66cOqrc-R0bw@mail.gmail.com> (raw)
In-Reply-To: <CAKwvOdmSOGRDaCUgsEnEd=uyf83FzrRhJt-E3VnzX4FzDwPFWw@mail.gmail.com>
Hi Nick,
On Tue, Aug 28, 2018 at 7:05 PM, Nick Desaulniers
<ndesaulniers@google.com> wrote:
> On Tue, Aug 28, 2018 at 8:10 AM Miguel Ojeda
> <miguel.ojeda.sandonis@gmail.com> wrote:
>>
>> 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 :-)
>
> So if __has_attribute came with gcc 5, then that means that this patch
> will break assume_aligned for gcc-4.9 users and no_sanitize_address
> for gcc-4.8 and gcc-4.9 users? The slab allocator uses
> assume_aligned, and no_sanitize_address for CONFIG_KASAN. Should this
> patch ever come back through stable, Android and ChromeOS
> gcc-4.9/KASAN builds will break.
>
Indeed, KASAN requires it:
This is strictly a debugging feature and it requires a gcc version
of 4.9.2 or later. Detection of out of bounds accesses to stack or
global variables requires gcc 5.0 or later.
So we should just support it. However, __no_sanitize_address is only
used when CONFIG_KASAN is enabled (to define __no_kasan_or_inline). So
I would say it is an attribute for a particular CONFIG (like those of
sparse). Therefore, I think we should simply remove
__no_sanitize_address for general use (let's see how it looks).
For __assume_aligned, it is "only" an optimization, but I think it is
a general one, so we should keep it in attributes.h; I will simply add
the gcc 4.9 support knowledge.
On that topic: actually, some of the attributes that we have that are
"required" are not really "required" in the strict sense: we could
test for them; but I wanted to minimize the amount of noise for gcc <
5 since we have to manually write the support table (and anyway most
compilers support them). Whenever we are past gcc 5 in a few years, we
could actually test for the non-strictly-required attribute if we want
to be extra nice to new compilers :-)
> I don't think we should leave that for a follow up; I would like to
> see it as part of this patch. It's ok to have explicit version checks
> for those 2 attributes since it's not possible to feature detect them
> for the versions of gcc that we support in this code base. I think
> you should add them in a v2 of this patch. Then we can point to this
> commit as the *shining example* of how to do proper feature detection,
> falling back to version checks only as a last resort.
Thanks for the kind words!
I also read your other comments in the previous email -- no comments
on those. I will prepare v2.
Cheers,
Miguel
next prev parent reply other threads:[~2018-08-28 20:33 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-26 17:57 [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes Miguel Ojeda
2018-08-26 18:30 ` Miguel Ojeda
2018-08-26 18:50 ` Joe Perches
2018-08-27 12:33 ` Miguel Ojeda
2018-08-27 17:43 ` Nick Desaulniers
2018-08-27 17:48 ` Nick Desaulniers
2018-08-28 15:10 ` Miguel Ojeda
2018-08-28 17:05 ` Nick Desaulniers
2018-08-28 20:33 ` Miguel Ojeda [this message]
2018-08-28 15:03 ` Miguel Ojeda
2018-08-28 16:53 ` Nick Desaulniers
2018-08-28 20:41 ` Miguel Ojeda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANiq72=0SPHyjQjKXyv1wojFG7R54Y3k7eqbk_66cOqrc-R0bw@mail.gmail.com' \
--to=miguel.ojeda.sandonis@gmail.com \
--cc=arnd@arndb.de \
--cc=asmadeus@codewreck.org \
--cc=efriedma@codeaurora.org \
--cc=geert@linux-m68k.org \
--cc=gregkh@linuxfoundation.org \
--cc=joe@perches.com \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=ndesaulniers@google.com \
--cc=sparse@chrisli.org \
--cc=torvalds@linux-foundation.org \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).