Messages in this thread | | | From | Nick Desaulniers <> | Date | Tue, 25 Jun 2019 15:57:04 -0700 | Subject | Re: [PATCH] perf/x86/intel: Mark expected switch fall-throughs |
| |
On Tue, Jun 25, 2019 at 3:29 PM Joe Perches <joe@perches.com> wrote: > > On Tue, 2019-06-25 at 11:15 -0700, Nick Desaulniers wrote: > > Unreleased versions of Clang built from source can; the latest release > > of Clang-8 doesn't have asm goto support required for > > CONFIG_JUMP_LABEL. Things can get complicated based on which kernel > > tree/branch (mainline, -next, stable), arch, and configs, but I think > > we just have a few long tail bugs left. > > At some point, when clang generically compiles the kernel, > I believe it'd be good to remove the various bits that > are unusual like CONFIG_CC_HAS_ASM_GOTO in Makefile > and the scripts/clang-version.sh and the like. > > This could help when compiling a specific .config on > different systems. > > Maybe add the equivalent compiler-gcc.h #define below > even before the removals > > (whatever minor/patchlevel appropriate) > --- > include/linux/compiler-clang.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h > index 333a6695a918..b46aece0f9ca 100644 > --- a/include/linux/compiler-clang.h > +++ b/include/linux/compiler-clang.h > @@ -5,6 +5,14 @@ > > /* Compiler specific definitions for Clang compiler */ > > +#define CLANG_VERSION (__clang_major__ * 10000 \ > + + __clang_minor__ * 100 \ > + + __clang_patchlevel__) > + > +#if CLANG_VERSION < 90000 > +# error Sorry, your compiler is too old - please upgrade it. > +#endif > +
Heh, I've definitely thought about clang version checks. And I agree with your implementation which matches the gcc 4.6 check in include/linux/compiler-gcc.h.
There are cases when it's appropriate, such as when a certain language feature has no way of feature detection. Let's take for example 2 different GNU C extensions.
On one hand, consider __GCC_ASM_FLAG_OUTPUTS__ (https://developers.redhat.com/blog/2016/02/25/new-asm-flags-feature-for-x86-in-gcc-6/). I like the design of __GCC_ASM_FLAG_OUTPUTS__ because it's straightforward to do feature detection via preprocessor checks. This makes it easy to use the feature when the compiler supports it, or provide another implementation, regardless of compiler, compiler version preprocessor defines, etc. It's compiler agnostic; if the feature is there then use it or provide a fallback (or error). I think the code in include/linux/compiler_attributes.h also exemplifies this mindset. (I hope https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66970 can be implemented soon, too, towards this goal of more portable C code).
On the other hand, consider most other GNU C extensions. How do I test whether they exist in my compiler or not? Is it everything or nothing (do they all have to exist?). In those cases you either end up shelling out to something like autoconf (which is what I consider the current infra around CONFIG_CC_HAS_ASM_GOTO), or code filled with tons of version checks for specific compilers which are brittle.
Of the two cases, now consider what happens when my compiler that previously did not support a particular feature now does. In the first case, the guards were compiler agnostic, and I *don't have to change the source* to make use of the feature in the new compiler. In the second case, I *need to modify the source* to update the version checks to be correct.
That's why I consider version checks to be brittle. Just to hammer this point away a little more, consider this code in glib for asm goto detection: https://github.com/GNOME/glib/blob/cbfa776fc149fcc3e351fbdf68c1a299519f4905/glib/gbitlock.c#L182. This version check literally will not work for clang-9, though it does support asm goto. Unfortunately, asm goto doesn't have nice preprocessor defines like __GCC_ASM_FLAG_OUTPUTS__ does, so someone literally *needs to edit the source* of glib to make it take advantage of asm goto in clang-9+ (even though clang-9+ supports the feature in question). Feature detection and its benefits over version detection are well understood in the web development community where devs there have to worry about implementations from different vendors.
Back to your point about adding a minimal version of Clang to the kernel; I don't really want to do this. For non-x86 architectures, people are happily compiling their kernels with versions of clang as old as clang-4. And if it continues to work for them; I'm happy. And if it doesn't, and they raise an alarm, we're happy to take a look. Old LTS distros may have ancient builds of clang, so maybe some kind of hint would be nice, but I'd also like to support older versions where we can and I think choosing clang-9 for x86's sake is too x86-centric. A version check on CONFIG_JUMP_LABEL is maybe more appropriate, so it cannot be selected if you're using clang && your version of clang is not clang-9 or greater? -- Thanks, ~Nick Desaulniers
| |