Hi Nick,
On Tue, Aug 28, 2018 at 7:05 PM, Nick Desaulniers
<ndesaulni...@google.com> wrote:
> On Tue, Aug 28, 2018 at 8:10 AM Miguel Ojeda
> <miguel.ojeda.sando...@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
