| Message ID | 20240531183751.100541-2-jhubbard@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/lib.mk: LLVM=1, CC=clang, and warnings | expand |
On Fri, May 31, 2024 at 11:37:50AM -0700, John Hubbard wrote: > The kselftests may be built in a couple different ways: > make LLVM=1 > make CC=clang > > In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest > of lib.mk, and any Makefiles that include lib.mk, can base decisions > solely on whether or not LLVM is set. ICBW but I believe there are still some architectures with clang but not lld support where there's a use case for using CC=clang.
On 6/3/24 8:32 AM, Mark Brown wrote: > On Fri, May 31, 2024 at 11:37:50AM -0700, John Hubbard wrote: >> The kselftests may be built in a couple different ways: >> make LLVM=1 >> make CC=clang >> >> In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest >> of lib.mk, and any Makefiles that include lib.mk, can base decisions >> solely on whether or not LLVM is set. > > ICBW but I believe there are still some architectures with clang but not > lld support where there's a use case for using CC=clang. I'm inclined to wait for those to make themselves known... :) thanks,
On 6/3/24 10:09 AM, John Hubbard wrote: > On 6/3/24 8:32 AM, Mark Brown wrote: >> On Fri, May 31, 2024 at 11:37:50AM -0700, John Hubbard wrote: >>> The kselftests may be built in a couple different ways: >>> make LLVM=1 >>> make CC=clang >>> >>> In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest >>> of lib.mk, and any Makefiles that include lib.mk, can base decisions >>> solely on whether or not LLVM is set. >> >> ICBW but I believe there are still some architectures with clang but not >> lld support where there's a use case for using CC=clang. > > I'm inclined to wait for those to make themselves known... :) > ...but thinking about this some more, maybe this patch is actually a Bad Idea. Because it is encouraging weirdness and divergence from how kbuild does it. And kbuild is very clear [1]: Building with LLVM Invoke make via: make LLVM=1 to compile for the host target. For cross compiling: make LLVM=1 ARCH=arm64 The LLVM= argument LLVM has substitutes for GNU binutils utilities. They can be enabled individually. The full list of supported make variables: make CC=clang LD=ld.lld AR=llvm-ar NM=llvm-nm STRIP=llvm-strip \ OBJCOPY=llvm-objcopy OBJDUMP=llvm-objdump READELF=llvm-readelf \ HOSTCC=clang HOSTCXX=clang++ HOSTAR=llvm-ar HOSTLD=ld.lld LLVM=1 expands to the above. [1] https://docs.kernel.org/kbuild/llvm.html thanks,
On Mon, Jun 03, 2024 at 04:32:30PM +0100, Mark Brown wrote: > On Fri, May 31, 2024 at 11:37:50AM -0700, John Hubbard wrote: > > The kselftests may be built in a couple different ways: > > make LLVM=1 > > make CC=clang > > > > In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest > > of lib.mk, and any Makefiles that include lib.mk, can base decisions > > solely on whether or not LLVM is set. > > ICBW but I believe there are still some architectures with clang but not > lld support where there's a use case for using CC=clang. Does CC=clang even work for the selftests? lib.mk here uses 'CC :=' so won't CC=clang get overridden to CC=$(CROSS_COMPILE)gcc? Cheers, Nathan
On 6/3/24 3:47 PM, Nathan Chancellor wrote: > On Mon, Jun 03, 2024 at 04:32:30PM +0100, Mark Brown wrote: >> On Fri, May 31, 2024 at 11:37:50AM -0700, John Hubbard wrote: >>> The kselftests may be built in a couple different ways: >>> make LLVM=1 >>> make CC=clang >>> >>> In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest >>> of lib.mk, and any Makefiles that include lib.mk, can base decisions >>> solely on whether or not LLVM is set. >> >> ICBW but I believe there are still some architectures with clang but not >> lld support where there's a use case for using CC=clang. > > Does CC=clang even work for the selftests? lib.mk here uses 'CC :=' so > won't CC=clang get overridden to CC=$(CROSS_COMPILE)gcc? > I received a report that someone (I forget who or what) was definitely attempting to just set CC=clang. But yes, it definitely doesn't work properly for CROSS_COMPILE. And the more we talk it through, the less I like this direction that I went off on. Let's just drop this patch and instead consider moving kselftest builds closer to kbuild, instead of making it more different. thanks,
On Mon, Jun 03, 2024 at 03:47:06PM -0700, Nathan Chancellor wrote: > On Mon, Jun 03, 2024 at 04:32:30PM +0100, Mark Brown wrote: > > ICBW but I believe there are still some architectures with clang but not > > lld support where there's a use case for using CC=clang. > Does CC=clang even work for the selftests? lib.mk here uses 'CC :=' so > won't CC=clang get overridden to CC=$(CROSS_COMPILE)gcc? No idea, it's not a configuration I'm interested in myself.
On 04/06/2024 05:55, John Hubbard wrote: > On 6/3/24 3:47 PM, Nathan Chancellor wrote: >> On Mon, Jun 03, 2024 at 04:32:30PM +0100, Mark Brown wrote: >>> On Fri, May 31, 2024 at 11:37:50AM -0700, John Hubbard wrote: >>>> The kselftests may be built in a couple different ways: >>>> make LLVM=1 >>>> make CC=clang >>>> >>>> In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest >>>> of lib.mk, and any Makefiles that include lib.mk, can base decisions >>>> solely on whether or not LLVM is set. >>> >>> ICBW but I believe there are still some architectures with clang but not >>> lld support where there's a use case for using CC=clang. >> >> Does CC=clang even work for the selftests? lib.mk here uses 'CC :=' so >> won't CC=clang get overridden to CC=$(CROSS_COMPILE)gcc? >> > > I received a report that someone (I forget who or what) was definitely > attempting to just set CC=clang. But yes, it definitely doesn't work > properly for CROSS_COMPILE. This history as I recall, is that I got a bug report [1] stating that: # tools/testing/selftests/fchmodat2$ make CC=clang and # tools/testing/selftests/openat2$ make CC=clang were both failing due to the -static-libsan / -static-libasan difference between gcc and clang. I attempted to fix that with [2], which used cc-option to determine which variant to use. That never got picked up, and John coincidentally did a similar fix, but relying on LLVM=1 instead. If we are concluding that CC=clang is an invalid way to do this, then I guess we should report that back to [1]? [1] https://lore.kernel.org/all/202404141807.LgsqXPY5-lkp@intel.com/ [2] https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/ Thanks, Ryan > > And the more we talk it through, the less I like this direction that > I went off on. Let's just drop this patch and instead consider moving > kselftest builds closer to kbuild, instead of making it more different. > > > thanks,
On Fri, Jun 07, 2024 at 12:12:19PM +0100, Ryan Roberts wrote: > On 04/06/2024 05:55, John Hubbard wrote: > > On 6/3/24 3:47 PM, Nathan Chancellor wrote: > >> Does CC=clang even work for the selftests? lib.mk here uses 'CC :=' so > >> won't CC=clang get overridden to CC=$(CROSS_COMPILE)gcc? > >> > > > > I received a report that someone (I forget who or what) was definitely > > attempting to just set CC=clang. But yes, it definitely doesn't work > > properly for CROSS_COMPILE. > > This history as I recall, is that I got a bug report [1] stating that: > > # tools/testing/selftests/fchmodat2$ make CC=clang > > and > > # tools/testing/selftests/openat2$ make CC=clang > > were both failing due to the -static-libsan / -static-libasan difference between > gcc and clang. I attempted to fix that with [2], which used cc-option to > determine which variant to use. That never got picked up, and John > coincidentally did a similar fix, but relying on LLVM=1 instead. > > If we are concluding that CC=clang is an invalid way to do this, then I guess we > should report that back to [1]? > > [1] https://lore.kernel.org/all/202404141807.LgsqXPY5-lkp@intel.com/ > [2] > https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/ I can only speak from the perspective of the main kernel build, as I don't really know much if anything about the selftests, but I think CC=clang and LLVM=1 should both be valid. Ideally, they would behave as they do for the main kernel build (i.e., CC=clang just uses clang for the compiler and LLVM=1 uses the entire LLVM tools). I realize that for the selftests, there is probably little use for tools other than the compiler, assembler, and linker but I think consistency is desirable here. I am not at all familiar with the selftests build system, which is completely different from Kbuild, but I would ack a patch that does that. Otherwise, I think having a different meaning or handling of CC=clang or LLVM=1 is the end of the world, but I do think that it should be documented. Cheers, Nathan
On 6/7/24 10:15 AM, Nathan Chancellor wrote: > On Fri, Jun 07, 2024 at 12:12:19PM +0100, Ryan Roberts wrote: >> On 04/06/2024 05:55, John Hubbard wrote: >>> On 6/3/24 3:47 PM, Nathan Chancellor wrote: ... >> If we are concluding that CC=clang is an invalid way to do this, then I guess we >> should report that back to [1]? >> >> [1] https://lore.kernel.org/all/202404141807.LgsqXPY5-lkp@intel.com/ >> [2] >> https://lore.kernel.org/linux-kselftest/20240417160740.2019530-1-ryan.roberts@arm.com/ > > I can only speak from the perspective of the main kernel build, as I > don't really know much if anything about the selftests, but I think > CC=clang and LLVM=1 should both be valid. Ideally, they would behave as > they do for the main kernel build (i.e., CC=clang just uses clang for > the compiler and LLVM=1 uses the entire LLVM tools). I realize that for > the selftests, there is probably little use for tools other than the > compiler, assembler, and linker but I think consistency is desirable > here. > > I am not at all familiar with the selftests build system, which is > completely different from Kbuild, but I would ack a patch that does > that. Otherwise, I think having a different meaning or handling of > CC=clang or LLVM=1 is the end of the world, but I do think that it > should be documented. > OK, that can be easily done, as shown below. And there are so far only a handful of selftests that key off of LLVM (plus a few of my pending patches). I can post this, plus a few patches (and patch updates for pending patches) to use the new CC_IS_CLANG where appropriate: diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 429535816dbd..ea643d1a65dc 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -43,6 +43,15 @@ else CC := $(CROSS_COMPILE)gcc endif # LLVM +# CC might have been set above (by inferring it from LLVM==1), or CC might have +# been set from the environment. In either case, if CC is an invocation of clang +# in any form, set CC_IS_CLANG. This allows subsystem selftests to selectively +# control clang-specific behavior, such as, in particular, compiler warnings. +CC_IS_CLANG := 0 +ifeq ($(findstring clang,$(CC)),clang) + CC_IS_CLANG := 1 +endif + ifeq (0,$(MAKELEVEL)) ifeq ($(OUTPUT),) OUTPUT := $(shell pwd) thanks,
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 429535816dbd..2902787b89b2 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -1,5 +1,17 @@ # This mimics the top-level Makefile. We do it explicitly here so that this # Makefile can operate with or without the kbuild infrastructure. + +# The kselftests may be built in a couple different ways: +# make LLVM=1 +# make CC=clang +# +# In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest of +# lib.mk, and any Makefiles that include lib.mk, can base decisions solely on +# whether or not LLVM is set. +ifeq ($(CC),clang) + LLVM := 1 +endif + ifneq ($(LLVM),) ifneq ($(filter %/,$(LLVM)),) LLVM_PREFIX := $(LLVM)
The kselftests may be built in a couple different ways: make LLVM=1 make CC=clang In order to handle both cases, set LLVM=1 if CC=clang. That way,the rest of lib.mk, and any Makefiles that include lib.mk, can base decisions solely on whether or not LLVM is set. Cc: Nathan Chancellor <nathan@kernel.org> Cc: Ryan Roberts <ryan.roberts@arm.com> Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- tools/testing/selftests/lib.mk | 12 ++++++++++++ 1 file changed, 12 insertions(+)