kbuild: Allow a suffix with $(LLVM)

PATCH In review Nathan Chancellor 2 participants 3 messages Mar 2, 2022 – Mar 4, 2022

Subsystems (4)

Documentation Kbuild Kselftest Clang/LLVM build support

lore

Share & tools

lore

Nathan Chancellor

author maintainer

2022-03-02 23:47 UTC #0 lore raw

PATCH 1/1 kbuild: Allow a suffix with $(LLVM)

The LLVM make 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 tool variables with the unversioned binaries.

There was some discussion during the review of the patch that introduces LLVM=1 around versioned binaries, 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, that can be cumbersome to developers who are constantly testing series with different toolchains and versions. It is simple enough to support these versioned binaries 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.

Link https://lore.kernel.org/r/[email protected]/ open here

Reviewed-by Kees Cook <[email protected]>

Reviewed-by Nick Desaulniers <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

---

RFC -> v1: https://lore.kernel.org/r/Yh%[email protected]/

* Tidy up commit message slightly.

* Add tags.

* Add links to prior discussions for context.

* Add change to tools/testing/selftests/lib.mk.

I would like for this to go through the Kbuild tree, please ack as necessary.

 Documentation/kbuild/llvm.rst  |  7 +++++++
 Makefile                       | 24 ++++++++++++++----------
 tools/scripts/Makefile.include | 20 ++++++++++++--------
 tools/testing/selftests/lib.mk |  6 +++++-
 4 files changed, 38 insertions(+), 19 deletions(-)

Documentation/kbuild/llvm.rst +7 -0

index d32616891dcf..5805a8473a36 100644
@@ -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.

Makefile +14 ~~-10~~

index a82095c69fdd..963840c00eae 100644
@@ -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++

@@ -444,14 +448,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

tools/scripts/Makefile.include +12 -8

index 79d102304470..ab3b2a7dcc94 100644
@@ -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

tools/testing/selftests/lib.mk +5 -1

index a40add31a2e3..b3ab713537c6 100644
@@ -1,7 +1,11 @@
 # This mimics the top-level Makefile. We do it explicitly here so that this
 # Makefile can operate with or without the kbuild infrastructure.
 ifneq ($(LLVM),)
-CC := clang
+ifneq ($(LLVM),1)
+LLVM_SFX := $(LLVM)
+endif
+
+CC := clang$(LLVM_SFX)
 else
 CC := $(CROSS_COMPILE)gcc
 endif

base-commit: 55de8686df7ed2b5237867b130e30c728bbd9db4

Signature · 1 line

2.35.1

Masahiro Yamada

2022-03-04 11:17 UTC #1 lore raw

Quoted 64 lines · Nathan Chancellor

The LLVM make 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 tool variables with the unversioned binaries.
There was some discussion during the review of the patch that introduces LLVM=1 around versioned binaries, 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, that can be cumbersome to developers who are constantly testing series with different toolchains and versions. It is simple enough to support these versioned binaries 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.
Link https://lore.kernel.org/r/[email protected]/ open here
Link https://lore.kernel.org/r/[email protected]/ open here
Suggested-by Peter Zijlstra <[email protected]>
Reviewed-by Kees Cook <[email protected]>
Reviewed-by Nick Desaulniers <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>
---
RFC -> v1: https://lore.kernel.org/r/Yh%[email protected]/
* Tidy up commit message slightly.
* Add tags.
* Add links to prior discussions for context.
* Add change to tools/testing/selftests/lib.mk.
I would like for this to go through the Kbuild tree, please ack as necessary.
 Documentation/kbuild/llvm.rst  |  7 +++++++
 Makefile                       | 24 ++++++++++++++----------
 tools/scripts/Makefile.include | 20 ++++++++++++--------
 tools/testing/selftests/lib.mk |  6 +++++-
 4 files changed, 38 insertions(+), 19 deletions(-)
Documentation/kbuild/llvm.rst +0 -0
index d32616891dcf..5805a8473a36 100644
@@ -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

Documentation/kbuild/llvm.rst They can be enabled individually. The full list of the parameters: :: · line 71

Quoted 10 lines

Documentation/kbuild/llvm.rst +7 -0

 
+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.

Perhaps, it might be worth mentioning the difference between LLVM=1 and LLVM=<suffix>

The current behavior is,
any value other than '1' is regarded as a suffix.

Makefile HOST_LFS_LDFLAGS := $(shell getconf LFS_LDFLAGS 2>/dev/null) · line 429

Quoted 13 lines

Makefile +3 -2

index a82095c69fdd..963840c00eae 100644
@@ -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

I am OK with this, but please note LLVM=0
uses 'clang0', 'ld.lld0' instead of disabling
LLVM explicitly.

This might be a small surprise because LLVM_IAS=0
is used to disable the integrated assembler.

If you want handle LLVM=<suffix>
only when <suffix> start with a hyphen,
you can do like this:

ifneq ($(filter -%, $(LLVM)),)
LLVM_SFX := $(LLVM)
endif

In the future, If somebody requests to support
make LLVM=/path/to/my/own/llvm/dir/
to use llvm tools in that path,
we can expand the code like this:

# "LLVM=foo/bar/" is a syntax sugar of "LLVM=1 LLVM_PFX=foo/bar" # "LLVM=-foo" is a syntax sugar of "LLVM=1 LLVM_SFX=-foo"

ifneq ($(filter %/, $(LLVM)),)
LLVM_PFX := $(LLVM)
else ifneq ($(filter -%, $(LLVM)),)
LLVM_SFX := $(LLVM)
endif

Lastly, I personally prefer to fully spell LLVM_SUFFIX as Nick originally suggested:
https://lkml.org/lkml/2020/3/17/1477

Signature · 2 lines

Best Regards
Masahiro Yamada

Nathan Chancellor

author maintainer

2022-03-04 16:27 UTC #2 lore raw

Quoted 80 lines · Masahiro Yamada

Quoted 64 lines · Nathan Chancellor
The LLVM make 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 tool variables with the unversioned binaries.
There was some discussion during the review of the patch that introduces LLVM=1 around versioned binaries, 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, that can be cumbersome to developers who are constantly testing series with different toolchains and versions. It is simple enough to support these versioned binaries 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.
Link https://lore.kernel.org/r/[email protected]/ open here
Link https://lore.kernel.org/r/[email protected]/ open here
Suggested-by Peter Zijlstra <[email protected]>
Reviewed-by Kees Cook <[email protected]>
Reviewed-by Nick Desaulniers <[email protected]>
Signed-off-by Nathan Chancellor <[email protected]>
---
RFC -> v1: https://lore.kernel.org/r/Yh%[email protected]/
* Tidy up commit message slightly.
* Add tags.
* Add links to prior discussions for context.
* Add change to tools/testing/selftests/lib.mk.
I would like for this to go through the Kbuild tree, please ack as necessary.
 Documentation/kbuild/llvm.rst  |  7 +++++++
 Makefile                       | 24 ++++++++++++++----------
 tools/scripts/Makefile.include | 20 ++++++++++++--------
 tools/testing/selftests/lib.mk |  6 +++++-
 4 files changed, 38 insertions(+), 19 deletions(-)
Documentation/kbuild/llvm.rst +0 -0
index d32616891dcf..5805a8473a36 100644
@@ -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
Documentation/kbuild/llvm.rst They can be enabled individually. The full list of the parameters: :: · line 71
Quoted 10 lines
Documentation/kbuild/llvm.rst +7 -0
 
+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.
Perhaps, it might be worth mentioning the difference between LLVM=1 and LLVM=<suffix>

Quoted 2 lines

The current behavior is,
any value other than '1' is regarded as a suffix.

Maybe just adding something like:

"... prefer to test an explicit version rather than the unsuffixed executables like above, ..."

? I'll try to come up with a clearer way to word everything for v2.

Quoted 18 lines

Makefile HOST_LFS_LDFLAGS := $(shell getconf LFS_LDFLAGS 2>/dev/null) · line 429
Quoted 13 lines
Makefile +3 -2
index a82095c69fdd..963840c00eae 100644
@@ -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
I am OK with this, but please note LLVM=0
uses 'clang0', 'ld.lld0' instead of disabling
LLVM explicitly.

Quoted 2 lines

This might be a small surprise because LLVM_IAS=0
is used to disable the integrated assembler.

Right, but we have that problem right now, as LLVM=0 is currently treated like LLVM=1. I suppose I could add a line to the documentation in a follow up change to clarify this.

Quoted 7 lines

If you want handle LLVM=<suffix>
only when <suffix> start with a hyphen,
you can do like this:
ifneq ($(filter -%, $(LLVM)),)
LLVM_SFX := $(LLVM)
endif

I did think about this. I guess the only reason I did not do that in this version is someone might have a different suffix scheme than Debian's but it is probably better to be a little bit more precise based on what we know in this moment. I will change it to that and update the documentation.

Quoted 7 lines

In the future, If somebody requests to support
make LLVM=/path/to/my/own/llvm/dir/
to use llvm tools in that path,
we can expand the code like this:

Quoted 8 lines

# "LLVM=foo/bar/" is a syntax sugar of "LLVM=1 LLVM_PFX=foo/bar" # "LLVM=-foo" is a syntax sugar of "LLVM=1 LLVM_SFX=-foo"
ifneq ($(filter %/, $(LLVM)),)
LLVM_PFX := $(LLVM)
else ifneq ($(filter -%, $(LLVM)),)
LLVM_SFX := $(LLVM)
endif

I know I personally I would use the prefix form when testing with LLVM=1 so I think I will just go ahead and support that now, especially since Peter had aimed to support a full path with his CC patch that we NAK'd.

Quoted 3 lines

Lastly, I personally prefer to fully spell LLVM_SUFFIX as Nick originally suggested:
https://lkml.org/lkml/2020/3/17/1477

Ack, I have changed this locally and I'll send a v2 along shortly once I have written some documentation to codify these suggested changes.

Thank you for the comments and review, cheers!
Nathan