Messages in this thread | | | From | Miguel Ojeda <> | Date | Tue, 28 Aug 2018 22:33:10 +0200 | Subject | Re: [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes |
| |
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
| |