Date:   Thu, 24 Feb 2022 16:11:19 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, joao@...rdrivepizza.com, hjl.tools@...il.com,
        jpoimboe@...hat.com, andrew.cooper3@...rix.com,
        linux-kernel@...r.kernel.org, ndesaulniers@...gle.com,
        samitolvanen@...gle.com, mark.rutland@....com,
        alyssa.milburn@...el.com, mbenes@...e.cz, rostedt@...dmis.org,
        mhiramat@...nel.org, alexei.starovoitov@...il.com,
        Nathan Chancellor <nathan@...nel.org>, llvm@...ts.linux.dev
Subject: Re: [PATCH v2 01/39] kbuild: Fix clang build

nit: Subject maybe could be something more specific like:
"kbuild: Detect and apply clang version suffix to defaults"

On Thu, Feb 24, 2022 at 03:51:39PM +0100, Peter Zijlstra wrote:
> Debian (and derived) distros ship their compilers as -$ver suffixed
> binaries. For gcc it is sufficent to use:
> 
>  $ make CC=gcc-12
> 
> However, clang builds (esp. clang-lto) need a whole array of tools to be
> exactly right, leading to unweildy stuff like:

Yeah, I have had this problem with gcc versions too (when trying to
select older compilers when cross compiling).

> [...]
> which is, quite franktly, totally insane and unusable. Instead make
> the CC variable DTRT, enabling one such as myself to use:
> 
>  $ make CC=clang-13

This form is intended to mix clang and binutils, as there is a long tail
of (usually architecture-specific) issues with ld.lld and Clang's
assembler. It's only relatively recently that clang + ld.lld works
happily on x86_64. Growing a way to split that logic by architecture
might be interesting, but also confusing...

> This also lets one quickly test different clang versions.
> Additionally, also support path based LLVM suites like:
> 
>  $ make CC=/opt/llvm/bin/clang
> 
> This changes the default to LLVM=1 when CC is clang, mixing toolchains
> is still possible by explicitly adding LLVM=0.

I do like the path idea -- this is much cleaner in Clang's case than the
gcc/binutils mixture where a versioned binutils might not even exist in
a given environment. I've been fighting versioned cross compilers with
gcc and ld.bfd. It's almost worse. ;)

> [...]
> --- a/Makefile
> +++ b/Makefile
> @@ -423,9 +423,29 @@ HOST_LFS_CFLAGS := $(shell getconf LFS_C
>  HOST_LFS_LDFLAGS := $(shell getconf LFS_LDFLAGS 2>/dev/null)
>  HOST_LFS_LIBS := $(shell getconf LFS_LIBS 2>/dev/null)
>  
> -ifneq ($(LLVM),)
> -HOSTCC	= clang
> -HOSTCXX	= clang++
> +# powerpc and s390 don't yet work with LLVM as a whole

Er, this, uh, doesn't really capture the matrix. ;) See
https://clangbuiltlinux.github.io/

> +ifeq ($(ARCH),powerpc)
> +LLVM = 0
> +endif
> +ifeq ($(ARCH),s390)
> +LLVM = 0
> +endif
> +
> +# otherwise, if CC=clang, default to using LLVM to enable LTO

I find this comment confusing: using LLVM=1 lets LTO be possible,
strictly speaking, it doesn't enable it.

> +CC_BASE := $(shell echo $(CC) | sed 's/.*\///')

I would expect $(shell basename $(CC)) (or similarly "dirname") for
these sorts of path manipulations, but I'll bet there's some Makefile
string syntax to do this too, to avoid $(shell) entirely...

> +CC_NAME := $(shell echo $(CC_BASE) | cut -b "1-5")

O_o  cut -d- -f1

> +ifeq ($(shell test "$(CC_NAME)" = "clang"; echo $$?),0)
> +LLVM ?= 1
> +LLVM_PFX := $(shell echo $(CC) | sed 's/\(.*\/\)\?.*/\1/')

dirname

> +LLVM_SFX := $(shell echo $(CC_BASE) | cut -b "6-")

cut -d- -f2-

> +endif

This versioned suffix logic I'm fine with, though I'd prefer we gain
this for both Clang and GCC, as I've needed it there too, specifically
with the handling of CROSS_COMPILE:

$ make CROSS_COMPILE=/path/to/abi- CC=/path/to/abi-gcc-10 LD=/path/to/abi-ld-binutilsversion

Anyway, I guess that could be a separate patch.

> +# if not set by now, do not use LLVM
> +LLVM ?= 0

I think, however, the LLVM state needs to stay default=0. The "how to
build with Clang" already details the "how" on this:
https://www.kernel.org/doc/html/latest/kbuild/llvm.html

But I do agree: extracting the version suffix would make things much
easier.

Regardless, even if LLVM=1 is made the default, I think that should be
separate from the path and suffix logic.

Also, please CC the "CLANG/LLVM BUILD SUPPORT" maintainers in the
future:
M:      Nathan Chancellor <nathan@...nel.org>
M:      Nick Desaulniers <ndesaulniers@...gle.com>
L:      llvm@...ts.linux.dev
S:      Supported
W:      https://clangbuiltlinux.github.io/
B:      https://github.com/ClangBuiltLinux/linux/issues
C:      irc://irc.libera.chat/clangbuiltlinux
F:      Documentation/kbuild/llvm.rst
F:      include/linux/compiler-clang.h
F:      scripts/Makefile.clang
F:      scripts/clang-tools/
K:      \b(?i:clang|llvm)\b

-- 
Kees Cook

Powered by blists - more mailing lists