Messages in this thread | | | From | Nick Desaulniers <> | Date | Tue, 28 Aug 2018 10:05:30 -0700 | Subject | Re: [PATCH] include/linux/compiler*.h: Use feature checking instead of version checks for attributes |
| |
On Tue, Aug 28, 2018 at 8:10 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > Hi Nick, > > On Mon, Aug 27, 2018 at 7:48 PM, Nick Desaulniers > <ndesaulniers@google.com> 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
| |