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

[Nick Desaulniers]

On Tue, Aug 28, 2018 at 8:10 AM Miguel Ojeda
<miguel.ojeda.sandonis@xxxxxxxxx> wrote:
>
> Hi Nick,
>
> On Mon, Aug 27, 2018 at 7:48 PM, Nick Desaulniers
> <ndesaulniers@xxxxxxxxxx> wrote:
> > On Mon, Aug 27, 2018 at 10:43 AM Nick Desaulniers
> >> > +
> >> > +/*
> >> > + * Optional attributes: your compiler may or may not support them.
> >> > + *
> >> > + * To check for them, 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 it by hand.
> >> > + */
> >> > +#ifndef __has_attribute
> >> > +#define __has_attribute(x) __GCC46_has_attribute_##x
> >> > +#define __GCC46_has_attribute_assume_aligned 0
> >> > +#define __GCC46_has_attribute_designated_init 0
> >> > +#define __GCC46_has_attribute_externally_visible 1
> >> > +#define __GCC46_has_attribute_noclone 1
> >> > +#define __GCC46_has_attribute_optimize 1
> >> > +#define __GCC46_has_attribute_no_sanitize_address 0
> >> > +#endif
> >
> > And a follow up; I'm trying to understand what will happen in the case
> > of say gcc 4.9 here.  Were any of these supported between gcc 4.6 and
> > 5.0?  If so, then this code will not use them.  It's simpler than
> > explicit version checks, but it won't use features that are supported.
> >
>
> 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.

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,
~Nick Desaulniers