diff mbox

arm: simplify handling of Thumb related options

Message ID 1458493115-26904-1-git-send-email-thomas.petazzoni@free-electrons.com
State New
Headers show

Commit Message

Thomas Petazzoni March 20, 2016, 4:58 p.m. UTC
Currently, the Thumb support on ARM has three related Config.in
options, which are not trivial for users to understand, and are in
fact not needed:

 - The USE_BX option is not needed: knowing whether BX is available or
   not is easy. If you have an ARM > v4 or ARMv4T, then BX is
   available, otherwise it's not. This is the logic used in glibc.

 - The USE_LDREXSTREX option is not needed: whenever Thumb2 is
   available, ldrex/strex are available, so we can simply rely on
   __thumb2__ to determine whether ldrex/strex should be used, without
   requiring a Config.in option.

 - Once USE_BX and USE_LDREXSTREX are removed, the only thing left
   that COMPILE_IN_THUMB does is to set -mthumb. This makes the option
   unnecessary, as on ARM at least, the user is already supposed to
   pass -march=<foo> or other compiler options tuning the library for
   a specific ARM variant. There is no reason to do otherwise for
   Thumb, which allows to get rid of the COMPILE_IN_THUMB option.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 Rules.mak                                          |  1 -
 extra/Configs/Config.arm                           | 22 ----------------------
 libc/sysdeps/linux/arm/bits/arm_bx.h               | 10 ++++------
 libc/sysdeps/linux/arm/clone.S                     |  2 +-
 .../linuxthreads.old/sysdeps/arm/pt-machine.h      |  7 +++----
 5 files changed, 8 insertions(+), 34 deletions(-)

Comments

Rich Felker March 20, 2016, 5:40 p.m. UTC | #1
On Sun, Mar 20, 2016 at 05:58:35PM +0100, Thomas Petazzoni wrote:
> Currently, the Thumb support on ARM has three related Config.in
> options, which are not trivial for users to understand, and are in
> fact not needed:
> 
>  - The USE_BX option is not needed: knowing whether BX is available or
>    not is easy. If you have an ARM > v4 or ARMv4T, then BX is
>    available, otherwise it's not. This is the logic used in glibc.

This sounds correct.

>  - The USE_LDREXSTREX option is not needed: whenever Thumb2 is
>    available, ldrex/strex are available, so we can simply rely on
>    __thumb2__ to determine whether ldrex/strex should be used, without
>    requiring a Config.in option.

__thumb2__ is sufficient for ldrex/strex but not sufficient; v6 has
them in the arm isa. Or does logic only apply when you've selected to
use thumb?

Rich
Thomas Petazzoni March 20, 2016, 5:47 p.m. UTC | #2
Hello,

Thanks for your feedback!

On Sun, 20 Mar 2016 13:40:40 -0400, Rich Felker wrote:
> On Sun, Mar 20, 2016 at 05:58:35PM +0100, Thomas Petazzoni wrote:
> > Currently, the Thumb support on ARM has three related Config.in
> > options, which are not trivial for users to understand, and are in
> > fact not needed:
> > 
> >  - The USE_BX option is not needed: knowing whether BX is available or
> >    not is easy. If you have an ARM > v4 or ARMv4T, then BX is
> >    available, otherwise it's not. This is the logic used in glibc.
> 
> This sounds correct.
> 
> >  - The USE_LDREXSTREX option is not needed: whenever Thumb2 is
> >    available, ldrex/strex are available, so we can simply rely on
> >    __thumb2__ to determine whether ldrex/strex should be used, without
> >    requiring a Config.in option.
> 
> __thumb2__ is sufficient for ldrex/strex but not sufficient; v6 has
> them in the arm isa.

I'm not sure what you mean between:

 (1) ldrex/strex are also available on plain ARMv6 (i.e not Thumb2).

 (2) ldrex/strex are not available on ARMv6, while some ARMv6 support
     Thumb2.

If you mean (1), then before my patch, the ldrex/strex code was only
enabled when USE_LDREXSTREX was enabled, and this option had a "depends
on COMPILE_IN_THUMB". Hence my patch doesn't change the fact that
the ldrex/strex logic is only used on Thumb. Whether it can/should be
enabled in non-Thumb configurations is a separate matter from what my
patch is doing.

If you mean (2), then there is indeed a problem. Though, according to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/14979.html,
"On a v6 or later core (ARM11 onwards) you should use the LDREX/STREX
instructions instead", which would suggest that such instructions are
available.

Best regards,

Thomas
Rich Felker March 20, 2016, 6:36 p.m. UTC | #3
On Sun, Mar 20, 2016 at 06:47:04PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> Thanks for your feedback!
> 
> On Sun, 20 Mar 2016 13:40:40 -0400, Rich Felker wrote:
> > On Sun, Mar 20, 2016 at 05:58:35PM +0100, Thomas Petazzoni wrote:
> > > Currently, the Thumb support on ARM has three related Config.in
> > > options, which are not trivial for users to understand, and are in
> > > fact not needed:
> > > 
> > >  - The USE_BX option is not needed: knowing whether BX is available or
> > >    not is easy. If you have an ARM > v4 or ARMv4T, then BX is
> > >    available, otherwise it's not. This is the logic used in glibc.
> > 
> > This sounds correct.
> > 
> > >  - The USE_LDREXSTREX option is not needed: whenever Thumb2 is
> > >    available, ldrex/strex are available, so we can simply rely on
> > >    __thumb2__ to determine whether ldrex/strex should be used, without
> > >    requiring a Config.in option.
> > 
> > __thumb2__ is sufficient for ldrex/strex but not sufficient; v6 has
> > them in the arm isa.
> 
> I'm not sure what you mean between:
> 
>  (1) ldrex/strex are also available on plain ARMv6 (i.e not Thumb2).
> 
>  (2) ldrex/strex are not available on ARMv6, while some ARMv6 support
>      Thumb2.

(1) is what I mean.

> If you mean (1), then before my patch, the ldrex/strex code was only
> enabled when USE_LDREXSTREX was enabled, and this option had a "depends
> on COMPILE_IN_THUMB". Hence my patch doesn't change the fact that
> the ldrex/strex logic is only used on Thumb. Whether it can/should be
> enabled in non-Thumb configurations is a separate matter from what my
> patch is doing.

OK, then the logic was wrong before, too. :-)

> If you mean (2), then there is indeed a problem. Though, according to
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/14979.html,
> "On a v6 or later core (ARM11 onwards) you should use the LDREX/STREX
> instructions instead", which would suggest that such instructions are
> available.

Indeed, and it's further complicated by the fact that none of these
instructions are proper memory barriers so they're basically useless
without a separate barrier instruction, which does not exist on
pre-v6, and on v6, only as a coprocessor operation that's not
available in thumb1 or on uc models. Not until v7 did arm have proper
working atomics. :(

Rich
Thomas Petazzoni March 20, 2016, 8:32 p.m. UTC | #4
Hello,

On Sun, 20 Mar 2016 14:36:05 -0400, Rich Felker wrote:

> > If you mean (1), then before my patch, the ldrex/strex code was only
> > enabled when USE_LDREXSTREX was enabled, and this option had a "depends
> > on COMPILE_IN_THUMB". Hence my patch doesn't change the fact that
> > the ldrex/strex logic is only used on Thumb. Whether it can/should be
> > enabled in non-Thumb configurations is a separate matter from what my
> > patch is doing.
> 
> OK, then the logic was wrong before, too. :-)

Right, but my patch is not making it worse than it was.

> > If you mean (2), then there is indeed a problem. Though, according to
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/14979.html,
> > "On a v6 or later core (ARM11 onwards) you should use the LDREX/STREX
> > instructions instead", which would suggest that such instructions are
> > available.
> 
> Indeed, and it's further complicated by the fact that none of these
> instructions are proper memory barriers so they're basically useless
> without a separate barrier instruction, which does not exist on
> pre-v6, and on v6, only as a coprocessor operation that's not
> available in thumb1 or on uc models. Not until v7 did arm have proper
> working atomics. :(

Sure. But here as well, my patch is not making things worse I believe.

Best regards,

Thomas
Rich Felker March 21, 2016, 12:55 a.m. UTC | #5
On Sun, Mar 20, 2016 at 09:32:44PM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Sun, 20 Mar 2016 14:36:05 -0400, Rich Felker wrote:
> 
> > > If you mean (1), then before my patch, the ldrex/strex code was only
> > > enabled when USE_LDREXSTREX was enabled, and this option had a "depends
> > > on COMPILE_IN_THUMB". Hence my patch doesn't change the fact that
> > > the ldrex/strex logic is only used on Thumb. Whether it can/should be
> > > enabled in non-Thumb configurations is a separate matter from what my
> > > patch is doing.
> > 
> > OK, then the logic was wrong before, too. :-)
> 
> Right, but my patch is not making it worse than it was.
> 
> > > If you mean (2), then there is indeed a problem. Though, according to
> > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/14979.html,
> > > "On a v6 or later core (ARM11 onwards) you should use the LDREX/STREX
> > > instructions instead", which would suggest that such instructions are
> > > available.
> > 
> > Indeed, and it's further complicated by the fact that none of these
> > instructions are proper memory barriers so they're basically useless
> > without a separate barrier instruction, which does not exist on
> > pre-v6, and on v6, only as a coprocessor operation that's not
> > available in thumb1 or on uc models. Not until v7 did arm have proper
> > working atomics. :(
> 
> Sure. But here as well, my patch is not making things worse I believe.

Agreed. My only concern would be whether your patch makes it harder to
fix, but that's not my call to make.

Rich
Waldemar Brodkorb March 31, 2016, 5:46 p.m. UTC | #6
Hi Thomas,
Thomas Petazzoni wrote,

> Currently, the Thumb support on ARM has three related Config.in
> options, which are not trivial for users to understand, and are in
> fact not needed:
> 
>  - The USE_BX option is not needed: knowing whether BX is available or
>    not is easy. If you have an ARM > v4 or ARMv4T, then BX is
>    available, otherwise it's not. This is the logic used in glibc.
> 
>  - The USE_LDREXSTREX option is not needed: whenever Thumb2 is
>    available, ldrex/strex are available, so we can simply rely on
>    __thumb2__ to determine whether ldrex/strex should be used, without
>    requiring a Config.in option.
> 
>  - Once USE_BX and USE_LDREXSTREX are removed, the only thing left
>    that COMPILE_IN_THUMB does is to set -mthumb. This makes the option
>    unnecessary, as on ARM at least, the user is already supposed to
>    pass -march=<foo> or other compiler options tuning the library for
>    a specific ARM variant. There is no reason to do otherwise for
>    Thumb, which allows to get rid of the COMPILE_IN_THUMB option.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Applied and pushed.

Want to get the thread ARM noMMU problem analyzed and fixed before
the next release. Did you done any further testing?

 best regards
  Waldemar
Thomas Petazzoni March 31, 2016, 6:13 p.m. UTC | #7
Hello,

On Thu, 31 Mar 2016 19:46:21 +0200, Waldemar Brodkorb wrote:

> Want to get the thread ARM noMMU problem analyzed and fixed before
> the next release. Did you done any further testing?

No, I didn't. I away this week and next week, without access to my HW
platform, so I won't be doing any work on this soon.

Best regards,

Thomas
diff mbox

Patch

diff --git a/Rules.mak b/Rules.mak
index b1cecec..0ae3bb1 100644
--- a/Rules.mak
+++ b/Rules.mak
@@ -392,7 +392,6 @@  endif
 ifeq ($(TARGET_ARCH),arm)
 	CPU_CFLAGS-$(ARCH_LITTLE_ENDIAN)+=-mlittle-endian
 	CPU_CFLAGS-$(ARCH_BIG_ENDIAN)+=-mbig-endian
-	CPU_CFLAGS-$(COMPILE_IN_THUMB_MODE)+=-mthumb
 endif
 
 ifeq ($(TARGET_ARCH),metag)
diff --git a/extra/Configs/Config.arm b/extra/Configs/Config.arm
index 00cf982..0d02e3f 100644
--- a/extra/Configs/Config.arm
+++ b/extra/Configs/Config.arm
@@ -24,25 +24,3 @@  config CONFIG_ARM_EABI
 
 	  If you say 'n' here, then the library will be built for the
 	  old Linux ABI.
-
-config COMPILE_IN_THUMB_MODE
-	bool "Build using Thumb mode"
-	select USE_BX
-	select USE_LDREXSTREX
-	help
-	  Say 'y' here to force building uClibc in thumb mode.
-	  Say 'n' to use your compiler's default mode.
-
-config USE_BX
-	bool "Use BX in function return"
-	help
-	  Say 'y' to use BX to return from functions on your thumb-aware
-	  processor. Say 'y' if you need to use interworking. Say 'n' if not.
-	  It is safe to say 'y' even if you're not doing interworking.
-
-config USE_LDREXSTREX
-	bool "Use load-store exclusive ASM ops (not supported in SmartFusion)"
-	depends on COMPILE_IN_THUMB_MODE
-	default n
-	help
-	  Say 'y' to use LDREX/STREX ASM ops.
diff --git a/libc/sysdeps/linux/arm/bits/arm_bx.h b/libc/sysdeps/linux/arm/bits/arm_bx.h
index 2c29089..1c775b6 100644
--- a/libc/sysdeps/linux/arm/bits/arm_bx.h
+++ b/libc/sysdeps/linux/arm/bits/arm_bx.h
@@ -23,13 +23,11 @@ 
 #error Please include features.h first
 #endif /* features.h not yet included */
 
-#if defined(__USE_BX__)
-# if (__ARM_ARCH <= 4 && !defined __ARM_ARCH_4T__)
-#  error Use of BX was requested, but is not available on the target processor.
-# endif /* ARCH level */
-#endif /* __USE_BX__ */
+#if __ARM_ARCH > 4 || defined (__ARM_ARCH_4T__)
+# define ARCH_HAS_BX
+#endif
 
-#if defined(__USE_BX__) && (__ARM_ARCH > 4 || (__ARM_ARCH == 4 && defined __ARM_ARCH_4T__))
+#if defined(ARCH_HAS_BX)
 # define BX(reg)	bx reg
 # define BXC(cond, reg)	bx##cond reg
 #else
diff --git a/libc/sysdeps/linux/arm/clone.S b/libc/sysdeps/linux/arm/clone.S
index b4c7d8a..fd7590d 100644
--- a/libc/sysdeps/linux/arm/clone.S
+++ b/libc/sysdeps/linux/arm/clone.S
@@ -69,7 +69,7 @@  __clone:
 
 	@ pick the function arg and call address off the stack and execute
 	ldr	r0, [sp, #4]
-#if defined(__USE_BX__)
+#if defined(ARCH_HAS_BX)
 	ldr	r1, [sp]
 	bl	2f	@ blx r1
 #else
diff --git a/libpthread/linuxthreads.old/sysdeps/arm/pt-machine.h b/libpthread/linuxthreads.old/sysdeps/arm/pt-machine.h
index 2b877f9..fc17e9b 100644
--- a/libpthread/linuxthreads.old/sysdeps/arm/pt-machine.h
+++ b/libpthread/linuxthreads.old/sysdeps/arm/pt-machine.h
@@ -28,8 +28,7 @@ 
 # define PT_EI __extern_always_inline
 #endif
 
-#if defined(__thumb__)
-#if defined(__USE_LDREXSTREX__)
+#if defined(__thumb2__)
 PT_EI long int ldrex(int *spinlock)
 {
 	long int ret;
@@ -63,7 +62,7 @@  testandset (int *spinlock)
   return ret;
 }
 
-#else /* __USE_LDREXSTREX__ */
+#elif defined(__thumb__)
 
 /* This will not work on ARM1 or ARM2 because SWP is lacking on those
    machines.  Unfortunately we have no way to detect this at compile
@@ -88,7 +87,7 @@  PT_EI long int testandset (int *spinlock)
 	: "0"(1), "r"(spinlock));
   return ret;
 }
-#endif
+
 #else /* __thumb__ */
 
 PT_EI long int testandset (int *spinlock);