[PATCH] ARM: Reduce __thumb2__ definition to crypto files that require it

[PATCH] ARM: Reduce __thumb2__ definition to crypto files that require it

[Nathan Chancellor]

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(-)

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

Re: [PATCH] ARM: Reduce __thumb2__ definition to crypto files that require it

[Nick Desaulniers]

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
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] ARM: Reduce __thumb2__ definition to crypto files that require it

[Russell King (Oracle)]

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?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] ARM: Reduce __thumb2__ definition to crypto files that require it

[Nathan Chancellor]

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