Messages in this thread Patch in this message | | | From | Nick Desaulniers <> | Date | Thu, 17 Nov 2022 11:15:09 -0800 | Subject | Re: [PATCH] ARM: Drop '-mthumb' from AFLAGS_ISA |
| |
On Mon, Nov 14, 2022 at 2:58 PM Nathan Chancellor <nathan@kernel.org> wrote: > > When building with CONFIG_THUMB2_KERNEL=y + a version of clang from > Debian, the following warning occurs frequently:
I also needed to explicitly set CROSS_COMPILE=arm-linux-gnueabihf- to reproduce. Please add that detail to the commit message. Thanks for helping spot that difference on IRC.
It sounds like tuxmake (which our CI is built on) and perhaps kernelCI are both setting that variable, which is no longer necessary when using LLVM=1 for ARCH=arm.
Not CROSS_COMPILE=arm-linux-gnueabi- like the triple we use by default for ARCH=arm in scripts/Makefile.clang. So this issue arises from: 1. using debian's clang, which is carrying an out of tree patch affecting this. 2. using `CROSS_COMPILE=arm-linux-gnueabihf-`.
The use of both of those in conjunction I'd like to think would be relatively unlikely, but it seems that we have both CI systems doing this (and the patch LGTM regardless of changing the CI).
> > <built-in>:383:9: warning: '__thumb2__' macro redefined [-Wmacro-redefined] > #define __thumb2__ 2 > ^ > <built-in>:353:9: note: previous definition is here > #define __thumb2__ 1 > ^ > 1 warning generated. > > Debian carries a downstream patch that changes the default CPU of the > arm-linux-gnueabihf target from 'arm1176jzf-s' (v6) to 'cortex-a7' (v7). > As a result, '-mthumb' defines both '__thumb__' and '__thumb2__'. The > define of '__thumb2__' via the command line was purposefully added to > catch a situation like this.
And we caught something! It's almost like Ard has sight-beyond-sight or something when he made that suggestion. Coincidence? I think not... :P
> > In a similar vein as commit 26b12e084bce ("ARM: 9264/1: only use > -mtp=cp15 for the compiler"), do not add '-mthumb' to AFLAGS_ISA, as it > is already passed to the assembler via '-Wa,-mthumb' and '__thumb2__' is > already defined for preprocessing. > > Fixes: 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler") > Link: htps://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/17354b030ac4252ff6c5e9d01f4eba28bd406b2d/debian/patches/930008-arm.diff
Would you mind using https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/blob/snapshot/debian/patches/930008-arm.diff as the link instead? The link on this commit message is a diff against llvm-14, not ToT which is currently llvm-16; the context is quite different as the logic moved source files completely. Though it does look like Sylvestre has not yet cut a 16 branch for debian's patches.
If not, at least re-add the missing `t` from the protocol in the URL (s/htps/https/).
> Reported-by: "kernelci.org bot" <bot@kernelci.org> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
I verified this locally with LLVM built from source, comparing no out of tree patches vs just debian's 930008-arm.diff applied.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com>
---
If memory serves, this is perhaps the third time downstream debian patches to llvm have caused us initially-difficult-to-reproduce bugs. Sylvestre, going forward, would you mind please giving your diff's more descriptive file names, or making them actual commits with some context in the commit message? Time and resource permitting, submitting them upstream, even if they're not accepted, but pointing to the upstream discussion (if any) from commit messages would provide us more context for these kind of things. Maybe Serge could help you burn down those out of tree patches? ;)
> --- > arch/arm/Makefile | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/Makefile b/arch/arm/Makefile > index 357f0d9b8607..d1ebb746ff40 100644 > --- a/arch/arm/Makefile > +++ b/arch/arm/Makefile > @@ -131,8 +131,9 @@ endif > AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W) > > ifeq ($(CONFIG_THUMB2_KERNEL),y) > -CFLAGS_ISA :=-mthumb -Wa,-mimplicit-it=always $(AFLAGS_NOWARN) > +CFLAGS_ISA :=-Wa,-mimplicit-it=always $(AFLAGS_NOWARN) > AFLAGS_ISA :=$(CFLAGS_ISA) -Wa$(comma)-mthumb -D__thumb2__=2 > +CFLAGS_ISA +=-mthumb > else > CFLAGS_ISA :=$(call cc-option,-marm,) $(AFLAGS_NOWARN) > AFLAGS_ISA :=$(CFLAGS_ISA) > > base-commit: 0c52591d22e99759da3793f19249bbf45ad742bd > -- > 2.38.1 >
-- Thanks, ~Nick Desaulniers
| |