| Message ID | 1458493115-26904-1-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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 --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);
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(-)