[PATCH] Restore gcc check in mips asm/unroll.h

[PATCH] Restore gcc check in mips asm/unroll.h

[Cesar Eduardo Barros]

While raising the gcc version requirement to 4.9, the compile-time check
in the unroll macro was accidentally changed from being used on gcc and
clang to being used on clang only.

Restore the gcc check, changing it from "gcc >= 4.7" to "all gcc".

Fixes: 6ec4476ac825 ("Raise gcc version requirement to 4.9")
Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.eti.br>
---
 arch/mips/include/asm/unroll.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/mips/include/asm/unroll.h b/arch/mips/include/asm/unroll.h
index 8ed660adc84f..49009319ac2c 100644
--- a/arch/mips/include/asm/unroll.h
+++ b/arch/mips/include/asm/unroll.h
@@ -25,7 +25,8 @@
 	 * generate reasonable code for the switch statement,	\
 	 * so we skip the sanity check for those compilers.	\
 	 */							\
-	BUILD_BUG_ON((CONFIG_CLANG_VERSION >= 80000) &&		\
+	BUILD_BUG_ON((CONFIG_CC_IS_GCC ||			\
+		      CONFIG_CLANG_VERSION >= 80000) &&		\
 		     !__builtin_constant_p(times));		\
 								\
 	switch (times) {					\
-- 
2.26.2

Re: [PATCH] Restore gcc check in mips asm/unroll.h

[Linus Torvalds]

On Thu, Jul 9, 2020 at 3:11 PM Cesar Eduardo Barros
<cesarb@cesarb.eti.br> wrote:
>
> While raising the gcc version requirement to 4.9, the compile-time check
> in the unroll macro was accidentally changed from being used on gcc and
> clang to being used on clang only.
>
> Restore the gcc check, changing it from "gcc >= 4.7" to "all gcc".

This is clearly a thinko on my side: the old

    CONFIG_GCC_VERSION >= 40700

became pointless, so I removed, it, but it was mixed with an "||" so
we actually wanted to make it unconditional on gcc, and instead now it
checks for CLANG version even when it shouldn't.

My bad, and your patch is obviously correct.

At the same time, I do wonder if  we could just remove the check for
CLANG_VERSION >= 80000 too, and just remove all the compiler check
hackery entirely?

Older versions of clang weren't very good at compiling the Linux
kernel in the first place. Do people actually use old clang versions?
That sounds like a really bad idea.

People who used to build the kernel with clang tended to actually get
their clang version from the clang git sources afaik (I still do, but
that's because I test experimental new features that aren't released
yet), exactly because back in the bad old days there were so many
problems.

These days you can use release versions, but they'd presumably not be
older than clang-8.

Adding Nick - is it really reasonable to build any kernel with any
clang version before 8.0.0?

It turns out that the arm side also has a check for clang < 8 because
of -mcmodel=tiny, so raising the minimum required clang version to
that would solve that issue too.

Right now we don't mention minimum clang/llvm versions in our docs at
all, because we only mention gcc. Mayeb this would be good to clarify.

               Linus

Re: [PATCH] Restore gcc check in mips asm/unroll.h

[Nick Desaulniers]

On Thu, Jul 9, 2020 at 5:53 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Jul 9, 2020 at 3:11 PM Cesar Eduardo Barros
> <cesarb@cesarb.eti.br> wrote:
> >
> > While raising the gcc version requirement to 4.9, the compile-time check
> > in the unroll macro was accidentally changed from being used on gcc and
> > clang to being used on clang only.
> >
> > Restore the gcc check, changing it from "gcc >= 4.7" to "all gcc".
>
> This is clearly a thinko on my side: the old
>
>     CONFIG_GCC_VERSION >= 40700
>
> became pointless, so I removed, it, but it was mixed with an "||" so
> we actually wanted to make it unconditional on gcc, and instead now it
> checks for CLANG version even when it shouldn't.
>
> My bad, and your patch is obviously correct.
>
> At the same time, I do wonder if  we could just remove the check for
> CLANG_VERSION >= 80000 too, and just remove all the compiler check
> hackery entirely?

What I'd really like to see as a policy in the kernel going forward in
that ANY new commit that adds some hack or workaround for a specific
compiler version add a comment about which toolchain version was
problematic, that way when we drop support for that version years
later, we can drop whatever hacks and technical debt we've accumulated
to support that older version.  I'd prefer a comment that can be
`grep`ed rather than a commit message that may need to be searched.
That way when bump the compiler version we can grep for comparisons
against that version and start cleaning up code.

The case that comes to mind:
commit 87e0d4f0f37f ("kbuild: disable clang's default use of
-fmerge-all-constants")
cites https://bugs.llvm.org/show_bug.cgi?id=18538
The fix for which shipped shortly after reported in clang-6.
https://github.com/ClangBuiltLinux/linux/issues/9
Looking at the dates between kernel patch and toolchain patch, I guess
that the kernel patch authors didn't know what release that workaround
would be fixed in, but basically they need it for CLANG_VERSION <=
600001.
We can remove that entirely if we commit to a minimally supported
version of clang.

Also, I'm a little salty still about our conversation with
asm_volatile_goto:
https://lore.kernel.org/lkml/20180907222109.163802-1-ndesaulniers@google.com/
I think lore is missing your response, that triggered a v2:
https://lore.kernel.org/lkml/20181031182909.169342-1-ndesaulniers@google.com/
(Strange, I literally cannot find evidence that you ever responded to
that...am I going crazy? Looks like my work email has been being
auto-deleted because...lawyers) (lkml.org is missing it:
https://lkml.org/lkml/2018/9/7/1628, and I didn't have our mailing
list set up then).
You also know how I feel about empty asm statements (`asm("");`). ;)

Anyways, the point is "please tag these workarounds with a toolchain
version somehow such that someday we may pay off our debts."

>
> Older versions of clang weren't very good at compiling the Linux
> kernel in the first place. Do people actually use old clang versions?
> That sounds like a really bad idea.
>
> People who used to build the kernel with clang tended to actually get
> their clang version from the clang git sources afaik (I still do, but
> that's because I test experimental new features that aren't released
> yet), exactly because back in the bad old days there were so many

Thanks for the endorsement. :P

What's the latest on that `clac` in exception handlers discussion?

> problems.
>
> These days you can use release versions, but they'd presumably not be
> older than clang-8.
>
> Adding Nick - is it really reasonable to build any kernel with any
> clang version before 8.0.0?

TL;DR: probably not.

For Pixel 2, we shipped a Clang built kernel using clang-4.  Since
then I've moved it to be using near ToT Clang (clang-11).  That device
was aarch64 with no hard dependency on `asm goto` (only x86 added
that, and only in 4.20, so not really an issue for stable kernel
branches older than that).  `asm goto` support shipped in clang-9.

>
> It turns out that the arm side also has a check for clang < 8 because
> of -mcmodel=tiny, so raising the minimum required clang version to
> that would solve that issue too.
>
> Right now we don't mention minimum clang/llvm versions in our docs at
> all, because we only mention gcc. Mayeb this would be good to clarify.

Yeah, I think so, too.

Are you planning on attending plumbers? I'm planning a session on
talking about this more, which I think would be helpful.  What I
really want is the CI people in the room, because I don't want to
claim version X+ is supported if we don't have the CI coverage of it.
Also, there's a few footnotes as to our compatibility table at the
moment; completely missing backends, broken backends, targets we only
got working recently, etc. etc. etc..  I also think we need to be
delicate in the wording for what tools are required for building a
kernel vs optional or substitutes.

Will older versions of clang work? It's highly likely and we don't
have a list of what if anything is actually broken with them.  But if
we can get CI coverage for the latest release or two, I'm happy to
commit to supporting just those.
--
Thanks,
~Nick Desaulniers

Re: [PATCH] Restore gcc check in mips asm/unroll.h

[Linus Torvalds]

On Fri, Jul 10, 2020 at 11:43 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> What I'd really like to see as a policy in the kernel going forward in
> that ANY new commit that adds some hack or workaround for a specific
> compiler version add a comment about which toolchain version was
> problematic, that way when we drop support for that version years
> later, we can drop whatever hacks and technical debt we've accumulated
> to support that older version.

The problem is that at the time we find and fix things, it's often
_very_ unclear which compiler versions are affected.

We also have the situation that a lot of distro compilers aren't
necessarily completely "clean" versions, particularly for the
"enterprise" ones that get stuck on some old version and then fix up
their breakage by backporting fixes.

When it's some particular version of a compiler that supports a
particular feature, that tends to be much more straightforward. But
we've had bugs where it was very unclear when exactly the bug was
fixed (fi it was fixed at all by the time we do the workaround).

              Linus

Re: [PATCH] Restore gcc check in mips asm/unroll.h

[Nathan Chancellor]

On Fri, Jul 10, 2020 at 03:31:00PM -0700, Linus Torvalds wrote:
> On Fri, Jul 10, 2020 at 11:43 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > What I'd really like to see as a policy in the kernel going forward in
> > that ANY new commit that adds some hack or workaround for a specific
> > compiler version add a comment about which toolchain version was
> > problematic, that way when we drop support for that version years
> > later, we can drop whatever hacks and technical debt we've accumulated
> > to support that older version.
> 
> The problem is that at the time we find and fix things, it's often
> _very_ unclear which compiler versions are affected.
> 
> We also have the situation that a lot of distro compilers aren't
> necessarily completely "clean" versions, particularly for the
> "enterprise" ones that get stuck on some old version and then fix up
> their breakage by backporting fixes.

Indeed. I would say this is less common for most distributions with
clang, where they tend to stick closer to tip of tree, but it can still
happen. I guess there is not a really good solution for this but we
could just have a policy that as soon as you move away from the upstream
version, you are on your own.

> When it's some particular version of a compiler that supports a
> particular feature, that tends to be much more straightforward. But
> we've had bugs where it was very unclear when exactly the bug was
> fixed (fi it was fixed at all by the time we do the workaround).

As for putting a seal of approval on a minimum supported version of
LLVM/clang, I have my reservations. 0day keeps uncovering various issues
with its builds and clang's release model is different than from GCC's
so if we ever come across a compiler bug in an older version of clang,
we have basically no hope for getting it fixed. GCC supports older
series through bug fix releases for quite some time (GCC 7 was supported
for two and a half years), whereas with clang, they only see one
servicing release before the next major release (for example, clang
9.0.1 before clang 10.0.0) so it makes getting compiler fixes into the
hands of users much more difficult. I am trying to rectify that with
clang 10 though, where I have been testing that release against a bunch
of different configs both in tree and out of tree:
https://github.com/nathanchance/llvm-kernel-testing

However, I think at this point, we can say clang itself is in a good
position as of clang 9, certainly clang 10. I am less confident in
placing a minimum version on the LLVM tools such as lld though. For arm,
arm64, and x86_64, we are in fairly good shape as of clang 10 but I
think there is probably some more work/polishing to be done there; for
other architectures, it is worse. I suppose we would have to consider
the support model: under what cases is it acceptable to bump the minimum
required version versus inserting a bad compiler hack? As someone who is
not super familiar with the relationship between GCC and the kernel, it
appears to me that the general attitude towards compiler bugs has been
workaround it in the kernel while hoping that it gets fixed at some
point in GCC. We have been pretty aggressive about fixing the compiler
instead of inserting a workaround, which I feel like is the better
solution, but it makes supporting multiple versions of the compiler more
difficult (versus just saying use the latest). It is something that
needs to be discussed and agreed upon sooner rather than later though,
especially as we grow more and more polished.

There were some other thoughts that I had on our issue tracker here, if
anyone cares for them:

https://github.com/ClangBuiltLinux/linux/issues/941

Sorry for the brain dump and cheers,
Nathan

[PATCH] mips: Remove compiler check in unroll macro

[Nathan Chancellor]

CONFIG_CC_IS_GCC is undefined when Clang is used, which breaks the build
(see our Travis link below).

Clang 8 was chosen as a minimum version for this check because there
were some improvements around __builtin_constant_p in that release. In
reality, MIPS was not even buildable until clang 9 so that check was not
technically necessary. Just remove all compiler checks and just assume
that we have a working compiler.

Fixes: d4e60453266b ("Restore gcc check in mips asm/unroll.h")
Link: https://travis-ci.com/github/ClangBuiltLinux/continuous-integration/jobs/359642821
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 arch/mips/include/asm/unroll.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/mips/include/asm/unroll.h b/arch/mips/include/asm/unroll.h
index 49009319ac2c..7dd4a80e05d6 100644
--- a/arch/mips/include/asm/unroll.h
+++ b/arch/mips/include/asm/unroll.h
@@ -25,9 +25,7 @@
 	 * generate reasonable code for the switch statement,	\
 	 * so we skip the sanity check for those compilers.	\
 	 */							\
-	BUILD_BUG_ON((CONFIG_CC_IS_GCC ||			\
-		      CONFIG_CLANG_VERSION >= 80000) &&		\
-		     !__builtin_constant_p(times));		\
+	BUILD_BUG_ON(!__builtin_constant_p(times));		\
 								\
 	switch (times) {					\
 	case 32: fn(__VA_ARGS__); /* fall through */		\

base-commit: aa0c9086b40c17a7ad94425b3b70dd1fdd7497bf
-- 
2.27.0

Re: [PATCH] mips: Remove compiler check in unroll macro

[Linus Torvalds]

On Fri, Jul 10, 2020 at 3:34 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang 8 was chosen as a minimum version for this check because there
> were some improvements around __builtin_constant_p in that release. In
> reality, MIPS was not even buildable until clang 9 so that check was not
> technically necessary. Just remove all compiler checks and just assume
> that we have a working compiler.

Thanks, that looks much nicer.

Applied.

I think we could probably remove the (unrelated) clang-8 check in the
arm side too, but I guess I'll let arm/clang people worry about it.

            Linus

Re: [PATCH] mips: Remove compiler check in unroll macro

[Nathan Chancellor]

On Fri, Jul 10, 2020 at 03:43:43PM -0700, Linus Torvalds wrote:
> On Fri, Jul 10, 2020 at 3:34 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Clang 8 was chosen as a minimum version for this check because there
> > were some improvements around __builtin_constant_p in that release. In
> > reality, MIPS was not even buildable until clang 9 so that check was not
> > technically necessary. Just remove all compiler checks and just assume
> > that we have a working compiler.
> 
> Thanks, that looks much nicer.
> 
> Applied.
> 
> I think we could probably remove the (unrelated) clang-8 check in the
> arm side too, but I guess I'll let arm/clang people worry about it.
> 
>             Linus

Yes, we probably should. I'll comment more on that in the other thread.

Thanks for picking up the patch quickly!

Cheers,
Nathan