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.
Then, build upon that to disable a pair of clang warnings that are already silenced on gcc.
Doing it this way is much better than the piecemeal approach that I started with in [1] and [2]. Thanks to Nathan Chancellor for the patch reviews that led to this approach.
Changes since the first version:
1) Wrote a detailed explanation for suppressing two clang warnings, in both a lib.mk comment, and the commit description.
2) Added a Reviewed-by tag to the first patch.
[1] https://lore.kernel.org/20240527214704.300444-1-jhubbard@nvidia.com [2] https://lore.kernel.org/20240527213641.299458-1-jhubbard@nvidia.com
John Hubbard (2): selftests/lib.mk: handle both LLVM=1 and CC=clang builds selftests/lib.mk: silence some clang warnings that gcc already ignores
tools/testing/selftests/lib.mk | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
base-commit: e0cce98fe279b64f4a7d81b7f5c3a23d80b92fbc
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(+)
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)
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 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.robert...
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.robert...
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.robert...
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,
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.
gcc defaults to silence (off) for the following warnings, but clang defaults to the opposite. The warnings are not useful for the kernel itself, which is why they have remained disabled in gcc for the main kernel build. And it is only due to including kernel data structures in the selftests, that we get the warnings from clang.
-Waddress-of-packed-member -Wgnu-variable-sized-type-not-at-end
In other words, the warnings are not unique to the selftests: there is nothing that the selftests' code does that triggers these warnings, other than the act of including the kernel's data structures. Therefore, silence them for the clang builds as well.
This eliminates warnings for the net/ and user_events/ kselftest subsystems, in these files:
./net/af_unix/scm_rights.c ./net/timestamping.c ./net/ipsec.c ./user_events/perf_test.c
Cc: Nathan Chancellor nathan@kernel.org Signed-off-by: John Hubbard jhubbard@nvidia.com --- tools/testing/selftests/lib.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 2902787b89b2..c179c02281e9 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -50,6 +50,14 @@ else CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) endif # CROSS_COMPILE
+# gcc defaults to silence (off) for the following warnings, but clang defaults +# to the opposite. The warnings are not useful for the kernel itself, which is +# why they have remained disabled in gcc for the main kernel build. And it is +# only due to including kernel data structures in the selftests, that we get the +# warnings from clang. Therefore, disable the warnings for clang builds. +CFLAGS += -Wno-address-of-packed-member +CFLAGS += -Wno-gnu-variable-sized-type-not-at-end + CC := $(CLANG) $(CLANG_FLAGS) -fintegrated-as else CC := $(CROSS_COMPILE)gcc
On 5/31/24 11:37 AM, John Hubbard wrote:
gcc defaults to silence (off) for the following warnings, but clang defaults to the opposite. The warnings are not useful for the kernel itself, which is why they have remained disabled in gcc for the main kernel build. And it is only due to including kernel data structures in the selftests, that we get the warnings from clang.
-Waddress-of-packed-member -Wgnu-variable-sized-type-not-at-end
Even if patch 1/1 here is not merged, I would still like to get this one reviewed and merged. It still solves the problem for LLVM=1 builds.
thanks,
On Fri, May 31, 2024 at 11:37:51AM -0700, John Hubbard wrote:
gcc defaults to silence (off) for the following warnings, but clang defaults to the opposite. The warnings are not useful for the kernel itself, which is why they have remained disabled in gcc for the main kernel build. And it is only due to including kernel data structures in the selftests, that we get the warnings from clang.
-Waddress-of-packed-member -Wgnu-variable-sized-type-not-at-endIn other words, the warnings are not unique to the selftests: there is nothing that the selftests' code does that triggers these warnings, other than the act of including the kernel's data structures. Therefore, silence them for the clang builds as well.
This eliminates warnings for the net/ and user_events/ kselftest subsystems, in these files:
./net/af_unix/scm_rights.c ./net/timestamping.c ./net/ipsec.c ./user_events/perf_test.cCc: Nathan Chancellor nathan@kernel.org Signed-off-by: John Hubbard jhubbard@nvidia.com
This seems reasonable to me.
Acked-by: Nathan Chancellor nathan@kernel.org
tools/testing/selftests/lib.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 2902787b89b2..c179c02281e9 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -50,6 +50,14 @@ else CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) endif # CROSS_COMPILE +# gcc defaults to silence (off) for the following warnings, but clang defaults +# to the opposite. The warnings are not useful for the kernel itself, which is +# why they have remained disabled in gcc for the main kernel build. And it is +# only due to including kernel data structures in the selftests, that we get the +# warnings from clang. Therefore, disable the warnings for clang builds. +CFLAGS += -Wno-address-of-packed-member +CFLAGS += -Wno-gnu-variable-sized-type-not-at-end
CC := $(CLANG) $(CLANG_FLAGS) -fintegrated-as else CC := $(CROSS_COMPILE)gcc -- 2.45.1
On 6/3/24 16:36, Nathan Chancellor wrote:
On Fri, May 31, 2024 at 11:37:51AM -0700, John Hubbard wrote:
gcc defaults to silence (off) for the following warnings, but clang defaults to the opposite. The warnings are not useful for the kernel itself, which is why they have remained disabled in gcc for the main kernel build. And it is only due to including kernel data structures in the selftests, that we get the warnings from clang.
-Waddress-of-packed-member -Wgnu-variable-sized-type-not-at-endIn other words, the warnings are not unique to the selftests: there is nothing that the selftests' code does that triggers these warnings, other than the act of including the kernel's data structures. Therefore, silence them for the clang builds as well.
This eliminates warnings for the net/ and user_events/ kselftest subsystems, in these files:
./net/af_unix/scm_rights.c ./net/timestamping.c ./net/ipsec.c ./user_events/perf_test.cCc: Nathan Chancellor nathan@kernel.org Signed-off-by: John Hubbard jhubbard@nvidia.com
This seems reasonable to me.
Acked-by: Nathan Chancellor nathan@kernel.org
tools/testing/selftests/lib.mk | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 2902787b89b2..c179c02281e9 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -50,6 +50,14 @@ else CLANG_FLAGS += --target=$(notdir $(CROSS_COMPILE:%-=%)) endif # CROSS_COMPILE +# gcc defaults to silence (off) for the following warnings, but clang defaults +# to the opposite. The warnings are not useful for the kernel itself, which is +# why they have remained disabled in gcc for the main kernel build. And it is +# only due to including kernel data structures in the selftests, that we get the +# warnings from clang. Therefore, disable the warnings for clang builds. +CFLAGS += -Wno-address-of-packed-member +CFLAGS += -Wno-gnu-variable-sized-type-not-at-end
Thank you for adding this comment block.
CC := $(CLANG) $(CLANG_FLAGS) -fintegrated-as else CC := $(CROSS_COMPILE)gcc -- 2.45.1
Thank you both. I will apply this for the next release. I want this change soaking in next for a bit.
thanks, -- Shuah
linux-kselftest-mirror@lists.linaro.org
-
John Hubbard -
Mark Brown -
Nathan Chancellor -
Ryan Roberts -
Shuah Khan