Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

warning: "__thumb2__" redefined #1772

Closed
nickdesaulniers opened this issue Dec 15, 2022 · 7 comments
Closed

warning: "__thumb2__" redefined #1772

nickdesaulniers opened this issue Dec 15, 2022 · 7 comments
Assignees
@nathanchance
Labels
[ARCH] arm32 This bug impacts ARCH=arm [BUG] linux A bug that should be fixed in the mainline kernel. [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle [Reported-by] KernelCI Reported-by: kernelci.org bot <bot@kernelci.org>

Comments

@nickdesaulniers
Copy link
Member

Even with https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=59e2cf8d21e05391c42628eb9fb5bb40f9d9698f landed, KernelCI is reporting warning: "__thumb2__" redefined but for GCC builds.

@nathanchance found some Debian specific GCC patches that might be related:
https://salsa.debian.org/toolchain-team/gcc/-/blob/master/debian/patches/arm-multilib-defaults.diff
https://salsa.debian.org/toolchain-team/gcc/-/blob/master/debian/rules2#L516
https://salsa.debian.org/toolchain-team/gcc/-/blob/master/debian/rules.defs#L478

@nathanchance suggested:

If that is the case, I am not sure how we can keep '-D__thumb2__=2'...
It is possible we should just remove 'thumb2' from arch/arm/crypto/
and replace it with 'CONFIG_THUMB2_KERNEL'; I am open to other ideas
though.

That's probably going to be the most portable solution. Having patches to GCC and Clang for one distribution complicates the support matrix significantly.
@nickdesaulniers nickdesaulniers added [ARCH] arm32 This bug impacts ARCH=arm [Reported-by] KernelCI Reported-by: kernelci.org bot <bot@kernelci.org> [BUG] Untriaged Something isn't working [BUG] linux A bug that should be fixed in the mainline kernel. labels Dec 15, 2022
@nathanchance
Copy link
Member

Thoughts on something like this? I am a little pressed for time at the moment so I will send a patch on Monday:

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 4067f5169144..c5fd0bcf1b5b 100644
--- a/arch/arm/Makefile
+++ b/arch/arm/Makefile
@@ -131,9 +131,8 @@ endif
 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
-CFLAGS_ISA	+=-mthumb
+CFLAGS_ISA	:=-mthumb -Wa,-mimplicit-it=always $(AFLAGS_NOWARN)
+AFLAGS_ISA	:=$(CFLAGS_ISA) -Wa$(comma)-mthumb
 else
 CFLAGS_ISA	:=$(call cc-option,-marm,) $(AFLAGS_NOWARN)
 AFLAGS_ISA	:=$(CFLAGS_ISA)
diff --git a/arch/arm/crypto/poly1305-armv4.pl b/arch/arm/crypto/poly1305-armv4.pl
index 6d79498d3115..d26aa785cab2 100644
--- a/arch/arm/crypto/poly1305-armv4.pl
+++ b/arch/arm/crypto/poly1305-armv4.pl
@@ -49,7 +49,7 @@ $code.=<<___;
 .globl	poly1305_blocks_neon
 #endif
 
-#if defined(__thumb2__)
+#if defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 .syntax	unified
 .thumb
 #else
@@ -77,7 +77,7 @@ poly1305_init:
 	str	r3,[$ctx,#36]		@ clear is_base2_26
 	add	$ctx,$ctx,#20
 
-#ifdef	__thumb2__
+#if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	it	eq
 #endif
 	moveq	r0,#0
@@ -124,7 +124,7 @@ poly1305_init:
 
 #if	__ARM_MAX_ARCH__>=7 && !defined(__KERNEL__)
 	tst	r12,#ARMV7_NEON		@ check for NEON
-# ifdef	__thumb2__
+# if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	adr	r9,.Lpoly1305_blocks_neon
 	adr	r11,.Lpoly1305_blocks
 	it	ne
@@ -235,7 +235,7 @@ poly1305_blocks:
 .Loop:
 #if __ARM_ARCH__<7
 	ldrb	r0,[lr],#16		@ load input
-# ifdef	__thumb2__
+# if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	it	hi
 # endif
 	addhi	$h4,$h4,#1		@ 1<<128
@@ -411,22 +411,22 @@ poly1305_emit:
 	adc	$g4,$h4,#0
 	tst	$g4,#4			@ did it carry/borrow?
 
-#ifdef	__thumb2__
+#if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	it	ne
 #endif
 	movne	$h0,$g0
 	ldr	$g0,[$nonce,#0]
-#ifdef	__thumb2__
+#if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	it	ne
 #endif
 	movne	$h1,$g1
 	ldr	$g1,[$nonce,#4]
-#ifdef	__thumb2__
+#if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	it	ne
 #endif
 	movne	$h2,$g2
 	ldr	$g2,[$nonce,#8]
-#ifdef	__thumb2__
+#if	defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	it	ne
 #endif
 	movne	$h3,$g3
diff --git a/arch/arm/crypto/sha256-armv4.pl b/arch/arm/crypto/sha256-armv4.pl
index f3a2b54efd4e..f7c660736a00 100644
--- a/arch/arm/crypto/sha256-armv4.pl
+++ b/arch/arm/crypto/sha256-armv4.pl
@@ -174,7 +174,7 @@ $code=<<___;
 .code	32
 #else
 .syntax unified
-# ifdef __thumb2__
+# if defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 .thumb
 # else
 .code   32
@@ -591,7 +591,7 @@ my $Ktbl="r3";
 $code.=<<___;
 #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__)
 
-# ifdef __thumb2__
+# if defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 #  define INST(a,b,c,d)	.byte	c,d|0xc,a,b
 # else
 #  define INST(a,b,c,d)	.byte	a,b,c,d
@@ -602,7 +602,7 @@ $code.=<<___;
 sha256_block_data_order_armv8:
 .LARMv8:
 	vld1.32	{$ABCD,$EFGH},[$ctx]
-# ifdef __thumb2__
+# if defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 	adr	$Ktbl,.LARMv8
 	sub	$Ktbl,$Ktbl,#.LARMv8-K256
 # else
diff --git a/arch/arm/crypto/sha512-armv4.pl b/arch/arm/crypto/sha512-armv4.pl
index 2fc3516912fa..bbb995eaa46f 100644
--- a/arch/arm/crypto/sha512-armv4.pl
+++ b/arch/arm/crypto/sha512-armv4.pl
@@ -211,7 +211,7 @@ $code=<<___;
 .code	32
 #else
 .syntax unified
-# ifdef __thumb2__
+# if defined(__thumb2__) || defined(CONFIG_THUMB2_KERNEL)
 .thumb
 # else
 .code   32

I kept the __thumb2__ defines just in case but we can likely full cut over for the kernel.

cc @ardbiesheuvel

@ardbiesheuvel
Copy link

Those .pl and generated .S files were intended to remain in sync between the OpenSSL project and the Linux kernel, so if we can avoid touching those, I'd be happier.

Given that those OpenSSL perlasm files are the only references to __thumb2__ that exist, could we fix it locally and drop the global (re)definition entirely?

diff --git a/arch/arm/Makefile b/arch/arm/Makefile
index 4067f5169144246b..955b0362cdfb7a47 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 971e74546fb1bf31..13a1ff40c3345487 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)

@nathanchance
Copy link
Member

That seems reasonable, everything else should be using CONFIG_THUMB2_KERNEL in the kernel. I can test and send that along if it works with your Suggested-by.

@nathanchance
Copy link
Member

Patch submitted, thanks as always Ard!

https://lore.kernel.org/20221222193039.2267074-1-nathan@kernel.org/

@nathanchance nathanchance added [PATCH] Submitted A patch has been submitted for review and removed [BUG] Untriaged Something isn't working labels Dec 22, 2022
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue Dec 22, 2022
@nathanchance @intel-lab-lkp
ARM: Reduce __thumb2__ definition to crypto files that require it
5af1d11 
Commit 1d2e9b6 ("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 59e2cf8 ("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: ClangBuiltLinux#1772
Reported-by: "kernelci.org bot" <bot@kernelci.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
@nathanchance
Copy link
Member

I submitted that patch to Russell's issue tracker: https://www.armlinux.org.uk/developer/patches/viewpatch.php?id=9287/1

@nathanchance
Copy link
Member

Accepted: http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?id=2f62847cf6ae49a54515421f67b1badffaa805f3

@nathanchance nathanchance added [PATCH] Accepted A submitted patch has been accepted upstream and removed [PATCH] Submitted A patch has been submitted for review labels Jan 23, 2023
@nathanchance nathanchance self-assigned this Jan 23, 2023
roxell pushed a commit to roxell/linux that referenced this issue Jan 24, 2023
@nathanchance
ARM: 9287/1: Reduce __thumb2__ definition to crypto files that requir…
2f62847 
…e it

Commit 1d2e9b6 ("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 59e2cf8 ("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: ClangBuiltLinux#1772

Reported-by: "kernelci.org bot" <bot@kernelci.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Fixes: 59e2cf8 ("ARM: 9275/1: Drop '-mthumb' from AFLAGS_ISA")
Fixes: 1d2e9b6 ("ARM: 9265/1: pass -march= only to compiler")
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
riteshharjani pushed a commit to riteshharjani/linux that referenced this issue Jan 31, 2023
@nathanchance @riteshharjani
ARM: 9287/1: Reduce __thumb2__ definition to crypto files that requir…
7e1b221 
…e it

Commit 1d2e9b6 ("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 59e2cf8 ("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: ClangBuiltLinux#1772

Reported-by: "kernelci.org bot" <bot@kernelci.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Fixes: 59e2cf8 ("ARM: 9275/1: Drop '-mthumb' from AFLAGS_ISA")
Fixes: 1d2e9b6 ("ARM: 9265/1: pass -march= only to compiler")
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
damien-lemoal pushed a commit to damien-lemoal/linux that referenced this issue Feb 5, 2023
@nathanchance
ARM: 9287/1: Reduce __thumb2__ definition to crypto files that requir…
b02ac6c 
…e it

Commit 1d2e9b6 ("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 59e2cf8 ("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: ClangBuiltLinux/linux#1772

Reported-by: "kernelci.org bot" <bot@kernelci.org>
Suggested-by: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Tested-by: Nick Desaulniers <ndesaulniers@google.com>
Fixes: 59e2cf8 ("ARM: 9275/1: Drop '-mthumb' from AFLAGS_ISA")
Fixes: 1d2e9b6 ("ARM: 9265/1: pass -march= only to compiler")
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
@nathanchance
Copy link
Member

Fixed: https://git.kernel.org/linus/2f62847cf6ae49a54515421f67b1badffaa805f3

@nathanchance nathanchance added [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle and removed [PATCH] Accepted A submitted patch has been accepted upstream labels Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nathanchance nathanchance

Labels
[ARCH] arm32 This bug impacts ARCH=arm [BUG] linux A bug that should be fixed in the mainline kernel. [FIXED][LINUX] development cycle This bug was only present and fixed in a -next or -rc cycle [Reported-by] KernelCI Reported-by: kernelci.org bot <bot@kernelci.org>
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nickdesaulniers @ardbiesheuvel @nathanchance