On Thu, Nov 1, 2018 at 6:06 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> I'm not sure how much that matters (maybe the original check for 4.9.2
> was just a random pick by Andrey? Added to cc), but together with the
> movement to <linux/compiler_attributes.h> that looks like it also
> wouldn't want the CONFIG_KASAN tests, I wonder what the right merge
> resolution would be.
Good catch. I don't recall any special logic when I did that change,
so most likely I simply did like for the rest of the attributes and
took a look at when it was first supported (and documentation in gcc's
docs) in order to implement __has_attribute by hand.
But indeed, it *may* be that there is an (undocumented) problem
between 4.8 <= gcc < 4.9.2 with it. If so, we should document it down
and fix it. Andrey?
> Yes, I see the resolution in linux-next, and I think that one is odd
> and dubious, and now it *mixes* that old test of gcc-4.9.2 with the
> different test in compiler-attributes.
I missed that conflict completely, my bad (I did not miss all of them,
at least; one required fixing).
Hm.... at a quick look, why is it only on compiler-gcc.h? It should
either have a corresponding #define elsewhere or just be put directly
in another common header, no? (Adding Vasily & Martin to CC.)
> But I'm also unsure whether you meant for the "__has_attribute()" test
> to be usable outside the linux/compiler_attributes.h file, in which
> case I could just do
>
> #if defined(CONFIG_KASAN) && __has_attribute(__no_sanitize_address__)
>
> instead.
I think that (using __has_attribute() outside) may be a good idea: I
wanted to keep compiler_attributes.h as simple as possible by avoiding
#ifdefs inside that header (except for __has_attribute itself), as an
attempt to avoid going back to the mess of #ifdefs we had previously.
Basically, keeping the attributes in compiler_attributes.h that do not
depend on complex logic. So using __has_attribute *outside* the header
actually goes well with that principle, because it helps keeping stuff
out of it if they depend on other config options; without having to
rely on GCC_VERSION either.
[By the way, in case it clarifies: note that "optional" in that file
actually is a bit of a misnomer. I meant to say "optional" as in "not
supported by all compilers, so conditionally defined" ("optionally"
defined); rather than "optional" in the sense of "code still works
without the attribute". It caught Rasmus in one of his patches sent a
few days ago on top of this tree, so I want to change it or explain it
to avoid confusion.]
Cheers,
Miguel
