| Message ID | 20221222193039.2267074-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: Reduce __thumb2__ definition to crypto files that require it | expand |
On Thu, Dec 22, 2022 at 11:30 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Commit 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler") added > a __thumb2__ define to ASFLAGS to avoid build errors in the crypto code, > which relies on __thumb2__ for preprocessing. Commit 59e2cf8d21e0 ("ARM: > 9275/1: Drop '-mthumb' from AFLAGS_ISA") followed up on this by removing > -mthumb from AFLAGS so that __thumb2__ would not be defined when the > default target was ARMv7 or newer. > > Unfortunately, the second commit's fix assumes that the toolchain > defaults to -mno-thumb / -marm, which is not the case for Debian's > arm-linux-gnueabihf target, which defaults to -mthumb: > > $ echo | arm-linux-gnueabihf-gcc -dM -E - | grep __thumb > #define __thumb2__ 1 > #define __thumb__ 1 Interesting, that was hard to foresee in review of 1d2e9b67b001 and 59e2cf8d21e0. FWIW, their non-hf target does not. $ echo | arm-linux-gnueabi-gcc -dM -E - | grep __thumb $ > > This target is used by several CI systems, which will still see > redefined macro warnings, despite '-mthumb' not being present in the > flags: > > <command-line>: warning: "__thumb2__" redefined > <built-in>: note: this is the location of the previous definition > > Remove the global AFLAGS __thumb2__ define and move it to the crypto > folder where it is required by the imported OpenSSL algorithms; the rest > of the kernel should use the internal CONFIG_THUMB2_KERNEL symbol to > know whether or not Thumb2 is being used or not. Be sure that __thumb2__ > is undefined first so that there are no macro redefinition warnings. > > Link: https://github.com/ClangBuiltLinux/linux/issues/1772 > Reported-by: "kernelci.org bot" <bot@kernelci.org> > Suggested-by: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> Thanks Nathan and Ard. I ran the following tests: 1. build w/ llvm stock config CONFIG_THUMB2_KERNEL=y kernel. 2. build w/ llvm+debian's OOT patch https://salsa.debian.org/pkg-llvm-team/llvm-toolchain/-/raw/622dbcbd40b316ed3905a2d25d9623544a06e6b1/debian/patches/930008-arm.diff CONFIG_THUMB2_KERNEL=y kernel. 3. check that -U<foo> removes <foo> from being set by preprocessor for both debian's arm-linux-gnueabihf-gcc and clang -mthumb $ echo | arm-linux-gnueabihf-gcc -dM -E - | grep __thumb #define __thumb2__ 1 #define __thumb__ 1 $ echo | arm-linux-gnueabihf-gcc -dM -E - -U__thumb2__ | grep __thumb #define __thumb__ 1 $ echo | clang --target=arm-linux-gnueabihf -mthumb -dM -E - -mcpu=cortex-a7 | grep __thumb #define __thumb2__ 1 #define __thumb__ 1 $ echo | clang --target=arm-linux-gnueabihf -mthumb -dM -E - -mcpu=cortex-a7 -U__thumb2__ | grep __thumb #define __thumb__ 1 Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> > --- > arch/arm/Makefile | 2 +- > arch/arm/crypto/Makefile | 7 ++++++- > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/Makefile b/arch/arm/Makefile > index 4067f5169144..955b0362cdfb 100644 > --- a/arch/arm/Makefile > +++ b/arch/arm/Makefile > @@ -132,7 +132,7 @@ AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W) > > ifeq ($(CONFIG_THUMB2_KERNEL),y) > CFLAGS_ISA :=-Wa,-mimplicit-it=always $(AFLAGS_NOWARN) > -AFLAGS_ISA :=$(CFLAGS_ISA) -Wa$(comma)-mthumb -D__thumb2__=2 > +AFLAGS_ISA :=$(CFLAGS_ISA) -Wa$(comma)-mthumb > CFLAGS_ISA +=-mthumb > else > CFLAGS_ISA :=$(call cc-option,-marm,) $(AFLAGS_NOWARN) > diff --git a/arch/arm/crypto/Makefile b/arch/arm/crypto/Makefile > index 971e74546fb1..13e62c7c25dc 100644 > --- a/arch/arm/crypto/Makefile > +++ b/arch/arm/crypto/Makefile > @@ -53,7 +53,12 @@ $(obj)/%-core.S: $(src)/%-armv4.pl > > clean-files += poly1305-core.S sha256-core.S sha512-core.S > > +aflags-thumb2-$(CONFIG_THUMB2_KERNEL) := -U__thumb2__ -D__thumb2__=1 > + > +AFLAGS_sha256-core.o += $(aflags-thumb2-y) > +AFLAGS_sha512-core.o += $(aflags-thumb2-y) > + > # massage the perlasm code a bit so we only get the NEON routine if we need it > poly1305-aflags-$(CONFIG_CPU_V7) := -U__LINUX_ARM_ARCH__ -D__LINUX_ARM_ARCH__=5 > poly1305-aflags-$(CONFIG_KERNEL_MODE_NEON) := -U__LINUX_ARM_ARCH__ -D__LINUX_ARM_ARCH__=7 > -AFLAGS_poly1305-core.o += $(poly1305-aflags-y) > +AFLAGS_poly1305-core.o += $(poly1305-aflags-y) $(aflags-thumb2-y) > > base-commit: 71a7507afbc3f27c346898f13ab9bfd918613c34 > -- > 2.39.0 >
On Thu, Dec 22, 2022 at 11:52:27AM -0800, Nick Desaulniers wrote: > On Thu, Dec 22, 2022 at 11:30 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > Commit 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler") added > > a __thumb2__ define to ASFLAGS to avoid build errors in the crypto code, > > which relies on __thumb2__ for preprocessing. Commit 59e2cf8d21e0 ("ARM: > > 9275/1: Drop '-mthumb' from AFLAGS_ISA") followed up on this by removing > > -mthumb from AFLAGS so that __thumb2__ would not be defined when the > > default target was ARMv7 or newer. > > > > Unfortunately, the second commit's fix assumes that the toolchain > > defaults to -mno-thumb / -marm, which is not the case for Debian's > > arm-linux-gnueabihf target, which defaults to -mthumb: > > > > $ echo | arm-linux-gnueabihf-gcc -dM -E - | grep __thumb > > #define __thumb2__ 1 > > #define __thumb__ 1 > > Interesting, that was hard to foresee in review of 1d2e9b67b001 and > 59e2cf8d21e0. > > FWIW, their non-hf target does not. > $ echo | arm-linux-gnueabi-gcc -dM -E - | grep __thumb > $ > > > > > This target is used by several CI systems, which will still see > > redefined macro warnings, despite '-mthumb' not being present in the > > flags: > > > > <command-line>: warning: "__thumb2__" redefined > > <built-in>: note: this is the location of the previous definition > > > > Remove the global AFLAGS __thumb2__ define and move it to the crypto > > folder where it is required by the imported OpenSSL algorithms; the rest > > of the kernel should use the internal CONFIG_THUMB2_KERNEL symbol to > > know whether or not Thumb2 is being used or not. Be sure that __thumb2__ > > is undefined first so that there are no macro redefinition warnings. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1772 > > Reported-by: "kernelci.org bot" <bot@kernelci.org> > > Suggested-by: Ard Biesheuvel <ardb@kernel.org> > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > Thanks Nathan and Ard. Shouldn't this also have a fixes tag?
On Mon, Jan 23, 2023 at 10:48:24AM +0000, Russell King (Oracle) wrote: > On Thu, Dec 22, 2022 at 11:52:27AM -0800, Nick Desaulniers wrote: > > On Thu, Dec 22, 2022 at 11:30 AM Nathan Chancellor <nathan@kernel.org> wrote: > > > > > > Commit 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler") added > > > a __thumb2__ define to ASFLAGS to avoid build errors in the crypto code, > > > which relies on __thumb2__ for preprocessing. Commit 59e2cf8d21e0 ("ARM: > > > 9275/1: Drop '-mthumb' from AFLAGS_ISA") followed up on this by removing > > > -mthumb from AFLAGS so that __thumb2__ would not be defined when the > > > default target was ARMv7 or newer. > > > > > > Unfortunately, the second commit's fix assumes that the toolchain > > > defaults to -mno-thumb / -marm, which is not the case for Debian's > > > arm-linux-gnueabihf target, which defaults to -mthumb: > > > > > > $ echo | arm-linux-gnueabihf-gcc -dM -E - | grep __thumb > > > #define __thumb2__ 1 > > > #define __thumb__ 1 > > > > Interesting, that was hard to foresee in review of 1d2e9b67b001 and > > 59e2cf8d21e0. > > > > FWIW, their non-hf target does not. > > $ echo | arm-linux-gnueabi-gcc -dM -E - | grep __thumb > > $ > > > > > > > > This target is used by several CI systems, which will still see > > > redefined macro warnings, despite '-mthumb' not being present in the > > > flags: > > > > > > <command-line>: warning: "__thumb2__" redefined > > > <built-in>: note: this is the location of the previous definition > > > > > > Remove the global AFLAGS __thumb2__ define and move it to the crypto > > > folder where it is required by the imported OpenSSL algorithms; the rest > > > of the kernel should use the internal CONFIG_THUMB2_KERNEL symbol to > > > know whether or not Thumb2 is being used or not. Be sure that __thumb2__ > > > is undefined first so that there are no macro redefinition warnings. > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1772 > > > Reported-by: "kernelci.org bot" <bot@kernelci.org> > > > Suggested-by: Ard Biesheuvel <ardb@kernel.org> > > > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > > > > Thanks Nathan and Ard. > > Shouldn't this also have a fixes tag? Ugh, yes, sorry about that :( would you kind taking these when you apply the patch or would you like me to resubmit? Fixes: 59e2cf8d21e0 ("ARM: 9275/1: Drop '-mthumb' from AFLAGS_ISA") Fixes: 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler") Cheers, Nathan
diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 4067f5169144..955b0362cdfb 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -132,7 +132,7 @@ AFLAGS_NOWARN :=$(call as-option,-Wa$(comma)-mno-warn-deprecated,-Wa$(comma)-W) ifeq ($(CONFIG_THUMB2_KERNEL),y) CFLAGS_ISA :=-Wa,-mimplicit-it=always $(AFLAGS_NOWARN) -AFLAGS_ISA :=$(CFLAGS_ISA) -Wa$(comma)-mthumb -D__thumb2__=2 +AFLAGS_ISA :=$(CFLAGS_ISA) -Wa$(comma)-mthumb CFLAGS_ISA +=-mthumb else CFLAGS_ISA :=$(call cc-option,-marm,) $(AFLAGS_NOWARN) diff --git a/arch/arm/crypto/Makefile b/arch/arm/crypto/Makefile index 971e74546fb1..13e62c7c25dc 100644 --- a/arch/arm/crypto/Makefile +++ b/arch/arm/crypto/Makefile @@ -53,7 +53,12 @@ $(obj)/%-core.S: $(src)/%-armv4.pl clean-files += poly1305-core.S sha256-core.S sha512-core.S +aflags-thumb2-$(CONFIG_THUMB2_KERNEL) := -U__thumb2__ -D__thumb2__=1 + +AFLAGS_sha256-core.o += $(aflags-thumb2-y) +AFLAGS_sha512-core.o += $(aflags-thumb2-y) + # massage the perlasm code a bit so we only get the NEON routine if we need it poly1305-aflags-$(CONFIG_CPU_V7) := -U__LINUX_ARM_ARCH__ -D__LINUX_ARM_ARCH__=5 poly1305-aflags-$(CONFIG_KERNEL_MODE_NEON) := -U__LINUX_ARM_ARCH__ -D__LINUX_ARM_ARCH__=7 -AFLAGS_poly1305-core.o += $(poly1305-aflags-y) +AFLAGS_poly1305-core.o += $(poly1305-aflags-y) $(aflags-thumb2-y)
Commit 1d2e9b67b001 ("ARM: 9265/1: pass -march= only to compiler") added a __thumb2__ define to ASFLAGS to avoid build errors in the crypto code, which relies on __thumb2__ for preprocessing. Commit 59e2cf8d21e0 ("ARM: 9275/1: Drop '-mthumb' from AFLAGS_ISA") followed up on this by removing -mthumb from AFLAGS so that __thumb2__ would not be defined when the default target was ARMv7 or newer. Unfortunately, the second commit's fix assumes that the toolchain defaults to -mno-thumb / -marm, which is not the case for Debian's arm-linux-gnueabihf target, which defaults to -mthumb: $ echo | arm-linux-gnueabihf-gcc -dM -E - | grep __thumb #define __thumb2__ 1 #define __thumb__ 1 This target is used by several CI systems, which will still see redefined macro warnings, despite '-mthumb' not being present in the flags: <command-line>: warning: "__thumb2__" redefined <built-in>: note: this is the location of the previous definition Remove the global AFLAGS __thumb2__ define and move it to the crypto folder where it is required by the imported OpenSSL algorithms; the rest of the kernel should use the internal CONFIG_THUMB2_KERNEL symbol to know whether or not Thumb2 is being used or not. Be sure that __thumb2__ is undefined first so that there are no macro redefinition warnings. Link: https://github.com/ClangBuiltLinux/linux/issues/1772 Reported-by: "kernelci.org bot" <bot@kernelci.org> Suggested-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- arch/arm/Makefile | 2 +- arch/arm/crypto/Makefile | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) base-commit: 71a7507afbc3f27c346898f13ab9bfd918613c34