Message-ID: <CAKwvOdnzrMOCo4RRsfcR=K5ELWU8obgMqtOGZnx_avLrArjpRQ@mail.gmail.com>
Date:   Tue, 25 Jun 2019 15:57:04 -0700
From:   Nick Desaulniers <ndesaulniers@...gle.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Kan Liang <kan.liang@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        Shawn Landden <shawn@....icu>,
        Nathan Chancellor <natechancellor@...il.com>,
        Luc Van Oostenryck <luc.vanoostenryck@...il.com>,
        Chandler Carruth <chandlerc@...gle.com>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH] perf/x86/intel: Mark expected switch fall-throughs

On Tue, Jun 25, 2019 at 3:29 PM Joe Perches <joe@...ches.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

Powered by blists - more mailing lists