| Message ID | 20220302234400.782002-1-nathan@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: Allow a suffix with $(LLVM) | expand |
On Thu, Mar 3, 2022 at 8:47 AM Nathan Chancellor <nathan@kernel.org> wrote: > > 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/20200317215515.226917-1-ndesaulniers@google.com/ > Link: https://lore.kernel.org/r/20220224151322.072632223@infradead.org/ > Suggested-by: Peter Zijlstra <peterz@infradead.org> > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > > RFC -> v1: https://lore.kernel.org/r/Yh%2FegU1LZudfrgVy@dev-arch.thelio-3990X/ > > * 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(-) > > 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. 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. > diff --git a/Makefile b/Makefile > index a82095c69fdd..963840c00eae 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 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
On Fri, Mar 04, 2022 at 08:16:00PM +0900, Masahiro Yamada wrote: > On Thu, Mar 3, 2022 at 8:47 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > 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/20200317215515.226917-1-ndesaulniers@google.com/ > > Link: https://lore.kernel.org/r/20220224151322.072632223@infradead.org/ > > Suggested-by: Peter Zijlstra <peterz@infradead.org> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > --- > > > > RFC -> v1: https://lore.kernel.org/r/Yh%2FegU1LZudfrgVy@dev-arch.thelio-3990X/ > > > > * 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(-) > > > > 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. > > > 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. 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. > > diff --git a/Makefile b/Makefile > > index a82095c69fdd..963840c00eae 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 > > 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. 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. > 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. > 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 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. > 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
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 a82095c69fdd..963840c00eae 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++ @@ -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 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 diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index a40add31a2e3..b3ab713537c6 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -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