| Message ID | 20221114225719.1657174-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Drop '-mthumb' from AFLAGS_ISA | expand |
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 >
On Thu, Nov 17, 2022 at 11:15:09AM -0800, Nick Desaulniers wrote: > 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. Ack. > 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. Right; I suspect that it is unlikely that either of those entities will drop CROSS_COMPILE because they aim to work with multiple trees, which may still require CROSS_COMPILE. Maybe in five years when 5.15 is the oldest stable release that we support ;) > 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 Or perhaps a deep familiarity with the potential pitfalls of all this ;) > > 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. I would rather use an actual hash to reduce the risk of the link going stale from either a branch rename or file rename/removal. I can use a hash from the snapshot branch instead, if that would work for you? > If not, at least re-add the missing `t` from the protocol in the URL > (s/htps/https/). Oh whoops, good catch! > > 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> Thanks for the testing and review! I will send a v2 later today and submit it to Russell's patch tracker on Monday so that it can be picked up for -next. > --- > > 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? ;) At the very least, it is the second time; the first was https://github.com/ClangBuiltLinux/linux/issues/1355. > > --- > > 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 > > Cheers, Nathan
On Thu, Nov 17, 2022 at 11:34 AM Nathan Chancellor <nathan@kernel.org> wrote: > > On Thu, Nov 17, 2022 at 11:15:09AM -0800, Nick Desaulniers wrote: > > On Mon, Nov 14, 2022 at 2:58 PM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > 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. > > I would rather use an actual hash to reduce the risk of the link going > stale from either a branch rename or file rename/removal. I can use a > hash from the snapshot branch instead, if that would work for you? It doesn't matter much to me; I trust your judgement; you pick. Perhaps that depends if the snapshot branch has stable SHAs or whether they change over time? Maybe Sylvestre can comment on that.
Le 17/11/2022 à 23:51, Nick Desaulniers a écrit : > On Thu, Nov 17, 2022 at 11:34 AM Nathan Chancellor <nathan@kernel.org> wrote: >> >> On Thu, Nov 17, 2022 at 11:15:09AM -0800, Nick Desaulniers wrote: >>> On Mon, Nov 14, 2022 at 2:58 PM Nathan Chancellor <nathan@kernel.org> wrote: >>>> >>>> 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. >> >> I would rather use an actual hash to reduce the risk of the link going >> stale from either a branch rename or file rename/removal. I can use a >> hash from the snapshot branch instead, if that would work for you? > > It doesn't matter much to me; I trust your judgement; you pick. > Perhaps that depends if the snapshot branch has stable SHAs or whether > they change over time? Maybe Sylvestre can comment on that. Yeah, it can change overtime. I have to rebase them from time to time. Cheers, Sylvestre
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)
When building with CONFIG_THUMB2_KERNEL=y + a version of clang from Debian, the following warning occurs frequently: <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. 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 Reported-by: "kernelci.org bot" <bot@kernelci.org> Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- arch/arm/Makefile | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) base-commit: 0c52591d22e99759da3793f19249bbf45ad742bd