Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Hi all, On Fri, Aug 24, 2018 at 1:31 AM, Joe Perches wrote: > On Thu, 2018-08-23 at 16:12 -0700, Nick Desaulniers wrote: >> On Thu, Aug 23, 2018 at 2:19 PM Joe Perches wrote: >> > >> > On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote: >> > > Not this case; it's how we get gnu89 semantics for `extern inline` is >> > > not compiler specific (therefor should not go in a compiler specific >> > > header). >> > >> > It's not possible to know that compilers support what >> > __attribute__(()) and at what version that support >> > exists unless it is specified somewhere. >> >> __has_attribute: >> https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute >> >> The release notes of GCC-5 mention __has_attribute. >> https://gcc.gnu.org/gcc-5/changes.html > > So not available in the now minimum supported gcc 4.6? > We can workaround that until we have 5 as the minimum. I have tried and the result is quite nice. Sending the patch in a few moments -- compile-testing for a while allmodconfig on both 4.6 and 8.2... Cheers, Miguel
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Hi all, On Fri, Aug 24, 2018 at 1:31 AM, Joe Perches wrote: > On Thu, 2018-08-23 at 16:12 -0700, Nick Desaulniers wrote: >> On Thu, Aug 23, 2018 at 2:19 PM Joe Perches wrote: >> > >> > On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote: >> > > Not this case; it's how we get gnu89 semantics for `extern inline` is >> > > not compiler specific (therefor should not go in a compiler specific >> > > header). >> > >> > It's not possible to know that compilers support what >> > __attribute__(()) and at what version that support >> > exists unless it is specified somewhere. >> >> __has_attribute: >> https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute >> >> The release notes of GCC-5 mention __has_attribute. >> https://gcc.gnu.org/gcc-5/changes.html > > So not available in the now minimum supported gcc 4.6? > We can workaround that until we have 5 as the minimum. I have tried and the result is quite nice. Sending the patch in a few moments -- compile-testing for a while allmodconfig on both 4.6 and 8.2... Cheers, Miguel
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, 2018-08-23 at 16:12 -0700, Nick Desaulniers wrote:
> On Thu, Aug 23, 2018 at 2:19 PM Joe Perches wrote:
> >
> > On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> > > One reply for a bunch of the various threads, to keep the number of
> > > emails down:
> > >
> > > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote:
> > > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > > > +/* Compiler specific macros. */
> > > > > #ifdef __clang__
> > > > > #include
> > > >
> > > > probably better as
> > > >
> > > > #if defined(__clang)
> > > >
> > > > to match the style of the #elif defined()s below it
> > >
> > > Hi Joe,
> > > Thanks for the feedback. I always appreciate it. If you have some
> > > cleanups, want to send them to me, and I'll bundle them up for a PR?
> > > I'm ok with that change.
> > >
> > > > > +#ifdef __GNUC_STDC_INLINE__
> > > > > +# define __gnu_inline__attribute__((gnu_inline))
> > > > > +#else
> > > > > +# define __gnu_inline
> > > > > +#endif
> > > >
> > > > Perhaps __gnu_inline should be in compiler-gcc and this
> > > > should use
> > > >
> > > > #ifndef __gnu_inline
> > > > #define __gnu_inline
> > > > #endif
> > >
> > > Not this case; it's how we get gnu89 semantics for `extern inline` is
> > > not compiler specific (therefor should not go in a compiler specific
> > > header).
> >
> > It's not possible to know that compilers support what
> > __attribute__(()) and at what version that support
> > exists unless it is specified somewhere.
>
> __has_attribute:
> https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
>
> The release notes of GCC-5 mention __has_attribute.
> https://gcc.gnu.org/gcc-5/changes.html
So not available in the now minimum supported gcc 4.6?
> The point of feature detection is that it _doesn't matter_ what
> version that support exists. Either it does and you can use it, or it
> doesn't and you can decide whether to stop compiling or there's a
> valid work around.
>
> Feature detection should be preferred to explicit version checks
> except in 2 specific cases:
> 1. It's not possible to properly perform feature detection. Language
> features should not be added unless it's possible to safely check for
> them.
> 2. A very specific version of a very specific compiler is broken and
> needs to be explicitly guarded against.
>
> > As far as I can tell, gnu_inline is not recognized by clang.
> >
> > https://clang.llvm.org/docs/AttributeReference.html
>
> If that was the case, I would not have added it in commit d03db2bc26f0
> ("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> declarations").
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d03db2bc26f0e4a6849ad649a09c9c73fccdc656
Hard to know.
That commit message does not mention clang.
> Docs can sometimes fall behind, the lone source of truth is the source code.
> https://github.com/llvm-mirror/clang/search?q=gnu_inline_q=gnu_inline
Which no compiler user should have to read.
> Godbolt is also incredibly helpful for testing various compiler versions:
> https://godbolt.org/z/uMJ-mo
Thanks for that.
> https://reviews.llvm.org/D51190
That too.
cheers, Joe
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, 2018-08-23 at 16:12 -0700, Nick Desaulniers wrote:
> On Thu, Aug 23, 2018 at 2:19 PM Joe Perches wrote:
> >
> > On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> > > One reply for a bunch of the various threads, to keep the number of
> > > emails down:
> > >
> > > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote:
> > > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > > > +/* Compiler specific macros. */
> > > > > #ifdef __clang__
> > > > > #include
> > > >
> > > > probably better as
> > > >
> > > > #if defined(__clang)
> > > >
> > > > to match the style of the #elif defined()s below it
> > >
> > > Hi Joe,
> > > Thanks for the feedback. I always appreciate it. If you have some
> > > cleanups, want to send them to me, and I'll bundle them up for a PR?
> > > I'm ok with that change.
> > >
> > > > > +#ifdef __GNUC_STDC_INLINE__
> > > > > +# define __gnu_inline__attribute__((gnu_inline))
> > > > > +#else
> > > > > +# define __gnu_inline
> > > > > +#endif
> > > >
> > > > Perhaps __gnu_inline should be in compiler-gcc and this
> > > > should use
> > > >
> > > > #ifndef __gnu_inline
> > > > #define __gnu_inline
> > > > #endif
> > >
> > > Not this case; it's how we get gnu89 semantics for `extern inline` is
> > > not compiler specific (therefor should not go in a compiler specific
> > > header).
> >
> > It's not possible to know that compilers support what
> > __attribute__(()) and at what version that support
> > exists unless it is specified somewhere.
>
> __has_attribute:
> https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
>
> The release notes of GCC-5 mention __has_attribute.
> https://gcc.gnu.org/gcc-5/changes.html
So not available in the now minimum supported gcc 4.6?
> The point of feature detection is that it _doesn't matter_ what
> version that support exists. Either it does and you can use it, or it
> doesn't and you can decide whether to stop compiling or there's a
> valid work around.
>
> Feature detection should be preferred to explicit version checks
> except in 2 specific cases:
> 1. It's not possible to properly perform feature detection. Language
> features should not be added unless it's possible to safely check for
> them.
> 2. A very specific version of a very specific compiler is broken and
> needs to be explicitly guarded against.
>
> > As far as I can tell, gnu_inline is not recognized by clang.
> >
> > https://clang.llvm.org/docs/AttributeReference.html
>
> If that was the case, I would not have added it in commit d03db2bc26f0
> ("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
> declarations").
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d03db2bc26f0e4a6849ad649a09c9c73fccdc656
Hard to know.
That commit message does not mention clang.
> Docs can sometimes fall behind, the lone source of truth is the source code.
> https://github.com/llvm-mirror/clang/search?q=gnu_inline_q=gnu_inline
Which no compiler user should have to read.
> Godbolt is also incredibly helpful for testing various compiler versions:
> https://godbolt.org/z/uMJ-mo
Thanks for that.
> https://reviews.llvm.org/D51190
That too.
cheers, Joe
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, Aug 23, 2018 at 2:19 PM Joe Perches wrote:
>
> On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> > One reply for a bunch of the various threads, to keep the number of emails
> > down:
> >
> > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote:
> > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > > +/* Compiler specific macros. */
> > > > #ifdef __clang__
> > > > #include
> > >
> > > probably better as
> > >
> > > #if defined(__clang)
> > >
> > > to match the style of the #elif defined()s below it
> >
> > Hi Joe,
> > Thanks for the feedback. I always appreciate it. If you have some
> > cleanups, want to send them to me, and I'll bundle them up for a PR?
> > I'm ok with that change.
> >
> > > > +#ifdef __GNUC_STDC_INLINE__
> > > > +# define __gnu_inline__attribute__((gnu_inline))
> > > > +#else
> > > > +# define __gnu_inline
> > > > +#endif
> > >
> > > Perhaps __gnu_inline should be in compiler-gcc and this
> > > should use
> > >
> > > #ifndef __gnu_inline
> > > #define __gnu_inline
> > > #endif
> >
> > Not this case; it's how we get gnu89 semantics for `extern inline` is
> > not compiler specific (therefor should not go in a compiler specific
> > header).
>
> It's not possible to know that compilers support what
> __attribute__(()) and at what version that support
> exists unless it is specified somewhere.
__has_attribute:
https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
The release notes of GCC-5 mention __has_attribute.
https://gcc.gnu.org/gcc-5/changes.html
The point of feature detection is that it _doesn't matter_ what
version that support exists. Either it does and you can use it, or it
doesn't and you can decide whether to stop compiling or there's a
valid work around.
Feature detection should be preferred to explicit version checks
except in 2 specific cases:
1. It's not possible to properly perform feature detection. Language
features should not be added unless it's possible to safely check for
them.
2. A very specific version of a very specific compiler is broken and
needs to be explicitly guarded against.
> As far as I can tell, gnu_inline is not recognized by clang.
>
> https://clang.llvm.org/docs/AttributeReference.html
If that was the case, I would not have added it in commit d03db2bc26f0
("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
declarations").
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d03db2bc26f0e4a6849ad649a09c9c73fccdc656
Docs can sometimes fall behind, the lone source of truth is the source code.
https://github.com/llvm-mirror/clang/search?q=gnu_inline_q=gnu_inline
Godbolt is also incredibly helpful for testing various compiler versions:
https://godbolt.org/z/uMJ-mo
https://reviews.llvm.org/D51190
--
Thanks,
~Nick Desaulniers
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, Aug 23, 2018 at 2:19 PM Joe Perches wrote:
>
> On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote:
> > One reply for a bunch of the various threads, to keep the number of emails
> > down:
> >
> > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote:
> > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > > > +/* Compiler specific macros. */
> > > > #ifdef __clang__
> > > > #include
> > >
> > > probably better as
> > >
> > > #if defined(__clang)
> > >
> > > to match the style of the #elif defined()s below it
> >
> > Hi Joe,
> > Thanks for the feedback. I always appreciate it. If you have some
> > cleanups, want to send them to me, and I'll bundle them up for a PR?
> > I'm ok with that change.
> >
> > > > +#ifdef __GNUC_STDC_INLINE__
> > > > +# define __gnu_inline__attribute__((gnu_inline))
> > > > +#else
> > > > +# define __gnu_inline
> > > > +#endif
> > >
> > > Perhaps __gnu_inline should be in compiler-gcc and this
> > > should use
> > >
> > > #ifndef __gnu_inline
> > > #define __gnu_inline
> > > #endif
> >
> > Not this case; it's how we get gnu89 semantics for `extern inline` is
> > not compiler specific (therefor should not go in a compiler specific
> > header).
>
> It's not possible to know that compilers support what
> __attribute__(()) and at what version that support
> exists unless it is specified somewhere.
__has_attribute:
https://clang.llvm.org/docs/LanguageExtensions.html#has-attribute
The release notes of GCC-5 mention __has_attribute.
https://gcc.gnu.org/gcc-5/changes.html
The point of feature detection is that it _doesn't matter_ what
version that support exists. Either it does and you can use it, or it
doesn't and you can decide whether to stop compiling or there's a
valid work around.
Feature detection should be preferred to explicit version checks
except in 2 specific cases:
1. It's not possible to properly perform feature detection. Language
features should not be added unless it's possible to safely check for
them.
2. A very specific version of a very specific compiler is broken and
needs to be explicitly guarded against.
> As far as I can tell, gnu_inline is not recognized by clang.
>
> https://clang.llvm.org/docs/AttributeReference.html
If that was the case, I would not have added it in commit d03db2bc26f0
("compiler-gcc.h: Add __attribute__((gnu_inline)) to all inline
declarations").
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d03db2bc26f0e4a6849ad649a09c9c73fccdc656
Docs can sometimes fall behind, the lone source of truth is the source code.
https://github.com/llvm-mirror/clang/search?q=gnu_inline_q=gnu_inline
Godbolt is also incredibly helpful for testing various compiler versions:
https://godbolt.org/z/uMJ-mo
https://reviews.llvm.org/D51190
--
Thanks,
~Nick Desaulniers
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Nick Desaulniers wrote on Thu, Aug 23, 2018: > > On a side note, I noticed tools/include/linux/compiler.h includes > > linux/compiler-gcc.h but maybe it should include linux/compiler_types.h? > > (I'm not sure at who uses that header, so it really is an open question > > here) > > Without looking into the code, this sounds like compiler.h is wrong. > Looking at the source, there's references to ancient Android NDK's > (what?! Let me show this to the NDK maintainers). This whole thing > needs to be cleaned up, too, IMO. Oh, there's two compiler-gcc.h in > the tree: > > - tools/include/linux/compiler-gcc.h (that's what's being included in > the case you point out) > - include/linux/compiler-gcc.h > > Maybe they can be combined? Ah, I didn't notice there is another compiler-gcc... Looking closer there seem to be other "reused" headers (almost all of the headers in tools/include/linux have a counterpart in include/linux), and some e.g. rbtree.h went from being a copy of the main include's to a single '#include "../../../../include/linux/rbtree.h"' line to a slightly simplified copy again to avoid bringing in rcu as dependency... I think compiler.h could be done with a similar trick with a bit of massaging, but as things stand linux/compiler.h includes uapi/linux/types.h, and this complains about using kernel headers from user space... So this isn't as trivial as just making tools use the kernel's include at least. > > If you tried to align these, __always_unused and __alias(symbol) look > > off > > I have `set tabstop=8` in vim, and it looks correct there, but both > `git diff` and github's code viewer show it as off. Maybe I have my > settings wrong in vim and need to go back to first grade, but > (personal opinion ahead) you don't have this kind of nonsense > (ambiguity around how many spaces a tab should be displayed as in > various code viewers) if you just always use spaces everywhere, for > everything. Other large codebases use automatic code formatters (like > clang-format) and that prevents holy wars and other distractions. > There's even a cool utility called `git-clang-format` that can check > just the code you changed, which is useful for not reformatting the > entire codebase messing up git blame. Right, this is my mistake - the diff view adds a space so these "inner tabs" alignments can be messed up. FWIW I think checkpatch only complains about leading space-based indentation, the inner filling can be whatever you want, but this is fine and I was overzealous. > On Wed, Aug 22, 2018 at 7:43 PM Masahiro Yamada > wrote: > > It was previous included by all compilers, > > but now it is only by _true_ GCC. > > Good catch, and thanks for the report and testing. I missed that > because I was testing x86_64 and arm64 (which I guess don't hit that > in the configs I tested) and not arm32. Should be a simple fix to > move it to shared the definition. Send me a patch, or I'll get to it. > > > Even if I move the __naked definition > > to , > > I see a different error. > > Did that regress due to my change? If so, sorry and I'll look into it > more soon. I've looked at that one quickly and that warning looks legitimate to me, I'm not sure why it would appear only now but clang does document[1] not allowing the parameters to be used even in extended asm, and gcc documentation[2] says "While using extended asm or a mixture of basic asm and C code may appear to work, they cannot be depended upon to work reliably and are not supported." [1] http://infocenter.arm.com/help/topic/com.arm.doc.dui0774g/jhg1476893564298.html [2] https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html In this case it looks like the arguments are only used for sanity with __asmeq but in the first place the original commit for trusted foundations talks about it not respecting the API so naked makes sense but they're not making the API function naked (and the one which takes an argument does use its argument) so this all feels a bit weird to me. It might be worth asking the original authors on this one... -- Dominique Martinet
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Nick Desaulniers wrote on Thu, Aug 23, 2018: > > On a side note, I noticed tools/include/linux/compiler.h includes > > linux/compiler-gcc.h but maybe it should include linux/compiler_types.h? > > (I'm not sure at who uses that header, so it really is an open question > > here) > > Without looking into the code, this sounds like compiler.h is wrong. > Looking at the source, there's references to ancient Android NDK's > (what?! Let me show this to the NDK maintainers). This whole thing > needs to be cleaned up, too, IMO. Oh, there's two compiler-gcc.h in > the tree: > > - tools/include/linux/compiler-gcc.h (that's what's being included in > the case you point out) > - include/linux/compiler-gcc.h > > Maybe they can be combined? Ah, I didn't notice there is another compiler-gcc... Looking closer there seem to be other "reused" headers (almost all of the headers in tools/include/linux have a counterpart in include/linux), and some e.g. rbtree.h went from being a copy of the main include's to a single '#include "../../../../include/linux/rbtree.h"' line to a slightly simplified copy again to avoid bringing in rcu as dependency... I think compiler.h could be done with a similar trick with a bit of massaging, but as things stand linux/compiler.h includes uapi/linux/types.h, and this complains about using kernel headers from user space... So this isn't as trivial as just making tools use the kernel's include at least. > > If you tried to align these, __always_unused and __alias(symbol) look > > off > > I have `set tabstop=8` in vim, and it looks correct there, but both > `git diff` and github's code viewer show it as off. Maybe I have my > settings wrong in vim and need to go back to first grade, but > (personal opinion ahead) you don't have this kind of nonsense > (ambiguity around how many spaces a tab should be displayed as in > various code viewers) if you just always use spaces everywhere, for > everything. Other large codebases use automatic code formatters (like > clang-format) and that prevents holy wars and other distractions. > There's even a cool utility called `git-clang-format` that can check > just the code you changed, which is useful for not reformatting the > entire codebase messing up git blame. Right, this is my mistake - the diff view adds a space so these "inner tabs" alignments can be messed up. FWIW I think checkpatch only complains about leading space-based indentation, the inner filling can be whatever you want, but this is fine and I was overzealous. > On Wed, Aug 22, 2018 at 7:43 PM Masahiro Yamada > wrote: > > It was previous included by all compilers, > > but now it is only by _true_ GCC. > > Good catch, and thanks for the report and testing. I missed that > because I was testing x86_64 and arm64 (which I guess don't hit that > in the configs I tested) and not arm32. Should be a simple fix to > move it to shared the definition. Send me a patch, or I'll get to it. > > > Even if I move the __naked definition > > to , > > I see a different error. > > Did that regress due to my change? If so, sorry and I'll look into it > more soon. I've looked at that one quickly and that warning looks legitimate to me, I'm not sure why it would appear only now but clang does document[1] not allowing the parameters to be used even in extended asm, and gcc documentation[2] says "While using extended asm or a mixture of basic asm and C code may appear to work, they cannot be depended upon to work reliably and are not supported." [1] http://infocenter.arm.com/help/topic/com.arm.doc.dui0774g/jhg1476893564298.html [2] https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html In this case it looks like the arguments are only used for sanity with __asmeq but in the first place the original commit for trusted foundations talks about it not respecting the API so naked makes sense but they're not making the API function naked (and the one which takes an argument does use its argument) so this all feels a bit weird to me. It might be worth asking the original authors on this one... -- Dominique Martinet
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote: > One reply for a bunch of the various threads, to keep the number of emails > down: > > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote: > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote: > > > +/* Compiler specific macros. */ > > > #ifdef __clang__ > > > #include > > > > probably better as > > > > #if defined(__clang) > > > > to match the style of the #elif defined()s below it > > Hi Joe, > Thanks for the feedback. I always appreciate it. If you have some > cleanups, want to send them to me, and I'll bundle them up for a PR? > I'm ok with that change. > > > > +#ifdef __GNUC_STDC_INLINE__ > > > +# define __gnu_inline__attribute__((gnu_inline)) > > > +#else > > > +# define __gnu_inline > > > +#endif > > > > Perhaps __gnu_inline should be in compiler-gcc and this > > should use > > > > #ifndef __gnu_inline > > #define __gnu_inline > > #endif > > Not this case; it's how we get gnu89 semantics for `extern inline` is > not compiler specific (therefor should not go in a compiler specific > header). It's not possible to know that compilers support what __attribute__(()) and at what version that support exists unless it is specified somewhere. As far as I can tell, gnu_inline is not recognized by clang. https://clang.llvm.org/docs/AttributeReference.html
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote: > One reply for a bunch of the various threads, to keep the number of emails > down: > > On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote: > > On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote: > > > +/* Compiler specific macros. */ > > > #ifdef __clang__ > > > #include > > > > probably better as > > > > #if defined(__clang) > > > > to match the style of the #elif defined()s below it > > Hi Joe, > Thanks for the feedback. I always appreciate it. If you have some > cleanups, want to send them to me, and I'll bundle them up for a PR? > I'm ok with that change. > > > > +#ifdef __GNUC_STDC_INLINE__ > > > +# define __gnu_inline__attribute__((gnu_inline)) > > > +#else > > > +# define __gnu_inline > > > +#endif > > > > Perhaps __gnu_inline should be in compiler-gcc and this > > should use > > > > #ifndef __gnu_inline > > #define __gnu_inline > > #endif > > Not this case; it's how we get gnu89 semantics for `extern inline` is > not compiler specific (therefor should not go in a compiler specific > header). It's not possible to know that compilers support what __attribute__(()) and at what version that support exists unless it is specified somewhere. As far as I can tell, gnu_inline is not recognized by clang. https://clang.llvm.org/docs/AttributeReference.html
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote: > One of these days, I'll get frustrated enough to > rewrite checkpatch.pl as a set of clang tidy checks (so that it > actually parses the code), but I'll have to learn how to read perl > first to start translating. Good luck with that, remember that checkpatch works on diff chunks and not files.
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Thu, 2018-08-23 at 14:03 -0700, Nick Desaulniers wrote: > One of these days, I'll get frustrated enough to > rewrite checkpatch.pl as a set of clang tidy checks (so that it > actually parses the code), but I'll have to learn how to read perl > first to start translating. Good luck with that, remember that checkpatch works on diff chunks and not files.
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
One reply for a bunch of the various threads, to keep the number of emails down:
On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote:
> On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > +/* Compiler specific macros. */
> > #ifdef __clang__
> > #include
>
> probably better as
>
> #if defined(__clang)
>
> to match the style of the #elif defined()s below it
Hi Joe,
Thanks for the feedback. I always appreciate it. If you have some
cleanups, want to send them to me, and I'll bundle them up for a PR?
I'm ok with that change.
> > +#ifdef __GNUC_STDC_INLINE__
> > +# define __gnu_inline__attribute__((gnu_inline))
> > +#else
> > +# define __gnu_inline
> > +#endif
>
> Perhaps __gnu_inline should be in compiler-gcc and this
> should use
>
> #ifndef __gnu_inline
> #define __gnu_inline
> #endif
Not this case; it's how we get gnu89 semantics for `extern inline` is
not compiler specific (therefor should not go in a compiler specific
header).
> > +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> > + !defined(CONFIG_OPTIMIZE_INLINING)
> > +#define inline \
> > + inline __attribute__((always_inline, unused)) notrace __gnu_inline
> > +#else
> > +#define inline inline__attribute__((unused)) notrace __gnu_inline
> > +#endif
>
> This bit might be better adding another __ attribute like:
>
> #if defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) &&
> defined(CONFIG_OPTIMIZE_INLINING)
> #define __optimized_inline __unused
> #else
> #define __optimized_inline __unused __attribute__((always_inline))
> #endif
>
> #define inline inline __optimized_inline notrace __gnu_inline
Sure, as above I'm happy to take clean ups, but that might result in
treewide changes (maybe less benefit but could separate `inline` from
`__optimized_inline`). Maybe bikeshed territory, but `force_inline`
or some notion that we're trying to overrule the compiler here might
make intent clearer.
> > -#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
> > +#if defined(CONFIG_ARM) && \
> > + defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700
>
> I find the reversed version tests a bit odd to read
Sorry, I probably didn't need to reorder that. My thought process was
in terms of short circuiting, what order was most likely for us to
bail out of the condition first? If it hurts readability, I'm happy
to take clean ups.
On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet
wrote:
> Overall looks good to me, just pointing at the same error I wrote in my
> other mail here -- I saw that by the time I was done writing this this
> patch got taken but that alone will probably warrant a follow-up :/
> > +#define barrier() (__asm__ __volatile__("" : : : "memory"))
>
> barrier here has gained external () compared to the definition and
> compiler-gcc.h:
> #define barrier() __asm__ __volatile__("": : :"memory")
Dominique,
Yes, sorry about that (looks like Linus noticed that, too). Was a
stupid last minute mistake on my part, trying to appease
checkpatch.pl. One of these days, I'll get frustrated enough to
rewrite checkpatch.pl as a set of clang tidy checks (so that it
actually parses the code), but I'll have to learn how to read perl
first to start translating.
> I've also added a few style nitpicks/questions but feel free to ignore
> these.
No, please, I really appreciate you taking the time to actually test
this and provide feedback. That kind of support is critical in open
source.
> On a side note, I noticed tools/include/linux/compiler.h includes
> linux/compiler-gcc.h but maybe it should include linux/compiler_types.h?
> (I'm not sure at who uses that header, so it really is an open question
> here)
Without looking into the code, this sounds like compiler.h is wrong.
Looking at the source, there's references to ancient Android NDK's
(what?! Let me show this to the NDK maintainers). This whole thing
needs to be cleaned up, too, IMO. Oh, there's two compiler-gcc.h in
the tree:
- tools/include/linux/compiler-gcc.h (that's what's being included in
the case you point out)
- include/linux/compiler-gcc.h
Maybe they can be combined?
> > -#define __nostackprotector __optimize("no-stack-protector")
>
> I do not see this being added back, it's probably fine though as it
> doesn't look used?
> (I didn't check all removed lines, maybe about half)
For each case, I triple checked that the macro was actually being used
in the code. __nostackprotector was not, so I dropped it.
Sounds like I may have messed up `__naked` though, see below.
> I'm not sure I understand the logic of where you removed ifndef and
> where you kept them (but, well, this should work)
There were some cases were some symbols were defined in glibc's
headers, so `make CC=clang` (implying that HOSTCC=gcc) would produce
redefinition warnings. I should've added a comment to clarify.
> If you tried to align these, __always_unused and __alias(symbol) look
> off
I have `set tabstop=8` in vim, and it
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
One reply for a bunch of the various threads, to keep the number of emails down:
On Wed, Aug 22, 2018 at 5:20 PM Joe Perches wrote:
> On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> > +/* Compiler specific macros. */
> > #ifdef __clang__
> > #include
>
> probably better as
>
> #if defined(__clang)
>
> to match the style of the #elif defined()s below it
Hi Joe,
Thanks for the feedback. I always appreciate it. If you have some
cleanups, want to send them to me, and I'll bundle them up for a PR?
I'm ok with that change.
> > +#ifdef __GNUC_STDC_INLINE__
> > +# define __gnu_inline__attribute__((gnu_inline))
> > +#else
> > +# define __gnu_inline
> > +#endif
>
> Perhaps __gnu_inline should be in compiler-gcc and this
> should use
>
> #ifndef __gnu_inline
> #define __gnu_inline
> #endif
Not this case; it's how we get gnu89 semantics for `extern inline` is
not compiler specific (therefor should not go in a compiler specific
header).
> > +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> > + !defined(CONFIG_OPTIMIZE_INLINING)
> > +#define inline \
> > + inline __attribute__((always_inline, unused)) notrace __gnu_inline
> > +#else
> > +#define inline inline__attribute__((unused)) notrace __gnu_inline
> > +#endif
>
> This bit might be better adding another __ attribute like:
>
> #if defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) &&
> defined(CONFIG_OPTIMIZE_INLINING)
> #define __optimized_inline __unused
> #else
> #define __optimized_inline __unused __attribute__((always_inline))
> #endif
>
> #define inline inline __optimized_inline notrace __gnu_inline
Sure, as above I'm happy to take clean ups, but that might result in
treewide changes (maybe less benefit but could separate `inline` from
`__optimized_inline`). Maybe bikeshed territory, but `force_inline`
or some notion that we're trying to overrule the compiler here might
make intent clearer.
> > -#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
> > +#if defined(CONFIG_ARM) && \
> > + defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700
>
> I find the reversed version tests a bit odd to read
Sorry, I probably didn't need to reorder that. My thought process was
in terms of short circuiting, what order was most likely for us to
bail out of the condition first? If it hurts readability, I'm happy
to take clean ups.
On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet
wrote:
> Overall looks good to me, just pointing at the same error I wrote in my
> other mail here -- I saw that by the time I was done writing this this
> patch got taken but that alone will probably warrant a follow-up :/
> > +#define barrier() (__asm__ __volatile__("" : : : "memory"))
>
> barrier here has gained external () compared to the definition and
> compiler-gcc.h:
> #define barrier() __asm__ __volatile__("": : :"memory")
Dominique,
Yes, sorry about that (looks like Linus noticed that, too). Was a
stupid last minute mistake on my part, trying to appease
checkpatch.pl. One of these days, I'll get frustrated enough to
rewrite checkpatch.pl as a set of clang tidy checks (so that it
actually parses the code), but I'll have to learn how to read perl
first to start translating.
> I've also added a few style nitpicks/questions but feel free to ignore
> these.
No, please, I really appreciate you taking the time to actually test
this and provide feedback. That kind of support is critical in open
source.
> On a side note, I noticed tools/include/linux/compiler.h includes
> linux/compiler-gcc.h but maybe it should include linux/compiler_types.h?
> (I'm not sure at who uses that header, so it really is an open question
> here)
Without looking into the code, this sounds like compiler.h is wrong.
Looking at the source, there's references to ancient Android NDK's
(what?! Let me show this to the NDK maintainers). This whole thing
needs to be cleaned up, too, IMO. Oh, there's two compiler-gcc.h in
the tree:
- tools/include/linux/compiler-gcc.h (that's what's being included in
the case you point out)
- include/linux/compiler-gcc.h
Maybe they can be combined?
> > -#define __nostackprotector __optimize("no-stack-protector")
>
> I do not see this being added back, it's probably fine though as it
> doesn't look used?
> (I didn't check all removed lines, maybe about half)
For each case, I triple checked that the macro was actually being used
in the code. __nostackprotector was not, so I dropped it.
Sounds like I may have messed up `__naked` though, see below.
> I'm not sure I understand the logic of where you removed ifndef and
> where you kept them (but, well, this should work)
There were some cases were some symbols were defined in glibc's
headers, so `make CC=clang` (implying that HOSTCC=gcc) would produce
redefinition warnings. I should've added a comment to clarify.
> If you tried to align these, __always_unused and __alias(symbol) look
> off
I have `set tabstop=8` in vim, and it
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 6:02 PM, Linus Torvalds wrote: > On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet > wrote: >> >> Overall looks good to me, just pointing at the same error I wrote in my >> other mail here -- I saw that by the time I was done writing this this >> patch got taken but that alone will probably warrant a follow-up :/ > > I've fixed that manually, but when I tried to test it I just hit the > > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. > > error. > > Do you have some experimental clang build with asm goto support? What > version? Or is it just that you're building ARM, not x86? FWIW, when I do Clang test builds lately[1], I've been using: make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CC=clang (Note this requires a cross-compiled binutils installed in the PATH with as aarch64-linux-gnu-*, which is where Debian and Ubuntu put them.) -Kees [1] for today's defconfig build to finish, I had to: ./scripts/config -d CONFIG_ARM64_LSE_ATOMICS (I think my binutils are old) and comment out the BUILD_BUG_ON() in net/core/filter.c from 2dbb9b9e6df67 (which looks like it needs some attention since gcc has no problem with this) -- Kees Cook Pixel Security
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 6:02 PM, Linus Torvalds wrote: > On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet > wrote: >> >> Overall looks good to me, just pointing at the same error I wrote in my >> other mail here -- I saw that by the time I was done writing this this >> patch got taken but that alone will probably warrant a follow-up :/ > > I've fixed that manually, but when I tried to test it I just hit the > > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. > > error. > > Do you have some experimental clang build with asm goto support? What > version? Or is it just that you're building ARM, not x86? FWIW, when I do Clang test builds lately[1], I've been using: make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- CC=clang (Note this requires a cross-compiled binutils installed in the PATH with as aarch64-linux-gnu-*, which is where Debian and Ubuntu put them.) -Kees [1] for today's defconfig build to finish, I had to: ./scripts/config -d CONFIG_ARM64_LSE_ATOMICS (I think my binutils are old) and comment out the BUILD_BUG_ON() in net/core/filter.c from 2dbb9b9e6df67 (which looks like it needs some attention since gcc has no problem with this) -- Kees Cook Pixel Security
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Hi Nick
2018-08-23 8:37 GMT+09:00 Nick Desaulniers :
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
>
> Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
> __GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
> added compiler specific checks for __clang__ or __INTEL_COMPILER.
>
> This is brittle, as they happened to get compatibility by posing as a
> certain version of GCC. This broke when upgrading the minimal version
> of GCC required to build the kernel, to a version above what ICC and
> Clang claim to be.
>
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.
>
> Reported-by: Masahiro Yamada
> Suggested-by: Eli Friedman
> Suggested-by: Joe Perches
> Signed-off-by: Nick Desaulniers
> ---
> arch/arm/kernel/asm-offsets.c | 13 +-
> drivers/gpu/drm/i915/i915_utils.h | 2 +-
> drivers/watchdog/kempld_wdt.c | 5 -
> include/linux/compiler-clang.h| 20 ++-
> include/linux/compiler-gcc.h | 88 ---
> include/linux/compiler-intel.h| 13 +-
> include/linux/compiler_types.h| 238 ++
> mm/ksm.c | 4 +-
> mm/migrate.c | 3 +-
> 9 files changed, 133 insertions(+), 253 deletions(-)
If I build ARM linux with Clang,
I see the following error.
arch/arm/firmware/trusted_foundations.c:34:13: error: variable has
incomplete type 'void'
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
^
arch/arm/firmware/trusted_foundations.c:34:20: error: expected ';'
after top level declarator
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
^
;
arch/arm/firmware/trusted_foundations.c:77:25: error: use of
undeclared identifier 'trusted_foundations_ops'
register_firmware_ops(_foundations_ops);
^
3 errors generated.
scripts/Makefile.build:307: recipe for target
'arch/arm/firmware/trusted_foundations.o' failed
make[2]: *** [arch/arm/firmware/trusted_foundations.o] Error 1
Makefile:1057: recipe for target 'arch/arm/firmware' failed
make[1]: *** [arch/arm/firmware] Error 2
make[1]: *** Waiting for unfinished jobs
__naked is defined in
It was previous included by all compilers,
but now it is only by _true_ GCC.
Even if I move the __naked definition
to ,
I see a different error.
arch/arm/firmware/trusted_foundations.c:47:10: error: parameter
references not allowed in naked functions
: "r" (type), "r" (arg1), "r" (arg2)
^
arch/arm/firmware/trusted_foundations.c:34:13: note: attribute is here
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
^
./include/linux/compiler_types.h:277:33: note: expanded from macro '__naked'
#define __naked __attribute__((naked))
^
I do not know what a correct fix is.
--
Best Regards
Masahiro Yamada
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Hi Nick
2018-08-23 8:37 GMT+09:00 Nick Desaulniers :
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
>
> Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
> __GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
> added compiler specific checks for __clang__ or __INTEL_COMPILER.
>
> This is brittle, as they happened to get compatibility by posing as a
> certain version of GCC. This broke when upgrading the minimal version
> of GCC required to build the kernel, to a version above what ICC and
> Clang claim to be.
>
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.
>
> Reported-by: Masahiro Yamada
> Suggested-by: Eli Friedman
> Suggested-by: Joe Perches
> Signed-off-by: Nick Desaulniers
> ---
> arch/arm/kernel/asm-offsets.c | 13 +-
> drivers/gpu/drm/i915/i915_utils.h | 2 +-
> drivers/watchdog/kempld_wdt.c | 5 -
> include/linux/compiler-clang.h| 20 ++-
> include/linux/compiler-gcc.h | 88 ---
> include/linux/compiler-intel.h| 13 +-
> include/linux/compiler_types.h| 238 ++
> mm/ksm.c | 4 +-
> mm/migrate.c | 3 +-
> 9 files changed, 133 insertions(+), 253 deletions(-)
If I build ARM linux with Clang,
I see the following error.
arch/arm/firmware/trusted_foundations.c:34:13: error: variable has
incomplete type 'void'
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
^
arch/arm/firmware/trusted_foundations.c:34:20: error: expected ';'
after top level declarator
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
^
;
arch/arm/firmware/trusted_foundations.c:77:25: error: use of
undeclared identifier 'trusted_foundations_ops'
register_firmware_ops(_foundations_ops);
^
3 errors generated.
scripts/Makefile.build:307: recipe for target
'arch/arm/firmware/trusted_foundations.o' failed
make[2]: *** [arch/arm/firmware/trusted_foundations.o] Error 1
Makefile:1057: recipe for target 'arch/arm/firmware' failed
make[1]: *** [arch/arm/firmware] Error 2
make[1]: *** Waiting for unfinished jobs
__naked is defined in
It was previous included by all compilers,
but now it is only by _true_ GCC.
Even if I move the __naked definition
to ,
I see a different error.
arch/arm/firmware/trusted_foundations.c:47:10: error: parameter
references not allowed in naked functions
: "r" (type), "r" (arg1), "r" (arg2)
^
arch/arm/firmware/trusted_foundations.c:34:13: note: attribute is here
static void __naked tf_generic_smc(u32 type, u32 arg1, u32 arg2)
^
./include/linux/compiler_types.h:277:33: note: expanded from macro '__naked'
#define __naked __attribute__((naked))
^
I do not know what a correct fix is.
--
Best Regards
Masahiro Yamada
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 6:10 PM Dominique Martinet wrote: > > I'm not building linux directly, but BPF programs that indirectly uses > clang with bcc Oh, ok. I _can_ test the basic build (just not get a working link and a kernel) by just forcing it, so I guess I'll call that testing enough. I'll push it out. It's just Nick's patch, but with the extra barrier parenthesis removed. Linus
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 6:10 PM Dominique Martinet wrote: > > I'm not building linux directly, but BPF programs that indirectly uses > clang with bcc Oh, ok. I _can_ test the basic build (just not get a working link and a kernel) by just forcing it, so I guess I'll call that testing enough. I'll push it out. It's just Nick's patch, but with the extra barrier parenthesis removed. Linus
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Linus Torvalds wrote on Wed, Aug 22, 2018: > I've fixed that manually, but when I tried to test it I just hit the > > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. > > error. > > Do you have some experimental clang build with asm goto support? What > version? Or is it just that you're building ARM, not x86? I'm not building linux directly, but BPF programs that indirectly uses clang with bcc On fedora you can install bcc-tools and run e.g. /usr/share/bcc/tools/execsnoop that will compile something at runtime. If the package isn't available for your distro you can just install bcc and run the script from their repo[1] [1] https://raw.githubusercontent.com/iovisor/bcc/master/tools/execsnoop.py Thanks, -- Dominique
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Linus Torvalds wrote on Wed, Aug 22, 2018: > I've fixed that manually, but when I tried to test it I just hit the > > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. > > error. > > Do you have some experimental clang build with asm goto support? What > version? Or is it just that you're building ARM, not x86? I'm not building linux directly, but BPF programs that indirectly uses clang with bcc On fedora you can install bcc-tools and run e.g. /usr/share/bcc/tools/execsnoop that will compile something at runtime. If the package isn't available for your distro you can just install bcc and run the script from their repo[1] [1] https://raw.githubusercontent.com/iovisor/bcc/master/tools/execsnoop.py Thanks, -- Dominique
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet wrote: > > Overall looks good to me, just pointing at the same error I wrote in my > other mail here -- I saw that by the time I was done writing this this > patch got taken but that alone will probably warrant a follow-up :/ I've fixed that manually, but when I tried to test it I just hit the arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. error. Do you have some experimental clang build with asm goto support? What version? Or is it just that you're building ARM, not x86? Linus
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 5:25 PM Dominique Martinet wrote: > > Overall looks good to me, just pointing at the same error I wrote in my > other mail here -- I saw that by the time I was done writing this this > patch got taken but that alone will probably warrant a follow-up :/ I've fixed that manually, but when I tried to test it I just hit the arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. error. Do you have some experimental clang build with asm goto support? What version? Or is it just that you're building ARM, not x86? Linus
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Nick Desaulniers wrote on Wed, Aug 22, 2018:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
>
> Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
> __GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
> added compiler specific checks for __clang__ or __INTEL_COMPILER.
>
> This is brittle, as they happened to get compatibility by posing as a
> certain version of GCC. This broke when upgrading the minimal version
> of GCC required to build the kernel, to a version above what ICC and
> Clang claim to be.
>
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.
>
> Reported-by: Masahiro Yamada
> Suggested-by: Eli Friedman
> Suggested-by: Joe Perches
> Signed-off-by: Nick Desaulniers
Overall looks good to me, just pointing at the same error I wrote in my
other mail here -- I saw that by the time I was done writing this this
patch got taken but that alone will probably warrant a follow-up :/
I've also added a few style nitpicks/questions but feel free to ignore
these.
On a side note, I noticed tools/include/linux/compiler.h includes
linux/compiler-gcc.h but maybe it should include linux/compiler_types.h?
(I'm not sure at who uses that header, so it really is an open question
here)
> ---
> arch/arm/kernel/asm-offsets.c | 13 +-
> drivers/gpu/drm/i915/i915_utils.h | 2 +-
> drivers/watchdog/kempld_wdt.c | 5 -
> include/linux/compiler-clang.h| 20 ++-
> include/linux/compiler-gcc.h | 88 ---
> include/linux/compiler-intel.h| 13 +-
> include/linux/compiler_types.h| 238 ++
> mm/ksm.c | 4 +-
> mm/migrate.c | 3 +-
> 9 files changed, 133 insertions(+), 253 deletions(-)
>
> [...]
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 7087446c24c8..5f0754692ce6 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> [...]
> @@ -40,9 +30,17 @@
> * checks. Unfortunately, we don't know which version of gcc clang
> * pretends to be, so the macro may or may not be defined.
> */
> -#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
> #if __has_builtin(__builtin_mul_overflow) && \
> __has_builtin(__builtin_add_overflow) && \
> __has_builtin(__builtin_sub_overflow)
> #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> #endif
> +
> +/* The following are for compatibility with GCC, from compiler-gcc.h,
> + * and may be redefined here because they should not be shared with other
> + * compilers, like ICC.
> + */
> +#define barrier() (__asm__ __volatile__("" : : : "memory"))
barrier here has gained external () compared to the definition and
compiler-gcc.h:
#define barrier() __asm__ __volatile__("": : :"memory")
And this fails with bpf:
In file included from :3:
In file included from /virtual/include/bcc/helpers.h:23:
In file included from /lib/modules/4.18.0+/build/include/linux/log2.h:16:
In file included from /lib/modules/4.18.0+/build/include/linux/bitops.h:18:
/lib/modules/4.18.0+/build/arch/x86/include/asm/bitops.h:170:2: error: expected
expression
barrier();
^
/lib/modules/4.18.0+/build/include/linux/compiler-clang.h:43:20: note: expanded
from macro 'barrier'
#define barrier() (__asm__ __volatile__("" : : : "memory"))
^
> +#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> +#define __assume_aligned(a, ...) \
> + __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 250b9b7cfd60..763bbad1e258 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> [..]
> -#define __cold __attribute__((__cold__))
> -
> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
> __COUNTER__)
>
> #define __optimize(level)__attribute__((__optimize__(level)))
> -#define __nostackprotector __optimize("no-stack-protector")
I do not see this being added back, it's probably fine though as it
doesn't look used?
(I didn't check all removed lines, maybe about half)
> #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index fbf337933fd8..90479a0f3986 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> [...]
> /* Is this type a native word size -- useful for atomic operations */
> -#ifndef __native_word
> -# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) ==
> sizeof(short) || sizeof(t) ==
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
Nick Desaulniers wrote on Wed, Aug 22, 2018:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
>
> Both Clang and ICC define __GNUC__, __GNUC_MINOR__, and
> __GNUC_PATCHLEVEL__ for quick compatibility with code bases that haven't
> added compiler specific checks for __clang__ or __INTEL_COMPILER.
>
> This is brittle, as they happened to get compatibility by posing as a
> certain version of GCC. This broke when upgrading the minimal version
> of GCC required to build the kernel, to a version above what ICC and
> Clang claim to be.
>
> Rather than always including compiler-gcc.h then undefining or
> redefining macros in compiler-intel.h or compiler-clang.h, let's
> separate out the compiler specific macro definitions into mutually
> exclusive headers, do more proper compiler detection, and keep shared
> definitions in compiler_types.h.
>
> Reported-by: Masahiro Yamada
> Suggested-by: Eli Friedman
> Suggested-by: Joe Perches
> Signed-off-by: Nick Desaulniers
Overall looks good to me, just pointing at the same error I wrote in my
other mail here -- I saw that by the time I was done writing this this
patch got taken but that alone will probably warrant a follow-up :/
I've also added a few style nitpicks/questions but feel free to ignore
these.
On a side note, I noticed tools/include/linux/compiler.h includes
linux/compiler-gcc.h but maybe it should include linux/compiler_types.h?
(I'm not sure at who uses that header, so it really is an open question
here)
> ---
> arch/arm/kernel/asm-offsets.c | 13 +-
> drivers/gpu/drm/i915/i915_utils.h | 2 +-
> drivers/watchdog/kempld_wdt.c | 5 -
> include/linux/compiler-clang.h| 20 ++-
> include/linux/compiler-gcc.h | 88 ---
> include/linux/compiler-intel.h| 13 +-
> include/linux/compiler_types.h| 238 ++
> mm/ksm.c | 4 +-
> mm/migrate.c | 3 +-
> 9 files changed, 133 insertions(+), 253 deletions(-)
>
> [...]
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 7087446c24c8..5f0754692ce6 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> [...]
> @@ -40,9 +30,17 @@
> * checks. Unfortunately, we don't know which version of gcc clang
> * pretends to be, so the macro may or may not be defined.
> */
> -#undef COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW
> #if __has_builtin(__builtin_mul_overflow) && \
> __has_builtin(__builtin_add_overflow) && \
> __has_builtin(__builtin_sub_overflow)
> #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1
> #endif
> +
> +/* The following are for compatibility with GCC, from compiler-gcc.h,
> + * and may be redefined here because they should not be shared with other
> + * compilers, like ICC.
> + */
> +#define barrier() (__asm__ __volatile__("" : : : "memory"))
barrier here has gained external () compared to the definition and
compiler-gcc.h:
#define barrier() __asm__ __volatile__("": : :"memory")
And this fails with bpf:
In file included from :3:
In file included from /virtual/include/bcc/helpers.h:23:
In file included from /lib/modules/4.18.0+/build/include/linux/log2.h:16:
In file included from /lib/modules/4.18.0+/build/include/linux/bitops.h:18:
/lib/modules/4.18.0+/build/arch/x86/include/asm/bitops.h:170:2: error: expected
expression
barrier();
^
/lib/modules/4.18.0+/build/include/linux/compiler-clang.h:43:20: note: expanded
from macro 'barrier'
#define barrier() (__asm__ __volatile__("" : : : "memory"))
^
> +#define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a), &(a)[0]))
> +#define __assume_aligned(a, ...) \
> + __attribute__((__assume_aligned__(a, ## __VA_ARGS__)))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index 250b9b7cfd60..763bbad1e258 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> [..]
> -#define __cold __attribute__((__cold__))
> -
> #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix),
> __COUNTER__)
>
> #define __optimize(level)__attribute__((__optimize__(level)))
> -#define __nostackprotector __optimize("no-stack-protector")
I do not see this being added back, it's probably fine though as it
doesn't look used?
(I didn't check all removed lines, maybe about half)
> #define __compiletime_object_size(obj) __builtin_object_size(obj, 0)
>
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index fbf337933fd8..90479a0f3986 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> [...]
> /* Is this type a native word size -- useful for atomic operations */
> -#ifndef __native_word
> -# define __native_word(t) (sizeof(t) == sizeof(char) || sizeof(t) ==
> sizeof(short) || sizeof(t) ==
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 4:39 PM Nick Desaulniers wrote: > > Rather than always including compiler-gcc.h then undefining or > redefining macros in compiler-intel.h or compiler-clang.h, let's > separate out the compiler specific macro definitions into mutually > exclusive headers, do more proper compiler detection, and keep shared > definitions in compiler_types.h. Ack, especially with the other cleanups leading to > 9 files changed, 133 insertions(+), 253 deletions(-) and I just took it directly, since I had taken the original cafa0010cd51 directly too. I'm sure this can still be tweaked, but let's get the baseline in and working. Linus
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, Aug 22, 2018 at 4:39 PM Nick Desaulniers wrote: > > Rather than always including compiler-gcc.h then undefining or > redefining macros in compiler-intel.h or compiler-clang.h, let's > separate out the compiler specific macro definitions into mutually > exclusive headers, do more proper compiler detection, and keep shared > definitions in compiler_types.h. Ack, especially with the other cleanups leading to > 9 files changed, 133 insertions(+), 253 deletions(-) and I just took it directly, since I had taken the original cafa0010cd51 directly too. I'm sure this can still be tweaked, but let's get the baseline in and working. Linus
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
style trivia:
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
[]
> @@ -54,32 +54,20 @@ extern void __chk_io_ptr(const volatile void __iomem *);
[]
> +/* Compiler specific macros. */
> #ifdef __clang__
> #include
probably better as
#if defined(__clang)
to match the style of the #elif defined()s below it
> +#elif defined(__INTEL_COMPILER)
> +#include
> +#elif defined(__GNUC__)
[]
> @@ -272,4 +174,92 @@ struct ftrace_likely_data {
[]
> +#ifdef __GNUC_STDC_INLINE__
> +# define __gnu_inline__attribute__((gnu_inline))
> +#else
> +# define __gnu_inline
> +#endif
Perhaps __gnu_inline should be in compiler-gcc and this
should use
#ifndef __gnu_inline
#define __gnu_inline
#endif
> +
> +/*
> + * Force always-inline if the user requests it so via the .config.
> + * GCC does not warn about unused static inline functions for
> + * -Wunused-function. This turns out to avoid the need for complex #ifdef
> + * directives. Suppress the warning in clang as well by using "unused"
> + * function attribute, which is redundant but not harmful for gcc.
> + * Prefer gnu_inline, so that extern inline functions do not emit an
> + * externally visible function. This makes extern inline behave as per gnu89
> + * semantics rather than c99. This prevents multiple symbol definition errors
> + * of extern inline functions at link time.
> + * A lot of inline functions can cause havoc with function tracing.
> + */
> +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> + !defined(CONFIG_OPTIMIZE_INLINING)
> +#define inline \
> + inline __attribute__((always_inline, unused)) notrace __gnu_inline
> +#else
> +#define inline inline__attribute__((unused)) notrace __gnu_inline
> +#endif
This bit might be better adding another __ attribute like:
#if defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) &&
defined(CONFIG_OPTIMIZE_INLINING)
#define __optimized_inline __unused
#else
#define __optimized_inline __unused __attribute__((always_inline))
#endif
#define inline inline __optimized_inline notrace __gnu_inline
> +
> +#define __inline__ inline
> +#define __inline inline
> +#define noinline __attribute__((noinline))
> +
> +#ifndef __always_inline
> +#define __always_inline inline __attribute__((always_inline))
> +#endif
[]
> diff --git a/mm/migrate.c b/mm/migrate.c
[]
> @@ -1131,7 +1131,8 @@ static int __unmap_and_move(struct page *page, struct
> page *newpage,
> * gcc 4.7 and 4.8 on arm get an ICEs when inlining unmap_and_move(). Work
> * around it.
> */
> -#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
> +#if defined(CONFIG_ARM) && \
> + defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700
I find the reversed version tests a bit odd to read
> #define ICE_noinline noinline
> #else
> #define ICE_noinline
Re: [PATCH] include/linux/compiler*.h: make compiler-*.h mutually exclusive
On Wed, 2018-08-22 at 16:37 -0700, Nick Desaulniers wrote:
> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6")
> recently exposed a brittle part of the build for supporting non-gcc
> compilers.
style trivia:
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
[]
> @@ -54,32 +54,20 @@ extern void __chk_io_ptr(const volatile void __iomem *);
[]
> +/* Compiler specific macros. */
> #ifdef __clang__
> #include
probably better as
#if defined(__clang)
to match the style of the #elif defined()s below it
> +#elif defined(__INTEL_COMPILER)
> +#include
> +#elif defined(__GNUC__)
[]
> @@ -272,4 +174,92 @@ struct ftrace_likely_data {
[]
> +#ifdef __GNUC_STDC_INLINE__
> +# define __gnu_inline__attribute__((gnu_inline))
> +#else
> +# define __gnu_inline
> +#endif
Perhaps __gnu_inline should be in compiler-gcc and this
should use
#ifndef __gnu_inline
#define __gnu_inline
#endif
> +
> +/*
> + * Force always-inline if the user requests it so via the .config.
> + * GCC does not warn about unused static inline functions for
> + * -Wunused-function. This turns out to avoid the need for complex #ifdef
> + * directives. Suppress the warning in clang as well by using "unused"
> + * function attribute, which is redundant but not harmful for gcc.
> + * Prefer gnu_inline, so that extern inline functions do not emit an
> + * externally visible function. This makes extern inline behave as per gnu89
> + * semantics rather than c99. This prevents multiple symbol definition errors
> + * of extern inline functions at link time.
> + * A lot of inline functions can cause havoc with function tracing.
> + */
> +#if !defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) || \
> + !defined(CONFIG_OPTIMIZE_INLINING)
> +#define inline \
> + inline __attribute__((always_inline, unused)) notrace __gnu_inline
> +#else
> +#define inline inline__attribute__((unused)) notrace __gnu_inline
> +#endif
This bit might be better adding another __ attribute like:
#if defined(CONFIG_ARCH_SUPPORTS_OPTIMIZED_INLINING) &&
defined(CONFIG_OPTIMIZE_INLINING)
#define __optimized_inline __unused
#else
#define __optimized_inline __unused __attribute__((always_inline))
#endif
#define inline inline __optimized_inline notrace __gnu_inline
> +
> +#define __inline__ inline
> +#define __inline inline
> +#define noinline __attribute__((noinline))
> +
> +#ifndef __always_inline
> +#define __always_inline inline __attribute__((always_inline))
> +#endif
[]
> diff --git a/mm/migrate.c b/mm/migrate.c
[]
> @@ -1131,7 +1131,8 @@ static int __unmap_and_move(struct page *page, struct
> page *newpage,
> * gcc 4.7 and 4.8 on arm get an ICEs when inlining unmap_and_move(). Work
> * around it.
> */
> -#if (GCC_VERSION >= 40700 && GCC_VERSION < 40900) && defined(CONFIG_ARM)
> +#if defined(CONFIG_ARM) && \
> + defined(GCC_VERSION) && GCC_VERSION < 40900 && GCC_VERSION >= 40700
I find the reversed version tests a bit odd to read
> #define ICE_noinline noinline
> #else
> #define ICE_noinline
