diff mbox series

[v2,1/2] selftests/lib.mk: handle both LLVM=1 and CC=clang builds

Message ID 20240531183751.100541-2-jhubbard@nvidia.com
State New
Headers show
Series selftests/lib.mk: LLVM=1, CC=clang, and warnings | expand

Commit Message

John Hubbard May 31, 2024, 6:37 p.m. UTC
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(+)

Comments

Mark Brown June 3, 2024, 3:32 p.m. UTC | #1
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.
John Hubbard June 3, 2024, 5:09 p.m. UTC | #2
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,
John Hubbard June 3, 2024, 8:11 p.m. UTC | #3
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,
Nathan Chancellor June 3, 2024, 10:47 p.m. UTC | #4
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
John Hubbard June 4, 2024, 4:55 a.m. UTC | #5
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,
Mark Brown June 4, 2024, 1:14 p.m. UTC | #6
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.
Ryan Roberts June 7, 2024, 11:12 a.m. UTC | #7
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,
Nathan Chancellor June 7, 2024, 5:15 p.m. UTC | #8
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
John Hubbard June 7, 2024, 8:13 p.m. UTC | #9
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 mbox series

Patch

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)