Re: [PATCH v2 01/39] kbuild: Fix clang build

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Kees Cook]

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@kernel.org>
M:      Nick Desaulniers <ndesaulniers@google.com>
L:      llvm@lists.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

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Nick Desaulniers]

On Thu, Feb 24, 2022 at 7:17 AM Peter Zijlstra <peterz@infradead.org> 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:
>
>  $ make CC=clang-13 LD=ld.lld-13 AR=llvm-ar-13 NM=llvm-nm-13 OBJCOPY=llvm-objcopy-13 OBJDUMP=llvm-objdump-13 READELF=llvm-readelf-13 STRIP=llvm-strip-13 LLVM=1
>
> 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 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

No, nack, we definitely do not want CC=clang to set LLVM=1. Those are
distinctly two different things for testing JUST the compiler
(CC=clang) vs the whole toolchain suite (LLVM=1). I do not wish to
change the semantics of those, and only for LLVM.

LLVM=1 means test clang, lld, llvm-objcopy, etc..
CC=clang means test clang, bfd, GNU objcopy, etc..
https://docs.kernel.org/kbuild/llvm.html#llvm-utilities

I don't wish to see the behavior of CC=clang change based on LLVM=0 being set.

> is still possible by explicitly adding LLVM=0.

Thanks for testing with LLVM, and even multiple versions of LLVM.

I'm still sympathetic, but only up to a point. A change like this MUST
CC the kbuild AND LLVM maintainers AND respective lists though.  It
also has little to do with the rest of the series.

As per our previous discussion
https://lore.kernel.org/linux-kbuild/CAKwvOd=x9E=7WcCiieso-CDiiU-wMFcXL4W3V5j8dq7BL5QT+w@mail.gmail.com/
I'm still of the opionion that this should be solved by modifications
(permanent or one off) to one's $PATH.

To see what that would look like, let's test that out:

$ sudo apt install clang-11 lld-11

$ cd linux
$ dirname $(readlink -f $(which clang-11))
$ PATH=$(dirname $(readlink -f $(which clang-11))):$PATH make LLVM=1
-j72 -s allnoconfig all
$ llvm-readelf -p .comment vmlinux
String dump of section '.comment':
[     0] Linker: LLD 11.1.0
[    14] Debian clang version 11.1.0-4+build3


If that's too much for the command line, then add a shell function to
your shell's .rc file:

$ which make_clang
make_clang () {
  ver=$1
  shift
  if ! [[ -n $(command -v clang-$ver) ]]
  then
    echo "clang-$ver not installed"
    return 1
  fi
  PATH=$(dirname $(readlink -f $(which clang-$ver))):$PATH make CC=clang $@
}

$ make_clang 11 -j72 -s clean allnoconfig all
$ llvm-readelf -p .comment vmlinux
String dump of section '.comment':
[     0] Debian clang version 11.1.0-4+build3


Even stuffing the dirname+readlink+which in a short helper fn would let you do:

$ make CC=$(helper clang-11)

Also, Kees mentions this is an issue for testing multiple different
versions of gcc, too.  There perhaps is a way to simplify the builds
for BOTH toolchains; i.e. a yet-to-be-created shared variable denoting
the suffix for binaries?  The primary pain point seems to be Debian's
suffixing scheme; it will suffix GCC, clang, and lld, but not GNU
binutils IIUC.

>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  Makefile                       |   45 +++++++++++++++++++++++++++---------
>  tools/scripts/Makefile.include |   50 ++++++++++++++++++++++++++++-------------
>  2 files changed, 68 insertions(+), 27 deletions(-)
>
> --- 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
> +ifeq ($(ARCH),powerpc)
> +LLVM = 0
> +endif
> +ifeq ($(ARCH),s390)
> +LLVM = 0
> +endif
> +
> +# otherwise, if CC=clang, default to using LLVM to enable LTO
> +CC_BASE := $(shell echo $(CC) | sed 's/.*\///')
> +CC_NAME := $(shell echo $(CC_BASE) | cut -b "1-5")
> +ifeq ($(shell test "$(CC_NAME)" = "clang"; echo $$?),0)
> +LLVM ?= 1
> +LLVM_PFX := $(shell echo $(CC) | sed 's/\(.*\/\)\?.*/\1/')

Just curious, what prefixes have you observed in the wild?

> +LLVM_SFX := $(shell echo $(CC_BASE) | cut -b "6-")
> +endif
> +
> +# if not set by now, do not use LLVM
> +LLVM ?= 0
> +
> +ifneq ($(LLVM),0)
> +HOSTCC = $(LLVM_PFX)clang$(LLVM_SFX)
> +HOSTCXX        = $(LLVM_PFX)clang++$(LLVM_SFX)
>  else
>  HOSTCC = gcc
>  HOSTCXX        = g++
> @@ -442,15 +462,15 @@ KBUILD_HOSTLDLIBS   := $(HOST_LFS_LIBS)
>
>  # Make variables (CC, etc...)
>  CPP            = $(CC) -E
> -ifneq ($(LLVM),)
> -CC             = clang
> -LD             = ld.lld
> -AR             = llvm-ar
> -NM             = llvm-nm
> -OBJCOPY                = llvm-objcopy
> -OBJDUMP                = llvm-objdump
> -READELF                = llvm-readelf
> -STRIP          = llvm-strip
> +ifneq ($(LLVM),0)
> +CC             = $(LLVM_PFX)clang$(LLVM_SFX)
> +LD             = $(LLVM_PFX)ld.lld$(LLVM_SFX)
> +AR             = $(LLVM_PFX)llvm-ar$(LLVM_SFX)
> +NM             = $(LLVM_PFX)llvm-nm$(LLVM_SFX)
> +OBJCOPY                = $(LLVM_PFX)llvm-objcopy$(LLVM_SFX)
> +OBJDUMP                = $(LLVM_PFX)llvm-objdump$(LLVM_SFX)
> +READELF                = $(LLVM_PFX)llvm-readelf$(LLVM_SFX)
> +STRIP          = $(LLVM_PFX)llvm-strip$(LLVM_SFX)
>  else
>  CC             = $(CROSS_COMPILE)gcc
>  LD             = $(CROSS_COMPILE)ld
> @@ -461,6 +481,7 @@ OBJDUMP             = $(CROSS_COMPILE)objdump
>  READELF                = $(CROSS_COMPILE)readelf
>  STRIP          = $(CROSS_COMPILE)strip
>  endif
> +
>  PAHOLE         = pahole
>  RESOLVE_BTFIDS = $(objtree)/tools/bpf/resolve_btfids/resolve_btfids
>  LEX            = flex
> --- a/tools/scripts/Makefile.include
> +++ b/tools/scripts/Makefile.include
> @@ -51,12 +51,32 @@ define allow-override
>      $(eval $(1) = $(2)))
>  endef
>
> -ifneq ($(LLVM),)
> -$(call allow-override,CC,clang)
> -$(call allow-override,AR,llvm-ar)
> -$(call allow-override,LD,ld.lld)
> -$(call allow-override,CXX,clang++)
> -$(call allow-override,STRIP,llvm-strip)
> +# powerpc and s390 don't yet work with LLVM as a whole
> +ifeq ($(ARCH),powerpc)
> +LLVM = 0
> +endif
> +ifeq ($(ARCH),s390)
> +LLVM = 0
> +endif
> +
> +# otherwise, if CC=clang, default to using LLVM to enable LTO
> +CC_BASE := $(shell echo $(CC) | sed 's/.*\///')
> +CC_NAME := $(shell echo $(CC_BASE) | cut -b "1-5")
> +ifeq ($(shell test "$(CC_NAME)" = "clang"; echo $$?),0)
> +LLVM ?= 1
> +LLVM_PFX := $(shell echo $(CC) | sed 's/\(.*\/\)\?.*/\1/')
> +LLVM_SFX := $(shell echo $(CC_BASE) | cut -b "6-")
> +endif
> +
> +# if not set by now, do not use LLVM
> +LLVM ?= 0
> +
> +ifneq ($(LLVM),0)
> +$(call allow-override,CC,$(LLVM_PFX)clang$(LLVM_SFX))
> +$(call allow-override,AR,$(LLVM_PFX)llvm-ar$(LLVM_SFX))
> +$(call allow-override,LD,$(LLVM_PFX)ld.lld$(LLVM_SFX))
> +$(call allow-override,CXX,$(LLVM_PFX)clang++$(LLVM_SFX))
> +$(call allow-override,STRIP,$(LLVM_PFX)llvm-strip$(LLVM_SFX))
>  else
>  # Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
>  $(call allow-override,CC,$(CROSS_COMPILE)gcc)
> @@ -68,10 +88,10 @@ endif
>
>  CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
>
> -ifneq ($(LLVM),)
> -HOSTAR  ?= llvm-ar
> -HOSTCC  ?= clang
> -HOSTLD  ?= ld.lld
> +ifneq ($(LLVM),0)
> +HOSTAR  ?= $(LLVM_PFX)llvm-ar$(LLVM_SFX)
> +HOSTCC  ?= $(LLVM_PFX)clang$(LLVM_SFX)
> +HOSTLD  ?= $(LLVM_PFX)ld.lld$(LLVM_SFX)
>  else
>  HOSTAR  ?= ar
>  HOSTCC  ?= gcc
> @@ -79,11 +99,11 @@ HOSTLD  ?= ld
>  endif
>
>  # Some tools require Clang, LLC and/or LLVM utils
> -CLANG          ?= clang
> -LLC            ?= llc
> -LLVM_CONFIG    ?= llvm-config
> -LLVM_OBJCOPY   ?= llvm-objcopy
> -LLVM_STRIP     ?= llvm-strip
> +CLANG          ?= $(LLVM_PFX)clang$(LLVM_SFX)
> +LLC            ?= $(LLVM_PFX)llc$(LLVM_SFX)
> +LLVM_CONFIG    ?= $(LLVM_PFX)llvm-config$(LLVM_SFX)
> +LLVM_OBJCOPY   ?= $(LLVM_PFX)llvm-objcopy$(LLVM_SFX)
> +LLVM_STRIP     ?= $(LLVM_PFX)llvm-strip$(LLVM_SFX)
>
>  ifeq ($(CC_NO_CLANG), 1)
>  EXTRA_WARNINGS += -Wstrict-aliasing=3
>
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Kees Cook]

On Tue, Mar 01, 2022 at 01:16:04PM -0800, Nick Desaulniers wrote:
> Also, Kees mentions this is an issue for testing multiple different
> versions of gcc, too.  There perhaps is a way to simplify the builds
> for BOTH toolchains; i.e. a yet-to-be-created shared variable denoting
> the suffix for binaries?  The primary pain point seems to be Debian's
> suffixing scheme; it will suffix GCC, clang, and lld, but not GNU
> binutils IIUC.

Right. Though I think auto-detection still makes sense.

If I do:

	make CC=clang-12 LLVM=1

it'd be nice if it also used ld.lld-12.

> [...]
> Just curious, what prefixes have you observed in the wild?

For me, where ever I built clang, and "/usr/bin"

-- 
Kees Cook

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Fangrui Song]

On 2022-03-01, Kees Cook wrote:
>On Tue, Mar 01, 2022 at 01:16:04PM -0800, Nick Desaulniers wrote:
>> Also, Kees mentions this is an issue for testing multiple different
>> versions of gcc, too.  There perhaps is a way to simplify the builds
>> for BOTH toolchains; i.e. a yet-to-be-created shared variable denoting
>> the suffix for binaries?  The primary pain point seems to be Debian's
>> suffixing scheme; it will suffix GCC, clang, and lld, but not GNU
>> binutils IIUC.
>
>Right. Though I think auto-detection still makes sense.
>
>If I do:
>
>	make CC=clang-12 LLVM=1
>
>it'd be nice if it also used ld.lld-12.

This transformation may be a bit magical.

On Debian, /usr/bin/clang-13 is a symlink to /usr/lib/llvm-13/bin/clang .
Will it be fine for the user to provide correct feasible PATH?

>> [...]
>> Just curious, what prefixes have you observed in the wild?
>
>For me, where ever I built clang, and "/usr/bin"
>
>-- 
>Kees Cook
>

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Nathan Chancellor]

On Tue, Mar 01, 2022 at 01:16:04PM -0800, Nick Desaulniers wrote:
> On Thu, Feb 24, 2022 at 7:17 AM Peter Zijlstra <peterz@infradead.org> 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:
> >
> >  $ make CC=clang-13 LD=ld.lld-13 AR=llvm-ar-13 NM=llvm-nm-13 OBJCOPY=llvm-objcopy-13 OBJDUMP=llvm-objdump-13 READELF=llvm-readelf-13 STRIP=llvm-strip-13 LLVM=1
> >
> > 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 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
> 
> No, nack, we definitely do not want CC=clang to set LLVM=1. Those are
> distinctly two different things for testing JUST the compiler
> (CC=clang) vs the whole toolchain suite (LLVM=1). I do not wish to
> change the semantics of those, and only for LLVM.

I agree with this. CC is only changing the compiler, not any of the
other build utilities. CC=gcc-12 works for GCC because you are only
using a different compiler, not an entirely new toolchain (as binutils
will be the same as just CC=gcc).

> LLVM=1 means test clang, lld, llvm-objcopy, etc..
> CC=clang means test clang, bfd, GNU objcopy, etc..
> https://docs.kernel.org/kbuild/llvm.html#llvm-utilities
> 
> I don't wish to see the behavior of CC=clang change based on LLVM=0 being set.
> 
> > is still possible by explicitly adding LLVM=0.
> 
> Thanks for testing with LLVM, and even multiple versions of LLVM.
> 
> I'm still sympathetic, but only up to a point. A change like this MUST
> CC the kbuild AND LLVM maintainers AND respective lists though.  It
> also has little to do with the rest of the series.
> 
> As per our previous discussion
> https://lore.kernel.org/linux-kbuild/CAKwvOd=x9E=7WcCiieso-CDiiU-wMFcXL4W3V5j8dq7BL5QT+w@mail.gmail.com/
> I'm still of the opionion that this should be solved by modifications
> (permanent or one off) to one's $PATH.

However, I think we could still address Peter's complaint of "there
should be an easier way for me to use the tools that are already in my
PATH" with his first iteration of this patch [1], which I feel is
totally reasonable:

$ make LLVM=-14

It is still easy to use (in fact, it is shorter than 'CC=clang-14') and
it does not change anything else about how we build with LLVM. We would
just have to add something along the lines of

"If your LLVM tools have a suffix like Debian's (clang-14, ld.lld-14,
etc.), use LLVM=<suffix>.

$ make LLVM=-14"

to Documentation/kbuild/llvm.rst.

I might change the patch not to be so clever though:

ifneq ($(LLVM),)
ifneq ($(LLVM),1)
LLVM_SFX := $(LLVM)
endif
endif

[1]: https://lore.kernel.org/r/YXqpFHeY26sEbort@hirez.programming.kicks-ass.net/

Cheers,
Nathan

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Kees Cook]

On Wed, Mar 02, 2022 at 09:37:04AM -0700, Nathan Chancellor wrote:
> However, I think we could still address Peter's complaint of "there
> should be an easier way for me to use the tools that are already in my
> PATH" with his first iteration of this patch [1], which I feel is
> totally reasonable:
> 
> $ make LLVM=-14
> 
> It is still easy to use (in fact, it is shorter than 'CC=clang-14') and
> it does not change anything else about how we build with LLVM. We would
> just have to add something along the lines of
> 
> "If your LLVM tools have a suffix like Debian's (clang-14, ld.lld-14,
> etc.), use LLVM=<suffix>.
> 
> $ make LLVM=-14"
> 
> to Documentation/kbuild/llvm.rst.
> 
> I might change the patch not to be so clever though:
> 
> ifneq ($(LLVM),)
> ifneq ($(LLVM),1)
> LLVM_SFX := $(LLVM)
> endif
> endif

I like this idea! I think it's much easier to control than PATH (though
I see the rationale there too).

-- 
Kees Cook

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Nick Desaulniers]

On Wed, Mar 2, 2022 at 8:37 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Tue, Mar 01, 2022 at 01:16:04PM -0800, Nick Desaulniers wrote:
> > As per our previous discussion
> > https://lore.kernel.org/linux-kbuild/CAKwvOd=x9E=7WcCiieso-CDiiU-wMFcXL4W3V5j8dq7BL5QT+w@mail.gmail.com/
> > I'm still of the opionion that this should be solved by modifications
> > (permanent or one off) to one's $PATH.
>
> However, I think we could still address Peter's complaint of "there
> should be an easier way for me to use the tools that are already in my
> PATH" with his first iteration of this patch [1], which I feel is
> totally reasonable:
>
> $ make LLVM=-14
>
> It is still easy to use (in fact, it is shorter than 'CC=clang-14') and
> it does not change anything else about how we build with LLVM. We would
> just have to add something along the lines of
>
> "If your LLVM tools have a suffix like Debian's (clang-14, ld.lld-14,
> etc.), use LLVM=<suffix>.

"If your LLVM tools have a suffix and you prefer to test an explicit
version rather than the unsuffixed executables ..."

>
> $ make LLVM=-14"
>
> to Documentation/kbuild/llvm.rst.
>
> I might change the patch not to be so clever though:
>
> ifneq ($(LLVM),)
> ifneq ($(LLVM),1)
> LLVM_SFX := $(LLVM)
> endif
> endif
>
> [1]: https://lore.kernel.org/r/YXqpFHeY26sEbort@hirez.programming.kicks-ass.net/

I'd be much more amenable to that approach.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Nathan Chancellor]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

On Wed, Mar 02, 2022 at 11:18:07AM -0800, Nick Desaulniers wrote:
> On Wed, Mar 2, 2022 at 8:37 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Tue, Mar 01, 2022 at 01:16:04PM -0800, Nick Desaulniers wrote:
> > > As per our previous discussion
> > > https://lore.kernel.org/linux-kbuild/CAKwvOd=x9E=7WcCiieso-CDiiU-wMFcXL4W3V5j8dq7BL5QT+w@mail.gmail.com/
> > > I'm still of the opionion that this should be solved by modifications
> > > (permanent or one off) to one's $PATH.
> >
> > However, I think we could still address Peter's complaint of "there
> > should be an easier way for me to use the tools that are already in my
> > PATH" with his first iteration of this patch [1], which I feel is
> > totally reasonable:
> >
> > $ make LLVM=-14
> >
> > It is still easy to use (in fact, it is shorter than 'CC=clang-14') and
> > it does not change anything else about how we build with LLVM. We would
> > just have to add something along the lines of
> >
> > "If your LLVM tools have a suffix like Debian's (clang-14, ld.lld-14,
> > etc.), use LLVM=<suffix>.
> 
> "If your LLVM tools have a suffix and you prefer to test an explicit
> version rather than the unsuffixed executables ..."

Ack, I included this.

> > $ make LLVM=-14"
> >
> > to Documentation/kbuild/llvm.rst.
> >
> > I might change the patch not to be so clever though:
> >
> > ifneq ($(LLVM),)
> > ifneq ($(LLVM),1)
> > LLVM_SFX := $(LLVM)
> > endif
> > endif
> >
> > [1]: https://lore.kernel.org/r/YXqpFHeY26sEbort@hirez.programming.kicks-ass.net/
> 
> I'd be much more amenable to that approach.

Sounds good, tentative patch attached, it passes all of my testing.
There is an instance of $(LLVM) in tools/testing/selftests/lib.mk that I
did not touch, as that will presumably have to go through the selftests
tree. I can send a separate patch for that later.

Peter, is this approach okay with you? If so, would you like to be
co-author or should I use a suggested-by tag?

Cheers,
Nathan

[-- Attachment #2: 0001-kbuild-Allow-a-suffix-with-LLVM.patch --]
[-- Type: text/plain, Size: 4675 bytes --]

From 83219caafbb7dbc2e41e3888ba5079d342aff633 Mon Sep 17 00:00:00 2001
From: Nathan Chancellor <nathan@kernel.org>
Date: Wed, 2 Mar 2022 13:28:14 -0700
Subject: [PATCH] kbuild: Allow a suffix with $(LLVM)

The LLVM variable allows a developer to quickly switch between the GNU
and LLVM tools. However, it does not handle versioned binaries, such as
the ones shipped by Debian, as LLVM=1 just defines the build variables
with the unversioned binaries.

There was some discussion during the review of the patch that introduces
LLVM=1 around this, ultimately coming to the conclusion that developers
can just add the folder that contains the unversioned binaries to their
PATH, as Debian's versioned suffixed binaries are really just symlinks
to the unversioned binaries in /usr/lib/llvm-#/bin:

$ realpath /usr/bin/clang-14
/usr/lib/llvm-14/bin/clang

$ PATH=/usr/lib/llvm-14/bin:$PATH make ... LLVM=1

However, it is simple enough to support this scheme directly in the
Kbuild system by allowing the developer to specify the version suffix
with LLVM=, which is shorter than the above suggestion:

$ make ... LLVM=-14

It does not change the meaning of LLVM=1 (which will continue to use
unversioned binaries) and it does not add too much additional complexity
to the existing $(LLVM) code, while allowing developers to quickly test
their series with different versions of the whole LLVM suite of tools.

Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 Documentation/kbuild/llvm.rst  |  7 +++++++
 Makefile                       | 24 ++++++++++++++----------
 tools/scripts/Makefile.include | 20 ++++++++++++--------
 3 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/Documentation/kbuild/llvm.rst b/Documentation/kbuild/llvm.rst
index d32616891dcf..5805a8473a36 100644
--- a/Documentation/kbuild/llvm.rst
+++ b/Documentation/kbuild/llvm.rst
@@ -60,6 +60,13 @@ They can be enabled individually. The full list of the parameters: ::
 	  OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump READELF=llvm-readelf \
 	  HOSTCC=clang HOSTCXX=clang++ HOSTAR=llvm-ar HOSTLD=ld.lld
 
+If your LLVM tools have a suffix and you prefer to test an explicit version rather
+than the unsuffixed executables, use ``LLVM=<suffix>``. For example: ::
+
+	make LLVM=-14
+
+will use ``clang-14``, ``ld.lld-14``, etc.
+
 The integrated assembler is enabled by default. You can pass ``LLVM_IAS=0`` to
 disable it.
 
diff --git a/Makefile b/Makefile
index daeb5c88b50b..89b61e693258 100644
--- a/Makefile
+++ b/Makefile
@@ -424,8 +424,12 @@ 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++
+ifneq ($(LLVM),1)
+LLVM_SFX := $(LLVM)
+endif
+
+HOSTCC	= clang$(LLVM_SFX)
+HOSTCXX	= clang++$(LLVM_SFX)
 else
 HOSTCC	= gcc
 HOSTCXX	= g++
@@ -443,14 +447,14 @@ KBUILD_HOSTLDLIBS   := $(HOST_LFS_LIBS) $(HOSTLDLIBS)
 # Make variables (CC, etc...)
 CPP		= $(CC) -E
 ifneq ($(LLVM),)
-CC		= clang
-LD		= ld.lld
-AR		= llvm-ar
-NM		= llvm-nm
-OBJCOPY		= llvm-objcopy
-OBJDUMP		= llvm-objdump
-READELF		= llvm-readelf
-STRIP		= llvm-strip
+CC		= clang$(LLVM_SFX)
+LD		= ld.lld$(LLVM_SFX)
+AR		= llvm-ar$(LLVM_SFX)
+NM		= llvm-nm$(LLVM_SFX)
+OBJCOPY		= llvm-objcopy$(LLVM_SFX)
+OBJDUMP		= llvm-objdump$(LLVM_SFX)
+READELF		= llvm-readelf$(LLVM_SFX)
+STRIP		= llvm-strip$(LLVM_SFX)
 else
 CC		= $(CROSS_COMPILE)gcc
 LD		= $(CROSS_COMPILE)ld
diff --git a/tools/scripts/Makefile.include b/tools/scripts/Makefile.include
index 79d102304470..ab3b2a7dcc94 100644
--- a/tools/scripts/Makefile.include
+++ b/tools/scripts/Makefile.include
@@ -52,11 +52,15 @@ define allow-override
 endef
 
 ifneq ($(LLVM),)
-$(call allow-override,CC,clang)
-$(call allow-override,AR,llvm-ar)
-$(call allow-override,LD,ld.lld)
-$(call allow-override,CXX,clang++)
-$(call allow-override,STRIP,llvm-strip)
+ifneq ($(LLVM),1)
+LLVM_SFX := $(LLVM)
+endif
+
+$(call allow-override,CC,clang$(LLVM_SFX))
+$(call allow-override,AR,llvm-ar$(LLVM_SFX))
+$(call allow-override,LD,ld.lld$(LLVM_SFX))
+$(call allow-override,CXX,clang++$(LLVM_SFX))
+$(call allow-override,STRIP,llvm-strip$(LLVM_SFX))
 else
 # Allow setting various cross-compile vars or setting CROSS_COMPILE as a prefix.
 $(call allow-override,CC,$(CROSS_COMPILE)gcc)
@@ -69,9 +73,9 @@ endif
 CC_NO_CLANG := $(shell $(CC) -dM -E -x c /dev/null | grep -Fq "__clang__"; echo $$?)
 
 ifneq ($(LLVM),)
-HOSTAR  ?= llvm-ar
-HOSTCC  ?= clang
-HOSTLD  ?= ld.lld
+HOSTAR  ?= llvm-ar$(LLVM_SFX)
+HOSTCC  ?= clang$(LLVM_SFX)
+HOSTLD  ?= ld.lld$(LLVM_SFX)
 else
 HOSTAR  ?= ar
 HOSTCC  ?= gcc

base-commit: fb184c4af9b9f4563e7a126219389986a71d5b5b
-- 
2.35.1

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Nick Desaulniers]

On Wed, Mar 2, 2022 at 1:15 PM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Wed, Mar 02, 2022 at 11:18:07AM -0800, Nick Desaulniers wrote:
> > I'd be much more amenable to that approach.
>
> Sounds good, tentative patch attached, it passes all of my testing.
> There is an instance of $(LLVM) in tools/testing/selftests/lib.mk that I
> did not touch, as that will presumably have to go through the selftests
> tree. I can send a separate patch for that later.

LGTM!
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Kees Cook]

On Wed, Mar 02, 2022 at 02:15:45PM -0700, Nathan Chancellor wrote:
> Sounds good, tentative patch attached, it passes all of my testing.
> There is an instance of $(LLVM) in tools/testing/selftests/lib.mk that I
> did not touch, as that will presumably have to go through the selftests
> tree. I can send a separate patch for that later.

I think it's fine to include that here, just to keep the logic together.

> Peter, is this approach okay with you? If so, would you like to be
> co-author or should I use a suggested-by tag?
> 
> Cheers,
> Nathan

> From 83219caafbb7dbc2e41e3888ba5079d342aff633 Mon Sep 17 00:00:00 2001
> From: Nathan Chancellor <nathan@kernel.org>
> Date: Wed, 2 Mar 2022 13:28:14 -0700
> Subject: [PATCH] kbuild: Allow a suffix with $(LLVM)
> 
> The LLVM variable allows a developer to quickly switch between the GNU
> and LLVM tools. However, it does not handle versioned binaries, such as
> the ones shipped by Debian, as LLVM=1 just defines the build variables
> with the unversioned binaries.
> 
> There was some discussion during the review of the patch that introduces
> LLVM=1 around this, ultimately coming to the conclusion that developers
> can just add the folder that contains the unversioned binaries to their
> PATH, as Debian's versioned suffixed binaries are really just symlinks
> to the unversioned binaries in /usr/lib/llvm-#/bin:
> 
> $ realpath /usr/bin/clang-14
> /usr/lib/llvm-14/bin/clang
> 
> $ PATH=/usr/lib/llvm-14/bin:$PATH make ... LLVM=1
> 
> However, it is simple enough to support this scheme directly in the
> Kbuild system by allowing the developer to specify the version suffix
> with LLVM=, which is shorter than the above suggestion:
> 
> $ make ... LLVM=-14
> 
> It does not change the meaning of LLVM=1 (which will continue to use
> unversioned binaries) and it does not add too much additional complexity
> to the existing $(LLVM) code, while allowing developers to quickly test
> their series with different versions of the whole LLVM suite of tools.
> 
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

I like it!

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v2 01/39] kbuild: Fix clang build

[Peter Zijlstra]

On Wed, Mar 02, 2022 at 02:15:45PM -0700, Nathan Chancellor wrote:
> Peter, is this approach okay with you? If so, would you like to be
> co-author or should I use a suggested-by tag?

Yeah, this helps lots. Suggested-by seems fine. Thanks!

Re: [PATCH v2 05/39] x86: Base IBT bits

[Kees Cook]

On Thu, Feb 24, 2022 at 03:51:43PM +0100, Peter Zijlstra wrote:
> Add Kconfig, Makefile and basic instruction support for x86 IBT.
> 
> XXX clang is not playing ball, probably lld being 'funny', I'm having
> problems with .plt entries appearing all over after linking.

I'll try to look into this; I know you've been chatting with Nathan
about it. Is there an open bug for it? (And any kind of reproducer
smaller than a 39 patch series we can show the linker folks?) :)

> [...]
> +config X86_KERNEL_IBT
> +	prompt "Indirect Branch Tracking"
> +	bool
> +	depends on X86_64 && CC_HAS_IBT
> +	help
> +	  Build the kernel with support for Indirect Branch Tracking, a
> +	  hardware supported CFI scheme. Any indirect call must land on

	  hardware support course-grain forward-edge Control Flow Integrity
	  protection. It enforces that all indirect calls must land on

> +	  an ENDBR instruction, as such, the compiler will litter the
> +	  code with them to make this happen.

"litter the code" -> "instrument the machine code".


> +
>  config X86_INTEL_MEMORY_PROTECTION_KEYS
>  	prompt "Memory Protection Keys"
>  	def_bool y
> --- a/arch/x86/Makefile
> +++ b/arch/x86/Makefile
> @@ -36,7 +36,7 @@ endif
>  
>  # How to compile the 16-bit code.  Note we always compile for -march=i386;
>  # that way we can complain to the user if the CPU is insufficient.
> -REALMODE_CFLAGS	:= -m16 -g -Os -DDISABLE_BRANCH_PROFILING \
> +REALMODE_CFLAGS	:= -m16 -g -Os -DDISABLE_BRANCH_PROFILING -D__DISABLE_EXPORTS \
>  		   -Wall -Wstrict-prototypes -march=i386 -mregparm=3 \
>  		   -fno-strict-aliasing -fomit-frame-pointer -fno-pic \
>  		   -mno-mmx -mno-sse $(call cc-option,-fcf-protection=none)

This change seems important separately from this patch, yes? (Or at
least a specific call-out in the commit log.)

Otherwise, looks good.

-- 
Kees Cook

Re: [PATCH v2 05/39] x86: Base IBT bits

[Nathan Chancellor]

On Thu, Feb 24, 2022 at 04:35:51PM -0800, Kees Cook wrote:
> On Thu, Feb 24, 2022 at 03:51:43PM +0100, Peter Zijlstra wrote:
> > Add Kconfig, Makefile and basic instruction support for x86 IBT.
> > 
> > XXX clang is not playing ball, probably lld being 'funny', I'm having
> > problems with .plt entries appearing all over after linking.
> 
> I'll try to look into this; I know you've been chatting with Nathan
> about it. Is there an open bug for it? (And any kind of reproducer
> smaller than a 39 patch series we can show the linker folks?) :)

I should be able to create a reproducer with cvise and file a bug on
GitHub around this tomorrow, I should have done it after Peter's
comments on IRC.

Cheers,
Nathan

> > [...]
> > +config X86_KERNEL_IBT
> > +	prompt "Indirect Branch Tracking"
> > +	bool
> > +	depends on X86_64 && CC_HAS_IBT
> > +	help
> > +	  Build the kernel with support for Indirect Branch Tracking, a
> > +	  hardware supported CFI scheme. Any indirect call must land on
> 
> 	  hardware support course-grain forward-edge Control Flow Integrity
> 	  protection. It enforces that all indirect calls must land on
> 
> > +	  an ENDBR instruction, as such, the compiler will litter the
> > +	  code with them to make this happen.
> 
> "litter the code" -> "instrument the machine code".
> 
> 
> > +
> >  config X86_INTEL_MEMORY_PROTECTION_KEYS
> >  	prompt "Memory Protection Keys"
> >  	def_bool y
> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -36,7 +36,7 @@ endif
> >  
> >  # How to compile the 16-bit code.  Note we always compile for -march=i386;
> >  # that way we can complain to the user if the CPU is insufficient.
> > -REALMODE_CFLAGS	:= -m16 -g -Os -DDISABLE_BRANCH_PROFILING \
> > +REALMODE_CFLAGS	:= -m16 -g -Os -DDISABLE_BRANCH_PROFILING -D__DISABLE_EXPORTS \
> >  		   -Wall -Wstrict-prototypes -march=i386 -mregparm=3 \
> >  		   -fno-strict-aliasing -fomit-frame-pointer -fno-pic \
> >  		   -mno-mmx -mno-sse $(call cc-option,-fcf-protection=none)
> 
> This change seems important separately from this patch, yes? (Or at
> least a specific call-out in the commit log.)
> 
> Otherwise, looks good.
> 
> -- 
> Kees Cook

Re: [PATCH v2 05/39] x86: Base IBT bits

[Nathan Chancellor]

On Thu, Feb 24, 2022 at 05:46:59PM -0700, Nathan Chancellor wrote:
> On Thu, Feb 24, 2022 at 04:35:51PM -0800, Kees Cook wrote:
> > On Thu, Feb 24, 2022 at 03:51:43PM +0100, Peter Zijlstra wrote:
> > > Add Kconfig, Makefile and basic instruction support for x86 IBT.
> > > 
> > > XXX clang is not playing ball, probably lld being 'funny', I'm having
> > > problems with .plt entries appearing all over after linking.
> > 
> > I'll try to look into this; I know you've been chatting with Nathan
> > about it. Is there an open bug for it? (And any kind of reproducer
> > smaller than a 39 patch series we can show the linker folks?) :)
> 
> I should be able to create a reproducer with cvise and file a bug on
> GitHub around this tomorrow, I should have done it after Peter's
> comments on IRC.

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

Cheers,
Nathan

Re: [PATCH v2 05/39] x86: Base IBT bits

[Joao Moreira]

> https://github.com/ClangBuiltLinux/linux/issues/1606

Candidate fix: https://reviews.llvm.org/D120600

Re: [PATCH v2 05/39] x86: Base IBT bits

[Kees Cook]

On Fri, Feb 25, 2022 at 04:29:49PM -0800, Joao Moreira wrote:
> > https://github.com/ClangBuiltLinux/linux/issues/1606
> 
> Candidate fix: https://reviews.llvm.org/D120600

And landed! Thanks!

Since this is a pretty small change, do you think it could be backported
to the clang-14 branch?

-- 
Kees Cook

Re: [PATCH v2 05/39] x86: Base IBT bits

[Fāng-ruì Sòng]

On Fri, Feb 25, 2022 at 8:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Feb 25, 2022 at 04:29:49PM -0800, Joao Moreira wrote:
> > > https://github.com/ClangBuiltLinux/linux/issues/1606
> >
> > Candidate fix: https://reviews.llvm.org/D120600
>
> And landed! Thanks!
>
> Since this is a pretty small change, do you think it could be backported
> to the clang-14 branch?
>
> --
> Kees Cook

I have pushed it to release/14.x :)
https://github.com/llvm/llvm-project/commit/f8ca5fabdb54fdf64b3dffb38ebf7d0220f415a2

The current release schedule is
https://discourse.llvm.org/t/llvm-14-0-0-release-schedule/5846


-- 
宋方睿

Re: [PATCH v2 05/39] x86: Base IBT bits

[Kees Cook]

On Fri, Feb 25, 2022 at 08:59:57PM -0800, Fāng-ruì Sòng wrote:
> On Fri, Feb 25, 2022 at 8:58 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Feb 25, 2022 at 04:29:49PM -0800, Joao Moreira wrote:
> > > > https://github.com/ClangBuiltLinux/linux/issues/1606
> > >
> > > Candidate fix: https://reviews.llvm.org/D120600
> >
> > And landed! Thanks!
> >
> > Since this is a pretty small change, do you think it could be backported
> > to the clang-14 branch?
> >
> > --
> > Kees Cook
> 
> I have pushed it to release/14.x :)
> https://github.com/llvm/llvm-project/commit/f8ca5fabdb54fdf64b3dffb38ebf7d0220f415a2
> 
> The current release schedule is
> https://discourse.llvm.org/t/llvm-14-0-0-release-schedule/5846

Great! :)

-- 
Kees Cook

Re: [PATCH v2 05/39] x86: Base IBT bits

[Peter Zijlstra]

On Thu, Feb 24, 2022 at 04:35:51PM -0800, Kees Cook wrote:

> > --- a/arch/x86/Makefile
> > +++ b/arch/x86/Makefile
> > @@ -36,7 +36,7 @@ endif
> >  
> >  # How to compile the 16-bit code.  Note we always compile for -march=i386;
> >  # that way we can complain to the user if the CPU is insufficient.
> > -REALMODE_CFLAGS	:= -m16 -g -Os -DDISABLE_BRANCH_PROFILING \
> > +REALMODE_CFLAGS	:= -m16 -g -Os -DDISABLE_BRANCH_PROFILING -D__DISABLE_EXPORTS \
> >  		   -Wall -Wstrict-prototypes -march=i386 -mregparm=3 \
> >  		   -fno-strict-aliasing -fomit-frame-pointer -fno-pic \
> >  		   -mno-mmx -mno-sse $(call cc-option,-fcf-protection=none)
> 
> This change seems important separately from this patch, yes? (Or at
> least a specific call-out in the commit log.)

It was to not stuff endbr in realmode code. *something* complained about
it (and Joao independently mentioned it on IRC).

Could be I hit some compile fail somewhere and this was the cleanest way
to simply make it go away.

I'll go mention it somewhere.