| Message ID | 1534834088-15835-1-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | New |
| Headers | show |
| Series | compiler-gcc: get back Clang build | expand |
Thanks for noticing, and sending this patch. I'm happy to see others testing with Clang. I noticed this too near the end of the day https://github.com/ClangBuiltLinux/linux/issues/27. On Mon, Aug 20, 2018 at 11:48 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > missed the fact that <linux/compiler-gcc.h> is included by Clang > as well as by GCC. > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > and it looks like GCC 4.2.1. > > $ scripts/gcc-version.sh -p clang > 040201 If you too are wondering why, see: https://reviews.llvm.org/D51011#1206981. > > If you try to build the kernel with Clang, you will get the > "Sorry, your compiler is too old - please upgrade it." > followed by a bunch of "unknown attribute" warnings. > > Add !defined(__clang__) to the minimum version check. I think this may eventually be part of a more-proper compiler check from the C preprocessor. > > Also, revive the version test blocks for versions >= 4.2.1 > in order to disable features not supported by Clang. Eh, this I'm not too enthused to see these being added back. > > Fixes: cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> > --- > > include/linux/compiler-gcc.h | 34 ++++++++++++++++++++++++++++------ > 1 file changed, 28 insertions(+), 6 deletions(-) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 250b9b7..8e41fd2 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -10,7 +10,7 @@ > + __GNUC_MINOR__ * 100 \ > + __GNUC_PATCHLEVEL__) > > -#if GCC_VERSION < 40600 > +#if !defined(__clang__) && GCC_VERSION < 40600 > # error Sorry, your compiler is too old - please upgrade it. > #endif > > @@ -163,7 +163,16 @@ > #define __used __attribute__((__used__)) > #define __compiler_offsetof(a, b) \ > __builtin_offsetof(a, b) > +#define __compiletime_object_size(obj) __builtin_object_size(obj, 0) > > +/* > + * GCC version specific checks > + * > + * Keep the version checks for 4.2.1 or newer > + * because Clang defines GCC_VERSION as 40201 > + */ > + > +#if GCC_VERSION >= 40300 > /* Mark functions as cold. gcc will assume any path leading to a call > * to them will be unlikely. This means a lot of manual unlikely()s > * are unnecessary now for any paths leading to the usual suspects > @@ -182,15 +191,20 @@ > > #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) > > +#ifndef __CHECKER__ > +# define __compiletime_warning(message) __attribute__((warning(message))) > +# define __compiletime_error(message) __attribute__((error(message))) > +#endif /* __CHECKER__ */ > +#endif /* GCC_VERSION >= 40300 */ > + > +#if GCC_VERSION >= 40400 > #define __optimize(level) __attribute__((__optimize__(level))) > #define __nostackprotector __optimize("no-stack-protector") > +#endif /* GCC_VERSION >= 40400 */ > > -#define __compiletime_object_size(obj) __builtin_object_size(obj, 0) > +#if GCC_VERSION >= 40500 > > #ifndef __CHECKER__ > -#define __compiletime_warning(message) __attribute__((warning(message))) > -#define __compiletime_error(message) __attribute__((error(message))) > - > #ifdef LATENT_ENTROPY_PLUGIN > #define __latent_entropy __attribute__((latent_entropy)) > #endif > @@ -232,6 +246,9 @@ > #define randomized_struct_fields_end } __randomize_layout; > #endif > > +#endif /* GCC_VERSION >= 40500 */ > + > +#if GCC_VERSION >= 40600 > /* > * When used with Link Time Optimization, gcc can optimize away C functions or > * variables which are referenced only from assembly code. __visible tells the > @@ -240,7 +257,7 @@ > */ > #define __visible __attribute__((externally_visible)) > > -/* gcc version specific checks */ > +#endif /* GCC_VERSION >= 40600 */ > > #if GCC_VERSION >= 40900 && !defined(__CHECKER__) > /* > @@ -274,8 +291,10 @@ > * folding in __builtin_bswap*() (yet), so don't set these for it. > */ > #if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP) && !defined(__CHECKER__) > +#if GCC_VERSION >= 40400 > #define __HAVE_BUILTIN_BSWAP32__ > #define __HAVE_BUILTIN_BSWAP64__ > +#endif > #if GCC_VERSION >= 40800 > #define __HAVE_BUILTIN_BSWAP16__ > #endif > @@ -327,9 +346,12 @@ > #define __diag_GCC_warn warning > #define __diag_GCC_error error > > +/* Compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */ > +#if GCC_VERSION >= 40600 > #define __diag_str1(s) #s > #define __diag_str(s) __diag_str1(s) > #define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) > +#endif > > #if GCC_VERSION >= 80000 > #define __diag_GCC_8(s) __diag(s) > -- > 2.7.4 > I think the current state of always including compiler-gcc.h, then possibly including compiler-clang.h or compiler-intel.h to overwrite some things is kind of a spaghetti mess, because now we wind up with these circular dependencies on GCC_VERSION. And that Clang just happened to declare __GNUC_MAJOR__ and friends in a way that didn't blow up until today was a bit of luck; that was a time bomb waiting to go off. I think it's time to shave this yak. Adding these GCC_VERSION checks back doesn't simplify the state of things. I think a more proper fix might be something along the lines of (in compiler_types.h): #include <linux/compiler-generic.h> /* potential new header for common definitions (or just put them above) */ #if /* more proper check for gcc and JUST gcc */ #include <linux/compiler-gcc.h> #elif /* compiler check for icc */ #include <linux/compiler-intel.h> #elif /* compiler check for clang */ #include <linux/compiler-clang.h> #endif #ifndef /* something from the above that's not mission critical */ #warning "compiler missing foo" #undef foo #endif #ifndef /* something from above that IS mission critical */ #error "compiler missing bar" #endif I appreciate the patch, but I think there's another way we can clean this up. -- Thanks, ~Nick Desaulniers
On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > missed the fact that <linux/compiler-gcc.h> is included by Clang > as well as by GCC. > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > and it looks like GCC 4.2.1. > > $ scripts/gcc-version.sh -p clang > 040201 Perhaps this would work, but I can't test it as my clang version doesn't otherwise build a defconfig and errors out with $ make CC=clang arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. --- include/linux/compiler-gcc.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 3e70b7d4e9ed..3a06ad823fa4 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -3,6 +3,23 @@ #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." #endif +/* + * Override clang compiler version #defines + * + * compiler_types.h always #includes compiler-gcc.h before compiler-clang,h + * but clang sets these __GNUC version #defines to 4.2.1. + * This breaks the gcc minimum version of 4.6.0, so override the clang + * definitions to 4.6.0 + */ +#ifdef __clang__ + #undef __GNUC__ + #undef __GNUC_MINOR__ + #undef __GNUC_PATCHLEVEL__ + #define __GNUC__ 4 + #define __GNUC_MINOR__ 6 + #define __GNUC_PATCHLEVEL__ 0 +#endif +
Nick Desaulniers Aug. 21, 2018, 8:09 a.m. UTC: > Thanks for noticing, and sending this patch. I'm happy to see others > testing with Clang. I noticed this too near the end of the day > https://github.com/ClangBuiltLinux/linux/issues/27. FWIW libbcc so many BPF users also use clang, so this has more impact than just testing to build linux with clang (not that this would be any reason to delay fixing either way) I would tend to agree havin a compiler-common + make clang/intel not include compiler-gcc would probably be best in the long run but we might want a quick fix for 4.19 meanwhile.. -- Dominique Martinet
On Tue, Aug 21, 2018 at 5:38 AM Dominique Martinet <asmadeus@codewreck.org> wrote: > > Nick Desaulniers Aug. 21, 2018, 8:09 a.m. UTC: > > Thanks for noticing, and sending this patch. I'm happy to see others > > testing with Clang. I noticed this too near the end of the day > > https://github.com/ClangBuiltLinux/linux/issues/27. > > FWIW libbcc so many BPF users also use clang, so this has more impact > than just testing to build linux with clang (not that this would be any > reason to delay fixing either way) > > I would tend to agree havin a compiler-common + make clang/intel not > include compiler-gcc would probably be best in the long run but we might > want a quick fix for 4.19 meanwhile.. That's fair. SOP here is quick (full) revert, then come up with a better fix. And I do prefer Masahiro's partial revert to a full revert of Joe's patch. That will give us more time to develop the proper fix rather than rush. I'll try to see how we can more properly split the compiler specific headers. Tested with gcc-7 and clang-8. Tested-by: Nick Desaulniers <ndesaulniers@google.com> -- Thanks, ~Nick Desaulniers
On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > missed the fact that <linux/compiler-gcc.h> is included by Clang > as well as by GCC. > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > and it looks like GCC 4.2.1. > > $ scripts/gcc-version.sh -p clang > 040201 > > If you try to build the kernel with Clang, you will get the > "Sorry, your compiler is too old - please upgrade it." > followed by a bunch of "unknown attribute" warnings. > > Add !defined(__clang__) to the minimum version check. > > Also, revive the version test blocks for versions >= 4.2.1 > in order to disable features not supported by Clang. What is the minimum clang version required to compile the kernel? What features are not supported by the minimum clang version? On my system, using clang $ clang -v clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) and $ git checkout v4.16 ; make clean ; make CC=clang defconfig ; make CC=clang HEAD is now at 0adb32858b0b... Linux 4.16 is successful but $ git checkout v4.17 ; make clean ; make CC=clang defconfig ; make CC=clang HEAD is now at 29dcea88779c... Linux 4.17 arch/x86/Makefile:184: *** Compiler lacks asm-goto support.. Stop. arch/x86/Makefile:184: *** Compiler lacks asm-goto support.. Stop.
On Tue, Aug 21, 2018 at 3:39 AM Joe Perches <joe@perches.com> wrote: > > On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: > > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > > missed the fact that <linux/compiler-gcc.h> is included by Clang > > as well as by GCC. > > > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > > and it looks like GCC 4.2.1. > > > > $ scripts/gcc-version.sh -p clang > > 040201 > > Perhaps this would work, but I can't test it as > my clang version doesn't otherwise build a defconfig > and errors out with > > $ make CC=clang > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. Sorry, we're working on implementing this in clang and llvm for x86. I recently reviewed the design doc, and am trying to see how I can actively help push this along. It's not a small/quick change to llvm, but we're working on it. -- Thanks, ~Nick Desaulniers
On Tue, 2018-08-21 at 09:32 -0700, Nick Desaulniers wrote: > On Tue, Aug 21, 2018 at 5:38 AM Dominique Martinet > <asmadeus@codewreck.org> wrote: > > > > Nick Desaulniers Aug. 21, 2018, 8:09 a.m. UTC: > > > Thanks for noticing, and sending this patch. I'm happy to see others > > > testing with Clang. I noticed this too near the end of the day > > > https://github.com/ClangBuiltLinux/linux/issues/27. > > > > FWIW libbcc so many BPF users also use clang, so this has more impact > > than just testing to build linux with clang (not that this would be any > > reason to delay fixing either way) > > > > I would tend to agree havin a compiler-common + make clang/intel not > > include compiler-gcc would probably be best in the long run but we might > > want a quick fix for 4.19 meanwhile.. > > That's fair. SOP here is quick (full) revert, then come up with a > better fix. And I do prefer Masahiro's partial revert to a full > revert of Joe's patch. That will give us more time to develop the > proper fix rather than rush. I'll try to see how we can more properly > split the compiler specific headers. > > Tested with gcc-7 and clang-8. clang-8? Isn't the latest officlal clang 6.0.1 ? http://releases.llvm.org/ vs https://clang.llvm.org/docs/ReleaseNotes.html So if something other than 6.0.x is required, then some additional check should probably be added to compiler-clang.h as well.
On Tue, Aug 21, 2018 at 9:33 AM Joe Perches <joe@perches.com> wrote: > > On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: > > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > > missed the fact that <linux/compiler-gcc.h> is included by Clang > > as well as by GCC. > > > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > > and it looks like GCC 4.2.1. > > > > $ scripts/gcc-version.sh -p clang > > 040201 > > > > If you try to build the kernel with Clang, you will get the > > "Sorry, your compiler is too old - please upgrade it." > > followed by a bunch of "unknown attribute" warnings. > > > > Add !defined(__clang__) to the minimum version check. > > > > Also, revive the version test blocks for versions >= 4.2.1 > > in order to disable features not supported by Clang. > > What is the minimum clang version required to compile the kernel? Depends on the architecture and which kernel version/LTS branch you're using. I'm trying to backport fixes to LTS branches, but sometimes a compiler upgrade is required. I know that's not great, but I'm actively trying to fix it. > What features are not supported by the minimum clang version? > > On my system, using clang > > $ clang -v > clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) > > and > > $ git checkout v4.16 ; make clean ; make CC=clang defconfig ; make CC=clang > HEAD is now at 0adb32858b0b... Linux 4.16 > > is successful > > but > > $ git checkout v4.17 ; make clean ; make CC=clang defconfig ; make CC=clang > HEAD is now at 29dcea88779c... Linux 4.17 > arch/x86/Makefile:184: *** Compiler lacks asm-goto support.. Stop. > arch/x86/Makefile:184: *** Compiler lacks asm-goto support.. Stop. > See commit e501ce9 ("x86: Force asm-goto"). $ git describe --contains e501ce9 | sed 's/~.*//' v4.17-rc1 -- Thanks, ~Nick Desaulniers
On Tue, Aug 21, 2018 at 9:45 AM Joe Perches <joe@perches.com> wrote: > > On Tue, 2018-08-21 at 09:32 -0700, Nick Desaulniers wrote: > > On Tue, Aug 21, 2018 at 5:38 AM Dominique Martinet > > <asmadeus@codewreck.org> wrote: > > > > > > Nick Desaulniers Aug. 21, 2018, 8:09 a.m. UTC: > > > > Thanks for noticing, and sending this patch. I'm happy to see others > > > > testing with Clang. I noticed this too near the end of the day > > > > https://github.com/ClangBuiltLinux/linux/issues/27. > > > > > > FWIW libbcc so many BPF users also use clang, so this has more impact > > > than just testing to build linux with clang (not that this would be any > > > reason to delay fixing either way) > > > > > > I would tend to agree havin a compiler-common + make clang/intel not > > > include compiler-gcc would probably be best in the long run but we might > > > want a quick fix for 4.19 meanwhile.. > > > > That's fair. SOP here is quick (full) revert, then come up with a > > better fix. And I do prefer Masahiro's partial revert to a full > > revert of Joe's patch. That will give us more time to develop the > > proper fix rather than rush. I'll try to see how we can more properly > > split the compiler specific headers. > > > > Tested with gcc-7 and clang-8. > > clang-8? Isn't the latest officlal clang 6.0.1 ? Yes, but I have a local llvm tree that I work out of, that's in my $PATH, so my version of clang is never too far behind Top of Tree. For android, we're using clang-5, but currently staging an upgrade to clang 6.0.1. > So if something other than 6.0.x is required, > then some additional check should probably be > added to compiler-clang.h as well. > Sure, but that doesn't need to go in Mashiro's patch today. That can wait for a proper separation between compiler headers where we can then implement improved version checks. -- Thanks, ~Nick Desaulniers
2018-08-22 1:33 GMT+09:00 Joe Perches <joe@perches.com>: > On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: >> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") >> missed the fact that <linux/compiler-gcc.h> is included by Clang >> as well as by GCC. >> >> Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ >> and it looks like GCC 4.2.1. >> >> $ scripts/gcc-version.sh -p clang >> 040201 >> >> If you try to build the kernel with Clang, you will get the >> "Sorry, your compiler is too old - please upgrade it." >> followed by a bunch of "unknown attribute" warnings. >> >> Add !defined(__clang__) to the minimum version check. >> >> Also, revive the version test blocks for versions >= 4.2.1 >> in order to disable features not supported by Clang. > > What is the minimum clang version required to compile the kernel? > What features are not supported by the minimum clang version? > > On my system, using clang > > $ clang -v > clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) > > and > > $ git checkout v4.16 ; make clean ; make CC=clang defconfig ; make CC=clang > HEAD is now at 0adb32858b0b... Linux 4.16 > > is successful > > but > > $ git checkout v4.17 ; make clean ; make CC=clang defconfig ; make CC=clang > HEAD is now at 29dcea88779c... Linux 4.17 > arch/x86/Makefile:184: *** Compiler lacks asm-goto support.. Stop. > arch/x86/Makefile:184: *** Compiler lacks asm-goto support.. Stop. > You cannot build x86 because asm-goto support is missing in clang. How about building arm or arm64? $ make ARCH=arm CC=clang CROSS_COMPILE=arm-linux-gnueabihf- defconfig all -- Best Regards Masahiro Yamada
Hi Joe, 2018-08-21 19:39 GMT+09:00 Joe Perches <joe@perches.com>: > On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: >> Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") >> missed the fact that <linux/compiler-gcc.h> is included by Clang >> as well as by GCC. >> >> Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ >> and it looks like GCC 4.2.1. >> >> $ scripts/gcc-version.sh -p clang >> 040201 > > Perhaps this would work, but I can't test it as > my clang version doesn't otherwise build a defconfig > and errors out with > > $ make CC=clang > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. > > --- > include/linux/compiler-gcc.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > index 3e70b7d4e9ed..3a06ad823fa4 100644 > --- a/include/linux/compiler-gcc.h > +++ b/include/linux/compiler-gcc.h > @@ -3,6 +3,23 @@ > #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." > #endif > > +/* > + * Override clang compiler version #defines > + * > + * compiler_types.h always #includes compiler-gcc.h before compiler-clang,h > + * but clang sets these __GNUC version #defines to 4.2.1. > + * This breaks the gcc minimum version of 4.6.0, so override the clang > + * definitions to 4.6.0 > + */ > +#ifdef __clang__ > + #undef __GNUC__ > + #undef __GNUC_MINOR__ > + #undef __GNUC_PATCHLEVEL__ > + #define __GNUC__ 4 > + #define __GNUC_MINOR__ 6 > + #define __GNUC_PATCHLEVEL__ 0 > +#endif > + > This patch does not work. It only addresses the "# error Sorry, your compiler is too old - please upgrade it." part. Attributes unsupported by Clang must be ifdef'ed out. Otherwise, you will see lots of unknown attribute warnings. -- Best Regards Masahiro Yamada
On Wed, 2018-08-22 at 02:13 +0900, Masahiro Yamada wrote: > Hi Joe, > > > 2018-08-21 19:39 GMT+09:00 Joe Perches <joe@perches.com>: > > On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: > > > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > > > missed the fact that <linux/compiler-gcc.h> is included by Clang > > > as well as by GCC. > > > > > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > > > and it looks like GCC 4.2.1. > > > > > > $ scripts/gcc-version.sh -p clang > > > 040201 > > > > Perhaps this would work, but I can't test it as > > my clang version doesn't otherwise build a defconfig > > and errors out with > > > > $ make CC=clang > > arch/x86/Makefile:179: *** Compiler lacks asm-goto support.. Stop. > > > > --- > > include/linux/compiler-gcc.h | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h > > index 3e70b7d4e9ed..3a06ad823fa4 100644 > > --- a/include/linux/compiler-gcc.h > > +++ b/include/linux/compiler-gcc.h > > @@ -3,6 +3,23 @@ > > #error "Please don't include <linux/compiler-gcc.h> directly, include <linux/compiler.h> instead." > > #endif > > > > +/* > > + * Override clang compiler version #defines > > + * > > + * compiler_types.h always #includes compiler-gcc.h before compiler-clang,h > > + * but clang sets these __GNUC version #defines to 4.2.1. > > + * This breaks the gcc minimum version of 4.6.0, so override the clang > > + * definitions to 4.6.0 > > + */ > > +#ifdef __clang__ > > + #undef __GNUC__ > > + #undef __GNUC_MINOR__ > > + #undef __GNUC_PATCHLEVEL__ > > + #define __GNUC__ 4 > > + #define __GNUC_MINOR__ 6 > > + #define __GNUC_PATCHLEVEL__ 0 > > +#endif > > + > > > > This patch does not work. > > It only addresses the > "# error Sorry, your compiler is too old - please upgrade it." part. > > > Attributes unsupported by Clang > must be ifdef'ed out. > > Otherwise, you will see lots of unknown attribute warnings. Then those attributes should be added or overridden in compiler-clang.h.
On Tue, 2018-08-21 at 09:57 -0700, Nick Desaulniers wrote: > On Tue, Aug 21, 2018 at 9:33 AM Joe Perches <joe@perches.com> wrote: > > > > On Tue, 2018-08-21 at 15:48 +0900, Masahiro Yamada wrote: > > > Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") > > > missed the fact that <linux/compiler-gcc.h> is included by Clang > > > as well as by GCC. > > > > > > Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ > > > and it looks like GCC 4.2.1. > > > > > > $ scripts/gcc-version.sh -p clang > > > 040201 > > > > > > If you try to build the kernel with Clang, you will get the > > > "Sorry, your compiler is too old - please upgrade it." > > > followed by a bunch of "unknown attribute" warnings. > > > > > > Add !defined(__clang__) to the minimum version check. > > > > > > Also, revive the version test blocks for versions >= 4.2.1 > > > in order to disable features not supported by Clang. > > > > What is the minimum clang version required to compile the kernel? > > Depends on the architecture and which kernel version/LTS branch you're > using. I'm trying to backport fixes to LTS branches, but sometimes a > compiler upgrade is required. I know that's not great, but I'm > actively trying to fix it. Can you enumerate these dependencies please. > > What features are not supported by the minimum clang version? And enumerate this too please. Uglifying the compiler-gcc file to support clang features that can be overridden in compiler-clang.h instead is not a good way to fix compilation for clang. But still, whatever features in the various clang versions that need accommodation also need listing out in compiler-clang.h, not in compiler-gcc.h > $ git describe --contains e501ce9 | sed 's/~.*//' > v4.17-rc1 Thanks, but I know this. The question remains, if clang can't compile v4.17, why does it immediately matter for v4.19? Why wouldn't overriding the clang __GNUC_<foo> #defines in compiler-gcc.h work acceptably with adding whatever is necessary to compiler-clang.h? I'll experiment with a compiler-clang.h patch.
Nick Desaulniers wrote on Tue, Aug 21, 2018: > On Tue, Aug 21, 2018 at 9:45 AM Joe Perches <joe@perches.com> wrote: > > > Tested with gcc-7 and clang-8. > > > > clang-8? Isn't the latest officlal clang 6.0.1 ? > [...] > > So if something other than 6.0.x is required, > > then some additional check should probably be > > added to compiler-clang.h as well. > > Sure, but that doesn't need to go in Mashiro's patch today. That can > wait for a proper separation between compiler headers where we can > then implement improved version checks. I can confirm that the patch here works for me with clang 6.0.1 as far as bpf programs are concerned (and gcc 8.1.1 for the kernel build itself on x86 while I was at it); so at least there is nothing on the compiler*.h headers that put off clang 6.0.1 (also replying to other subthread) From Joe Perches @ 2018-08-21 17:22 UTC: > The question remains, if clang can't compile > v4.17, why does it immediately matter for v4.19? I haven't had any problem with clang and 4.17 as far as building bpf programs is concerned, because the CC_HAVE_ASM_GOTO check is done in makefiles that aren't used in the bpf case and not in compiler-gcc.h or another header. So I guess the "immediately matters for v4.19" depends on how much you would care about bcc-compiled BPF programs. > Why wouldn't overriding the clang __GNUC_<foo> > #defines in compiler-gcc.h work acceptably with > adding whatever is necessary to compiler-clang.h? I think that could work, but at the point making a separate compiler-common.h and not including compiler-gcc.h for clang sounds better to me... More importantly here, either solution sound complex enough to require more than a few days and proper testing for all archs etc when compared to the partial revert we have here. -- Dominique Martinet
On Wed, 2018-08-22 at 06:16 +0200, Dominique Martinet wrote: > I think that could work, but at the point making a separate > compiler-common.h and not including compiler-gcc.h for clang sounds > better to me... More importantly here, either solution sound complex > enough to require more than a few days and proper testing for all archs > etc when compared to the partial revert we have here. The immediate need for a partial revert seems unnecessary as clang hasn't really worked for a couple releases now. The separate compiler file changes are much more sensible, even if it takes a few days.
Joe Perches wrote on Tue, Aug 21, 2018: > On Wed, 2018-08-22 at 06:16 +0200, Dominique Martinet wrote: > > I think that could work, but at the point making a separate > > compiler-common.h and not including compiler-gcc.h for clang sounds > > better to me... More importantly here, either solution sound complex > > enough to require more than a few days and proper testing for all archs > > etc when compared to the partial revert we have here. > > The immediate need for a partial revert seems unnecessary as > clang hasn't really worked for a couple releases now. Sorry for repeating myself, clang is used by bcc for compiling BPF programs (e.g. bpf_module_create_c_from_string() or any similar function will use the clang libs to compile the bpf program with linux headers), and this has always been working because it's not using our makefiles. This broke today in master and I only joined this thread after looking at why the build started failing today and noticing this patch, it definitely hasn't been broken for two releases, or I wouldn't be here with this timing :) > The separate compiler file changes are much more sensible, > even if it takes a few days. A few days are fine, but I think some form of fix in 4.19-rc1 would be good. I'll stop taking your time now, but I think you are/were underestimating how many people use clang with the linux kernel headers indirectly. BPF is a well-used tool :) Thanks, -- Dominique Martinet
On Tue, Aug 21, 2018 at 9:32 PM Dominique Martinet <asmadeus@codewreck.org> wrote: > > Joe Perches wrote on Tue, Aug 21, 2018: > > On Wed, 2018-08-22 at 06:16 +0200, Dominique Martinet wrote: > > > I think that could work, but at the point making a separate > > > compiler-common.h and not including compiler-gcc.h for clang sounds > > > better to me... More importantly here, either solution sound complex > > > enough to require more than a few days and proper testing for all archs > > > etc when compared to the partial revert we have here. > > > > The immediate need for a partial revert seems unnecessary as > > clang hasn't really worked for a couple releases now. > > Sorry for repeating myself, clang is used by bcc for compiling BPF > programs (e.g. bpf_module_create_c_from_string() or any similar function > will use the clang libs to compile the bpf program with linux headers), > and this has always been working because it's not using our makefiles. > > This broke today in master and I only joined this thread after looking > at why the build started failing today and noticing this patch, it > definitely hasn't been broken for two releases, or I wouldn't be here > with this timing :) > > > > The separate compiler file changes are much more sensible, > > even if it takes a few days. > > A few days are fine, but I think some form of fix in 4.19-rc1 would be > good. > > I'll stop taking your time now, but I think you are/were underestimating > how many people use clang with the linux kernel headers indirectly. > BPF is a well-used tool :) Hi Dominique, I'm currently testing a fix in https://github.com/ClangBuiltLinux/linux/commit/1f89ae7622c26b8131f42f3a362d6ef41b88a595, can you please share with me your steps to test/verify that the patch fixes the issue for eBPF? I'll go talk to a co-worker who know more about eBPF, but I've not yet done anything with it. Also, does anyone know who I should talk to about ICC testing? -- Thanks, ~Nick Desaulniers
On Wed, Aug 22, 2018 at 11:31 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > Hi Dominique, > I'm currently testing a fix in > https://github.com/ClangBuiltLinux/linux/commit/1f89ae7622c26b8131f42f3a362d6ef41b88a595, Sorry, maybe https://github.com/ClangBuiltLinux/linux/commits/compiler_detection is a better link, as the sha changes whenever I modify the patch and force push. -- Thanks, ~Nick Desaulniers
On Wed, 2018-08-22 at 11:31 -0700, Nick Desaulniers wrote: > On Tue, Aug 21, 2018 at 9:32 PM Dominique Martinet > <asmadeus@codewreck.org> wrote: > > > > Joe Perches wrote on Tue, Aug 21, 2018: > > > On Wed, 2018-08-22 at 06:16 +0200, Dominique Martinet wrote: > > > > I think that could work, but at the point making a separate > > > > compiler-common.h and not including compiler-gcc.h for clang sounds > > > > better to me... More importantly here, either solution sound complex > > > > enough to require more than a few days and proper testing for all archs > > > > etc when compared to the partial revert we have here. > > > > > > The immediate need for a partial revert seems unnecessary as > > > clang hasn't really worked for a couple releases now. > > > > Sorry for repeating myself, clang is used by bcc for compiling BPF > > programs (e.g. bpf_module_create_c_from_string() or any similar function > > will use the clang libs to compile the bpf program with linux headers), > > and this has always been working because it's not using our makefiles. > > > > This broke today in master and I only joined this thread after looking > > at why the build started failing today and noticing this patch, it > > definitely hasn't been broken for two releases, or I wouldn't be here > > with this timing :) > > > > > > > The separate compiler file changes are much more sensible, > > > even if it takes a few days. > > > > A few days are fine, but I think some form of fix in 4.19-rc1 would be > > good. > > > > I'll stop taking your time now, but I think you are/were underestimating > > how many people use clang with the linux kernel headers indirectly. > > BPF is a well-used tool :) > > Hi Dominique, > I'm currently testing a fix in > https://github.com/ClangBuiltLinux/linux/commit/1f89ae7622c26b8131f42f3a362d6ef41b88a595, > can you please share with me your steps to test/verify that the patch > fixes the issue for eBPF? I'll go talk to a co-worker who know more > about eBPF, but I've not yet done anything with it. A mild suggestion about the patch would be to break it up into 2 patches to improve how people read and review them. 1 include/linux/compiler-* 2 everything else Yes, some kernel configs might not build properly between 1 and 2 but that likely doesn't matter as those configs probably don't build before 1 either. Perhaps the test in arch/arm/kernel/asm-offsets.c for incompatible gcc compiler versions from 4.8.0 to 4.8.2 should be moved to compiler-gcc.h. > Also, does anyone know who I should talk to about ICC testing? No clue
On Wed, Aug 22, 2018 at 1:50 PM Joe Perches <joe@perches.com> wrote: > > On Wed, 2018-08-22 at 11:31 -0700, Nick Desaulniers wrote: > > On Tue, Aug 21, 2018 at 9:32 PM Dominique Martinet > > <asmadeus@codewreck.org> wrote: > > > > > > Joe Perches wrote on Tue, Aug 21, 2018: > > > > On Wed, 2018-08-22 at 06:16 +0200, Dominique Martinet wrote: > > > > > I think that could work, but at the point making a separate > > > > > compiler-common.h and not including compiler-gcc.h for clang sounds > > > > > better to me... More importantly here, either solution sound complex > > > > > enough to require more than a few days and proper testing for all archs > > > > > etc when compared to the partial revert we have here. > > > > > > > > The immediate need for a partial revert seems unnecessary as > > > > clang hasn't really worked for a couple releases now. > > > > > > Sorry for repeating myself, clang is used by bcc for compiling BPF > > > programs (e.g. bpf_module_create_c_from_string() or any similar function > > > will use the clang libs to compile the bpf program with linux headers), > > > and this has always been working because it's not using our makefiles. > > > > > > This broke today in master and I only joined this thread after looking > > > at why the build started failing today and noticing this patch, it > > > definitely hasn't been broken for two releases, or I wouldn't be here > > > with this timing :) > > > > > > > > > > The separate compiler file changes are much more sensible, > > > > even if it takes a few days. > > > > > > A few days are fine, but I think some form of fix in 4.19-rc1 would be > > > good. > > > > > > I'll stop taking your time now, but I think you are/were underestimating > > > how many people use clang with the linux kernel headers indirectly. > > > BPF is a well-used tool :) > > > > Hi Dominique, > > I'm currently testing a fix in > > https://github.com/ClangBuiltLinux/linux/commit/1f89ae7622c26b8131f42f3a362d6ef41b88a595, > > can you please share with me your steps to test/verify that the patch > > fixes the issue for eBPF? I'll go talk to a co-worker who know more > > about eBPF, but I've not yet done anything with it. > > A mild suggestion about the patch would be to break it up into > 2 patches to improve how people read and review them. > > 1 include/linux/compiler-* > 2 everything else > > Yes, some kernel configs might not build properly between 1 and 2 > but that likely doesn't matter as those configs probably don't > build before 1 either. If we ordered the patches so that the "everything else" went in first, it would not be a problem. The first patch would just be the checks that GCC_VERSION is defined. In general, I'm happy to split patches, but in this suggested case, it only shaves off 26 lines from the main body of work. I don't think 26 lines is enough to justify splitting for readability. But let me know if you feel strongly about that point and I'll be happy to split. (or possibly by "everything else" you meant something else?) > Perhaps the test in arch/arm/kernel/asm-offsets.c for incompatible > gcc compiler versions from 4.8.0 to 4.8.2 should be moved to > compiler-gcc.h. This a good suggestion, and one I've thought about, although in the context of builtins. *Detour ahead*. Builtins are not portable by default, and their use really should be feature detected (impossible currently on gcc due to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970) or version checks and protected by macros like the symbols in my current patch. I think I might soon hoist them to a shared header that safely detects their support or provides a fallback when possible. One issue is that some builtins are arch specific, so do you want to feature detect arch specific ones for builds of a different arch? And that's the problem I have with hoisting arch specific compiler checks into a shared header; builds of other archs should pay no build penalty for unrelated archs. All that to say that I think it's best to keep arch specific checks in arch/ specific subdirs, but I welcome more thoughts on this. Ok, I've boot tested this patch for x86_64 and arm64 in qemu, with CC=gcc-7.2, CC=clang-8, and CC=clang-8 HOSTCC=clang-8 and am feeling quite confident to send it for review. >> Also, does anyone know who I should talk to about ICC testing? > No clue + Daniel and HPA (the last commits to include/linux/compiler* mentioning `icc` in 2015 and 2013 respectively) -- Thanks, ~Nick Desaulniers
On Wed, 2018-08-22 at 16:05 -0700, Nick Desaulniers wrote: Hey Nick. > On Wed, Aug 22, 2018 at 1:50 PM Joe Perches <joe@perches.com> wrote: > > A mild suggestion about the patch would be to break it up into > > 2 patches to improve how people read and review them. > > > > 1 include/linux/compiler-* > > 2 everything else > > > > Yes, some kernel configs might not build properly between 1 and 2 > > but that likely doesn't matter as those configs probably don't > > build before 1 either. > > If we ordered the patches so that the "everything else" went in first, > it would not be a problem. The first patch would just be the checks > that GCC_VERSION is defined. > > In general, I'm happy to split patches, but in this suggested case, it > only shaves off 26 lines from the main body of work. No worries, I rarely care _that_ much about code, but seeing the subject with compiler-gcc and the first part of the patch about arch/ was a bit off-putting. You're the one doing the work here. Do what you think best.
Nick Desaulniers wrote on Wed, Aug 22, 2018: > I'm currently testing a fix in > https://github.com/ClangBuiltLinux/linux/commit/1f89ae7622c26b8131f42f3a362d6ef41b88a595, > can you please share with me your steps to test/verify that the patch > fixes the issue for eBPF? I'll go talk to a co-worker who know more > about eBPF, but I've not yet done anything with it. Thanks for the link. The simplest way to test with bcc for me would probably be to fetch the example from their repo itself[1], whether you install bcc through your distro or compile it yourself. There are dozens of example programs in the tools/ directory of which you can just run any, if will try to compile the program embedded wihtin the example at runtime with clang. [1] https://github.com/iovisor/bcc (I just noticed fedora provides a bcc-tools package which provides these tools directly, so if you're lucky you can just install that and run examples in /usr/share/bcc/tools) In particular I get a couple of errors with your patch, I think it boils down to this one: ----- In file included from <built-in>: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: In file included from /lib/modules/4.18.0+/build/arch/x86/include/asm/bitops.h:16: /lib/modules/4.18.0+/build/include/linux/compiler.h:217:3: 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")) ^ ----- And removing the parenthesis around the expression seems to work, they weren't here in the compiler-gcc.h file in the previous version? There might be other defines the simple examples I ran do not use but from a quick glance it doesn't look like it, thank you for the split work! -- Dominique Martinet
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 250b9b7..8e41fd2 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -10,7 +10,7 @@ + __GNUC_MINOR__ * 100 \ + __GNUC_PATCHLEVEL__) -#if GCC_VERSION < 40600 +#if !defined(__clang__) && GCC_VERSION < 40600 # error Sorry, your compiler is too old - please upgrade it. #endif @@ -163,7 +163,16 @@ #define __used __attribute__((__used__)) #define __compiler_offsetof(a, b) \ __builtin_offsetof(a, b) +#define __compiletime_object_size(obj) __builtin_object_size(obj, 0) +/* + * GCC version specific checks + * + * Keep the version checks for 4.2.1 or newer + * because Clang defines GCC_VERSION as 40201 + */ + +#if GCC_VERSION >= 40300 /* Mark functions as cold. gcc will assume any path leading to a call * to them will be unlikely. This means a lot of manual unlikely()s * are unnecessary now for any paths leading to the usual suspects @@ -182,15 +191,20 @@ #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__) +#ifndef __CHECKER__ +# define __compiletime_warning(message) __attribute__((warning(message))) +# define __compiletime_error(message) __attribute__((error(message))) +#endif /* __CHECKER__ */ +#endif /* GCC_VERSION >= 40300 */ + +#if GCC_VERSION >= 40400 #define __optimize(level) __attribute__((__optimize__(level))) #define __nostackprotector __optimize("no-stack-protector") +#endif /* GCC_VERSION >= 40400 */ -#define __compiletime_object_size(obj) __builtin_object_size(obj, 0) +#if GCC_VERSION >= 40500 #ifndef __CHECKER__ -#define __compiletime_warning(message) __attribute__((warning(message))) -#define __compiletime_error(message) __attribute__((error(message))) - #ifdef LATENT_ENTROPY_PLUGIN #define __latent_entropy __attribute__((latent_entropy)) #endif @@ -232,6 +246,9 @@ #define randomized_struct_fields_end } __randomize_layout; #endif +#endif /* GCC_VERSION >= 40500 */ + +#if GCC_VERSION >= 40600 /* * When used with Link Time Optimization, gcc can optimize away C functions or * variables which are referenced only from assembly code. __visible tells the @@ -240,7 +257,7 @@ */ #define __visible __attribute__((externally_visible)) -/* gcc version specific checks */ +#endif /* GCC_VERSION >= 40600 */ #if GCC_VERSION >= 40900 && !defined(__CHECKER__) /* @@ -274,8 +291,10 @@ * folding in __builtin_bswap*() (yet), so don't set these for it. */ #if defined(CONFIG_ARCH_USE_BUILTIN_BSWAP) && !defined(__CHECKER__) +#if GCC_VERSION >= 40400 #define __HAVE_BUILTIN_BSWAP32__ #define __HAVE_BUILTIN_BSWAP64__ +#endif #if GCC_VERSION >= 40800 #define __HAVE_BUILTIN_BSWAP16__ #endif @@ -327,9 +346,12 @@ #define __diag_GCC_warn warning #define __diag_GCC_error error +/* Compilers before gcc-4.6 do not understand "#pragma GCC diagnostic push" */ +#if GCC_VERSION >= 40600 #define __diag_str1(s) #s #define __diag_str(s) __diag_str1(s) #define __diag(s) _Pragma(__diag_str(GCC diagnostic s)) +#endif #if GCC_VERSION >= 80000 #define __diag_GCC_8(s) __diag(s)
Commit cafa0010cd51 ("Raise the minimum required gcc version to 4.6") missed the fact that <linux/compiler-gcc.h> is included by Clang as well as by GCC. Clang actually defines __GNUC__, __GNUC_MINOR__, __GNUC_PATCHLEVEL__ and it looks like GCC 4.2.1. $ scripts/gcc-version.sh -p clang 040201 If you try to build the kernel with Clang, you will get the "Sorry, your compiler is too old - please upgrade it." followed by a bunch of "unknown attribute" warnings. Add !defined(__clang__) to the minimum version check. Also, revive the version test blocks for versions >= 4.2.1 in order to disable features not supported by Clang. Fixes: cafa0010cd51 ("Raise the minimum required gcc version to 4.6") Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- include/linux/compiler-gcc.h | 34 ++++++++++++++++++++++++++++------ 1 file changed, 28 insertions(+), 6 deletions(-) -- 2.7.4