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

[Nick Desaulniers]

On Thu, Jul 9, 2020 at 5:53 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Jul 9, 2020 at 3:11 PM Cesar Eduardo Barros
> <cesarb@xxxxxxxxxxxxx> 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@xxxxxxxxxx/
I think lore is missing your response, that triggered a v2:
https://lore.kernel.org/lkml/20181031182909.169342-1-ndesaulniers@xxxxxxxxxx/
(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