| Message ID | 20210303183333.46543-1-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] kbuild: remove LLVM=1 test from HAS_LTO_CLANG | expand |
+ Sami On Wed, Mar 3, 2021 at 10:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > This guarding is wrong. As Documentation/kbuild/llvm.rst notes, LLVM=1 > switches the default of tools, but you can still override CC, LD, etc. > individually. > > BTW, LLVM is not 1/0 flag. If LLVM is not passed in, it is empty. Do we have the same problem with LLVM_IAS? LGTM otherwise, but wanted to check that before signing off. (Also, the rest of the patches in this series seem more related to DWARFv5 cleanups; this patch seems orthogonal while those are a visible progression). > > Non-zero return code is all treated as failure anyway. > > So, $(success,test $(LLVM) -eq 1) and $(success,test "$(LLVM)" = 1) > works equivalently in the sense that both are expanded to 'n' if LLVM > is not given. The difference is that the former internally fails due > to syntax error. > > $ test ${LLVM} -eq 1 > bash: test: -eq: unary operator expected > $ echo $? > 2 > > $ test "${LLVM}" -eq 1 > bash: test: : integer expression expected > $ echo $? > 2 > > $ test "${LLVM}" = 1 > echo $? > 1 > > $ test -n "${LLVM}" > $ echo $? > 1 > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > > arch/Kconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 2bb30673d8e6..2af10ebe5ed0 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -632,7 +632,6 @@ config HAS_LTO_CLANG > def_bool y > # Clang >= 11: https://github.com/ClangBuiltLinux/linux/issues/510 > depends on CC_IS_CLANG && CLANG_VERSION >= 110000 && LD_IS_LLD > - depends on $(success,test $(LLVM) -eq 1) IIRC, we needed some other LLVM utilities like llvm-nm and llvm-ar, which are checked below. So I guess we can still support CC=clang AR=llvm-ar NM=llvm-nm, and this check is redundant. > depends on $(success,test $(LLVM_IAS) -eq 1) > depends on $(success,$(NM) --help | head -n 1 | grep -qi llvm) > depends on $(success,$(AR) --help | head -n 1 | grep -qi llvm) > -- > 2.27.0 > -- Thanks, ~Nick Desaulniers
On Thu, Mar 04, 2021 at 03:33:30AM +0900, Masahiro Yamada wrote: > This guarding is wrong. As Documentation/kbuild/llvm.rst notes, LLVM=1 > switches the default of tools, but you can still override CC, LD, etc. > individually. > > BTW, LLVM is not 1/0 flag. If LLVM is not passed in, it is empty. > > Non-zero return code is all treated as failure anyway. > > So, $(success,test $(LLVM) -eq 1) and $(success,test "$(LLVM)" = 1) > works equivalently in the sense that both are expanded to 'n' if LLVM > is not given. The difference is that the former internally fails due > to syntax error. > > $ test ${LLVM} -eq 1 > bash: test: -eq: unary operator expected > $ echo $? > 2 > > $ test "${LLVM}" -eq 1 > bash: test: : integer expression expected > $ echo $? > 2 > > $ test "${LLVM}" = 1 > echo $? > 1 > > $ test -n "${LLVM}" > $ echo $? > 1 > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> Yes, there is not too much point of checking if $(LLVM) is set or not because we already check for the other tools. Reviewed-by: Nathan Chancellor <nathan@kernel.org> > --- > > arch/Kconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 2bb30673d8e6..2af10ebe5ed0 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -632,7 +632,6 @@ config HAS_LTO_CLANG > def_bool y > # Clang >= 11: https://github.com/ClangBuiltLinux/linux/issues/510 > depends on CC_IS_CLANG && CLANG_VERSION >= 110000 && LD_IS_LLD > - depends on $(success,test $(LLVM) -eq 1) > depends on $(success,test $(LLVM_IAS) -eq 1) > depends on $(success,$(NM) --help | head -n 1 | grep -qi llvm) > depends on $(success,$(AR) --help | head -n 1 | grep -qi llvm) > -- > 2.27.0 >
On Thu, Mar 4, 2021 at 5:47 AM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > + Sami > > On Wed, Mar 3, 2021 at 10:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > This guarding is wrong. As Documentation/kbuild/llvm.rst notes, LLVM=1 > > switches the default of tools, but you can still override CC, LD, etc. > > individually. > > > > BTW, LLVM is not 1/0 flag. If LLVM is not passed in, it is empty. > > Do we have the same problem with LLVM_IAS? LGTM otherwise, but wanted > to check that before signing off. 3/4 will replace this LLVM_IAS check with AS_IS_LLVM. We do not need to add a noise change. > > (Also, the rest of the patches in this series seem more related to > DWARFv5 cleanups; this patch seems orthogonal while those are a > visible progression). Kind of orthogonal, but I am touching the same code hunk, which would cause a merge conflict. > > > > Non-zero return code is all treated as failure anyway. > > > > So, $(success,test $(LLVM) -eq 1) and $(success,test "$(LLVM)" = 1) > > works equivalently in the sense that both are expanded to 'n' if LLVM > > is not given. The difference is that the former internally fails due > > to syntax error. > > > > $ test ${LLVM} -eq 1 > > bash: test: -eq: unary operator expected > > $ echo $? > > 2 > > > > $ test "${LLVM}" -eq 1 > > bash: test: : integer expression expected > > $ echo $? > > 2 > > > > $ test "${LLVM}" = 1 > > echo $? > > 1 > > > > $ test -n "${LLVM}" > > $ echo $? > > 1 > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > --- > > > > arch/Kconfig | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index 2bb30673d8e6..2af10ebe5ed0 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -632,7 +632,6 @@ config HAS_LTO_CLANG > > def_bool y > > # Clang >= 11: https://github.com/ClangBuiltLinux/linux/issues/510 > > depends on CC_IS_CLANG && CLANG_VERSION >= 110000 && LD_IS_LLD > > - depends on $(success,test $(LLVM) -eq 1) > > IIRC, we needed some other LLVM utilities like llvm-nm and llvm-ar, > which are checked below. So I guess we can still support CC=clang > AR=llvm-ar NM=llvm-nm, and this check is redundant. Yes, I think so. > > > depends on $(success,test $(LLVM_IAS) -eq 1) > > depends on $(success,$(NM) --help | head -n 1 | grep -qi llvm) > > depends on $(success,$(AR) --help | head -n 1 | grep -qi llvm) > > -- > > 2.27.0 > > > > > -- > Thanks, > ~Nick Desaulniers > > -- > You received this message because you are subscribed to the Google Groups "Clang Built Linux" group. > To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-linux+unsubscribe@googlegroups.com. > To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/CAKwvOdkhZGv_q9vgDdYY44OrbzmMD_E%2BGL3SyOk-jQ0kdXtMzg%40mail.gmail.com.
On Wed, Mar 3, 2021 at 12:47 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > + Sami > > On Wed, Mar 3, 2021 at 10:34 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > This guarding is wrong. As Documentation/kbuild/llvm.rst notes, LLVM=1 > > switches the default of tools, but you can still override CC, LD, etc. > > individually. > > > > BTW, LLVM is not 1/0 flag. If LLVM is not passed in, it is empty. > > Do we have the same problem with LLVM_IAS? LGTM otherwise, but wanted > to check that before signing off. > > (Also, the rest of the patches in this series seem more related to > DWARFv5 cleanups; this patch seems orthogonal while those are a > visible progression). > > > > > Non-zero return code is all treated as failure anyway. > > > > So, $(success,test $(LLVM) -eq 1) and $(success,test "$(LLVM)" = 1) > > works equivalently in the sense that both are expanded to 'n' if LLVM > > is not given. The difference is that the former internally fails due > > to syntax error. > > > > $ test ${LLVM} -eq 1 > > bash: test: -eq: unary operator expected > > $ echo $? > > 2 > > > > $ test "${LLVM}" -eq 1 > > bash: test: : integer expression expected > > $ echo $? > > 2 > > > > $ test "${LLVM}" = 1 > > echo $? > > 1 > > > > $ test -n "${LLVM}" > > $ echo $? > > 1 > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > --- > > > > arch/Kconfig | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index 2bb30673d8e6..2af10ebe5ed0 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -632,7 +632,6 @@ config HAS_LTO_CLANG > > def_bool y > > # Clang >= 11: https://github.com/ClangBuiltLinux/linux/issues/510 > > depends on CC_IS_CLANG && CLANG_VERSION >= 110000 && LD_IS_LLD > > - depends on $(success,test $(LLVM) -eq 1) > > IIRC, we needed some other LLVM utilities like llvm-nm and llvm-ar, > which are checked below. So I guess we can still support CC=clang > AR=llvm-ar NM=llvm-nm, and this check is redundant. I'm fine with removing the check, but the idea here was to just make it slightly harder for people to accidentally use a mismatched toolchain, even though checking for LLVM=1 doesn't stop them from doing so anyway. But yes, the only LLVM tools required in addition to the compiler and the linker are llvm-ar and llvm-nm. Sami
diff --git a/arch/Kconfig b/arch/Kconfig index 2bb30673d8e6..2af10ebe5ed0 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -632,7 +632,6 @@ config HAS_LTO_CLANG def_bool y # Clang >= 11: https://github.com/ClangBuiltLinux/linux/issues/510 depends on CC_IS_CLANG && CLANG_VERSION >= 110000 && LD_IS_LLD - depends on $(success,test $(LLVM) -eq 1) depends on $(success,test $(LLVM_IAS) -eq 1) depends on $(success,$(NM) --help | head -n 1 | grep -qi llvm) depends on $(success,$(AR) --help | head -n 1 | grep -qi llvm)
This guarding is wrong. As Documentation/kbuild/llvm.rst notes, LLVM=1 switches the default of tools, but you can still override CC, LD, etc. individually. BTW, LLVM is not 1/0 flag. If LLVM is not passed in, it is empty. Non-zero return code is all treated as failure anyway. So, $(success,test $(LLVM) -eq 1) and $(success,test "$(LLVM)" = 1) works equivalently in the sense that both are expanded to 'n' if LLVM is not given. The difference is that the former internally fails due to syntax error. $ test ${LLVM} -eq 1 bash: test: -eq: unary operator expected $ echo $? 2 $ test "${LLVM}" -eq 1 bash: test: : integer expression expected $ echo $? 2 $ test "${LLVM}" = 1 echo $? 1 $ test -n "${LLVM}" $ echo $? 1 Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- arch/Kconfig | 1 - 1 file changed, 1 deletion(-)