* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen @ 2018-04-13 15:10 Wilco Dijkstra 2018-04-13 15:43 ` Wilco Dijkstra 0 siblings, 1 reply; 23+ messages in thread From: Wilco Dijkstra @ 2018-04-13 15:10 UTC (permalink / raw) To: adhemerval.zanella; +Cc: pb, Ramana Radhakrishnan, nd, libc-alpha Hi, I can't see a valid reason for disabling Thumb-2 when it is available - it's not slower and if necessary you can align loops or force Thumb-2 instructions to be 32 bit in an inner loop. So basically if you have Thumb-2 available you might as well use it to get smaller code - there is no advantage in using Arm. Supporting a no-Thumb-2 build would be additional maintenance (unless it was an official variant it would not be tested as you've shown, and I think there are already too many variants...). Also keeping assembly code clean without lots of #if's that make it hard to follow the logic (or the carry flags!) is best. Wilco ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 15:10 [PATCH 4/4] arm: Enable ARM mode for armv6 strlen Wilco Dijkstra @ 2018-04-13 15:43 ` Wilco Dijkstra 2018-04-13 15:48 ` Florian Weimer 0 siblings, 1 reply; 23+ messages in thread From: Wilco Dijkstra @ 2018-04-13 15:43 UTC (permalink / raw) To: pb; +Cc: Ramana Radhakrishnan, nd, libc-alpha, adhemerval.zanella Phil Blundell wrote: > But here we are, two decades later, and the landscape is a bit > different. It's not entirely clear that -marm/-mthumb are very useful > options in an ARMv7 world because the user shouldn't need to care. In > an ideal world the compiler would be able to predict for any given > function whether T32 or A32 encoding would give the best results > (either speed or space according to the selected optimisation settings) > and proceed accordingly. Unfortunately in practice I don't think > we're quite there yet and for critical code there's still an element of > "try building it both ways round and see which one runs quickest" which > means the compiler flags probably are still necessary. Users might > even legitimately wish to try that experiment with glibc, so forbidding > them to compile it under -marm would not obviously be the right thing > to do. If there are still cases where GCC generates worse code for Thumb-2, then those can be addressed easily. Our very first Thumb-2 compiler had performance within 2% on ARM1156T2 more than 2 decades ago! Things have improved a lot since then, and system wide effects of code footprint have become more important, so using only Arm on a modern core just doesn't make any sense today. Wilco ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 15:43 ` Wilco Dijkstra @ 2018-04-13 15:48 ` Florian Weimer 0 siblings, 0 replies; 23+ messages in thread From: Florian Weimer @ 2018-04-13 15:48 UTC (permalink / raw) To: Wilco Dijkstra, pb Cc: Ramana Radhakrishnan, nd, libc-alpha, adhemerval.zanella On 04/13/2018 05:43 PM, Wilco Dijkstra wrote: > Things have improved a lot since then, and system wide effects of code > footprint have become more important, so using only Arm on a modern > core just doesn't make any sense today. We increasingly see on other architectures that people turn of configurable kernel features, maybe as some form of security hardening (threat surface reduction). This means that code which assumes the presence of these kernel features (because the used to be non-optional and may even be mentioned in ABI documents) no longer runs. Consequently, you have to convince others to change their kernel configuration, or you have to build your software differently. Thanks, Florian ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode @ 2018-04-11 21:16 Adhemerval Zanella 2018-04-11 21:16 ` [PATCH 4/4] arm: Enable ARM mode for armv6 strlen Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-11 21:16 UTC (permalink / raw) To: libc-alpha Current optimized armv7 neon memchr uses the NO_THUMB wrongly to conditionalize thumb instruction usage. The flags is meant to be defined before sysdep.h inclusion and to indicate the assembly requires to build in ARM mode, not to check whether thumb is enable or not. This patch fixes it by using the GCC provided '__thumb__' instead. Also, even if the implementation is fixed to not use thumb instructions it was clearly not proper checked in ARM mode: the carry bit flag will be reset in previous 'cmp synd, #0' and thus the 'bhi cntin, #0' won't be able to branch correctly if the loop finishes with 'cntin' being negative (indicating that some bytes still require to be checked). This patch also fixes it by checking the carry flag in previous loop iteration directly (in ARM mode it will run both '.Lmasklast' and '.Ltail' even if no byte is found in last loop iteration). Checked on arm-linux-gnueabihf (with -marm and -mthumb mode). [BZ #23031] * sysdeps/arm/armv7/multiarch/memchr_neon.S (memchr): Fix tail check on ARM mode. (NO_THUMB): Check __thumb__ instead. --- ChangeLog | 7 +++++++ sysdeps/arm/armv7/multiarch/memchr_neon.S | 9 +++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/sysdeps/arm/armv7/multiarch/memchr_neon.S b/sysdeps/arm/armv7/multiarch/memchr_neon.S index 1b2ae75..1b2a69d 100644 --- a/sysdeps/arm/armv7/multiarch/memchr_neon.S +++ b/sysdeps/arm/armv7/multiarch/memchr_neon.S @@ -68,7 +68,7 @@ * allows to identify exactly which byte has matched. */ -#ifndef NO_THUMB +#ifdef __thumb__ .thumb_func #else .arm @@ -132,7 +132,7 @@ ENTRY(memchr) /* The first block can also be the last */ bls .Lmasklast /* Have we found something already? */ -#ifndef NO_THUMB +#ifdef __thumb__ cbnz synd, .Ltail #else cmp synd, #0 @@ -176,14 +176,11 @@ ENTRY(memchr) vpadd.i8 vdata0_0, vdata0_0, vdata1_0 vpadd.i8 vdata0_0, vdata0_0, vdata0_0 vmov synd, vdata0_0[0] -#ifndef NO_THUMB +#ifdef __thumb__ cbz synd, .Lnotfound bhi .Ltail /* Uses the condition code from subs cntin, cntin, #32 above. */ #else - cmp synd, #0 - beq .Lnotfound - cmp cntin, #0 bhi .Ltail #endif -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-11 21:16 [PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode Adhemerval Zanella @ 2018-04-11 21:16 ` Adhemerval Zanella 2018-04-11 22:12 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-11 21:16 UTC (permalink / raw) To: libc-alpha Current optimized armv6t2 strlen uses the NO_THUMB wrongly to conditionalize thumb instruction usage. The flags is meant to be defined before sysdep.h inclusion and to indicate the assembly requires to build in ARM mode, not to check whether thumb is enable or not. This patch fixes it by using the GCC provided '__thumb__' instead. Checked on arm-linux-gnueabihf (with -marm -march=armv6t2). * sysdeps/arm/armv6t2/strlen.S (NO_THUMB): Check for __thumb__ instead. --- ChangeLog | 3 +++ sysdeps/arm/armv6t2/strlen.S | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/sysdeps/arm/armv6t2/strlen.S b/sysdeps/arm/armv6t2/strlen.S index 6988183..f4111c3 100644 --- a/sysdeps/arm/armv6t2/strlen.S +++ b/sysdeps/arm/armv6t2/strlen.S @@ -21,7 +21,7 @@ */ -#include <arm-features.h> /* This might #define NO_THUMB. */ +#include <arm-features.h> #include <sysdep.h> #ifdef __ARMEB__ @@ -32,7 +32,7 @@ #define S2HI lsl #endif -#ifndef NO_THUMB +#ifdef __thumb__ /* This code is best on Thumb. */ .thumb #else @@ -146,7 +146,7 @@ ENTRY(strlen) tst tmp1, #4 pld [src, #64] S2HI tmp2, const_m1, tmp2 -#ifdef NO_THUMB +#ifndef __thumb__ mvn tmp1, tmp2 orr data1a, data1a, tmp1 itt ne -- 2.7.4 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-11 21:16 ` [PATCH 4/4] arm: Enable ARM mode for armv6 strlen Adhemerval Zanella @ 2018-04-11 22:12 ` Phil Blundell 2018-04-12 12:42 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Phil Blundell @ 2018-04-11 22:12 UTC (permalink / raw) To: Adhemerval Zanella, libc-alpha On Wed, 2018-04-11 at 18:16 -0300, Adhemerval Zanella wrote: > Current optimized armv6t2 strlen uses the NO_THUMB wrongly to > conditionalize thumb instruction usage.  The flags is meant to be > defined before sysdep.h inclusion and to indicate the assembly > requires to build in ARM mode, not to check whether thumb is > enable or not.  This patch fixes it by using the GCC provided > '__thumb__' instead. Is it ever useful to build for ARM-state when on armv6t2 (i.e. Thumb2 is guaranteed to be available)? It's not totally obvious from reading the source that the ARM version is going to be better in any way than the Thumb one. Assuming that this is indeed something worth supporting, I think the short log for the patch ought to say "armv6t2" not "armv6". p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-11 22:12 ` Phil Blundell @ 2018-04-12 12:42 ` Adhemerval Zanella 2018-04-12 15:33 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-12 12:42 UTC (permalink / raw) To: Phil Blundell, libc-alpha On 11/04/2018 19:12, Phil Blundell wrote: > On Wed, 2018-04-11 at 18:16 -0300, Adhemerval Zanella wrote: >> Current optimized armv6t2 strlen uses the NO_THUMB wrongly to >> conditionalize thumb instruction usage.  The flags is meant to be >> defined before sysdep.h inclusion and to indicate the assembly >> requires to build in ARM mode, not to check whether thumb is >> enable or not.  This patch fixes it by using the GCC provided >> '__thumb__' instead. > > Is it ever useful to build for ARM-state when on armv6t2 (i.e. Thumb2 > is guaranteed to be available)? It's not totally obvious from reading > the source that the ARM version is going to be better in any way than > the Thumb one. I guess not, but even though the implementation allows it and the flag usage is wrong. Another option would just remove ARM code, but I think for this we will need to also add configure check to require thumb as well. > > Assuming that this is indeed something worth supporting, I think the > short log for the patch ought to say "armv6t2" not "armv6". Right, I changed it locally. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 12:42 ` Adhemerval Zanella @ 2018-04-12 15:33 ` Phil Blundell 2018-04-12 16:16 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Phil Blundell @ 2018-04-12 15:33 UTC (permalink / raw) To: Adhemerval Zanella, libc-alpha On Thu, 2018-04-12 at 09:41 -0300, Adhemerval Zanella wrote: > I guess not, but even though the implementation allows it and the > flag usage is wrong. Another option would just remove ARM code, but > I think for this we will need to also add configure check to require > thumb as well. If the ARM implementation has no benefit (i.e. is no faster than the Thumb one but takes the same/more space) then I think we should delete it. Having two versions of the code just seems like an unnecessary maintenance headache and source of bugs. What would the configure check be testing for? If the target arch is set to armv6t2 (which it must be if we're using this file) then, by definition, Thumb is available. And the fact that we've apparently been always building the Thumb version in the past seems like a further indication that the use of Thumb here is not a problem. p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 15:33 ` Phil Blundell @ 2018-04-12 16:16 ` Adhemerval Zanella 2018-04-12 19:20 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-12 16:16 UTC (permalink / raw) To: Phil Blundell, libc-alpha On 12/04/2018 12:33, Phil Blundell wrote: > On Thu, 2018-04-12 at 09:41 -0300, Adhemerval Zanella wrote: >> I guess not, but even though the implementation allows it and the >> flag usage is wrong. Another option would just remove ARM code, but >> I think for this we will need to also add configure check to require >> thumb as well. > > If the ARM implementation has no benefit (i.e. is no faster than the > Thumb one but takes the same/more space) then I think we should delete > it. Having two versions of the code just seems like an unnecessary > maintenance headache and source of bugs. Agreed. > > What would the configure check be testing for? If the target arch is > set to armv6t2 (which it must be if we're using this file) then, by > definition, Thumb is available. And the fact that we've apparently > been always building the Thumb version in the past seems like a further > indication that the use of Thumb here is not a problem. I sent the email before I realized armv6t2 always imply in thumb. I think for 3rd and 4th patch a better approach would be just to remove the ARM path and have one version, I will update them. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 16:16 ` Adhemerval Zanella @ 2018-04-12 19:20 ` Adhemerval Zanella 2018-04-12 19:28 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-12 19:20 UTC (permalink / raw) To: Phil Blundell, libc-alpha On 12/04/2018 13:16, Adhemerval Zanella wrote: > > > On 12/04/2018 12:33, Phil Blundell wrote: >> On Thu, 2018-04-12 at 09:41 -0300, Adhemerval Zanella wrote: >>> I guess not, but even though the implementation allows it and the >>> flag usage is wrong. Another option would just remove ARM code, but >>> I think for this we will need to also add configure check to require >>> thumb as well. >> >> If the ARM implementation has no benefit (i.e. is no faster than the >> Thumb one but takes the same/more space) then I think we should delete >> it. Having two versions of the code just seems like an unnecessary >> maintenance headache and source of bugs. > > Agreed. > >> >> What would the configure check be testing for? If the target arch is >> set to armv6t2 (which it must be if we're using this file) then, by >> definition, Thumb is available. And the fact that we've apparently >> been always building the Thumb version in the past seems like a further >> indication that the use of Thumb here is not a problem. > > I sent the email before I realized armv6t2 always imply in thumb. I think > for 3rd and 4th patch a better approach would be just to remove the ARM > path and have one version, I will update them. > In fact for strlen we still require to provide a ARM only mode, since armv7 build will select this implementation as default and it still possible the user would require a ARM only build. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 19:20 ` Adhemerval Zanella @ 2018-04-12 19:28 ` Phil Blundell [not found] ` <CAJA7tRahZ7L=zBs2z+zgCR=RTuccipdjBXhprjxybErebUwv1A@mail.gmail.com> 0 siblings, 1 reply; 23+ messages in thread From: Phil Blundell @ 2018-04-12 19:28 UTC (permalink / raw) To: Adhemerval Zanella, libc-alpha On Thu, 2018-04-12 at 16:20 -0300, Adhemerval Zanella wrote: > In fact for strlen we still require to provide a ARM only mode, since > armv7 build will select this implementation as default and it still > possible the user would require a ARM only build.  ARMv7 guarantees Thumb2 is available, doesn't it? p. ^ permalink raw reply [flat|nested] 23+ messages in thread
[parent not found: <CAJA7tRahZ7L=zBs2z+zgCR=RTuccipdjBXhprjxybErebUwv1A@mail.gmail.com>]
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen [not found] ` <CAJA7tRahZ7L=zBs2z+zgCR=RTuccipdjBXhprjxybErebUwv1A@mail.gmail.com> @ 2018-04-12 19:41 ` Adhemerval Zanella 2018-04-12 20:31 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-12 19:41 UTC (permalink / raw) To: Ramana Radhakrishnan, Phil Blundell; +Cc: GNU C Library On 12/04/2018 16:32, Ramana Radhakrishnan wrote: > > > On Thu, 12 Apr 2018, 20:28 Phil Blundell, <pb@pbcl.net <mailto:pb@pbcl.net>> wrote: > > On Thu, 2018-04-12 at 16:20 -0300, Adhemerval Zanella wrote: > > In fact for strlen we still require to provide a ARM only mode, since > > armv7 build will select this implementation as default and it still > > possible the user would require a ARM only build.  > > ARMv7 guarantees Thumb2 is available, doesn't it? > > > Yep. I am puzzled as to why this patchset is.needed . > > Ramana Because as reported by BZ#23031 [1], the user is using a kernel explicit configured to no enable thumb instructions. Although this kernel config is debatable whether is brings any gain to userland, it still a valid one. [1] https://sourceware.org/bugzilla/show_bug.cgi?id=23031 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 19:41 ` Adhemerval Zanella @ 2018-04-12 20:31 ` Phil Blundell 2018-04-12 20:49 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Phil Blundell @ 2018-04-12 20:31 UTC (permalink / raw) To: Adhemerval Zanella, Ramana Radhakrishnan; +Cc: GNU C Library On Thu, 2018-04-12 at 16:41 -0300, Adhemerval Zanella wrote: > Because as reported by BZ#23031 [1], the user is using a kernel > explicit > configured to no enable thumb instructions. Although this kernel > config > is debatable whether is brings any gain to userland, it still a valid > one. I don't think it's legitimate for the user to tell glibc that he's using ARMv7 and then configure the runtime environment to disable some parts of that architecture. If he doesn't want Thumb, he should configure glibc for an architecture that doesn't include it. p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 20:31 ` Phil Blundell @ 2018-04-12 20:49 ` Adhemerval Zanella 2018-04-13 9:56 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-12 20:49 UTC (permalink / raw) To: Phil Blundell, Ramana Radhakrishnan; +Cc: GNU C Library On 12/04/2018 17:31, Phil Blundell wrote: > On Thu, 2018-04-12 at 16:41 -0300, Adhemerval Zanella wrote: >> Because as reported by BZ#23031 [1], the user is using a kernel >> explicit >> configured to no enable thumb instructions. Although this kernel >> config >> is debatable whether is brings any gain to userland, it still a valid >> one. > > I don't think it's legitimate for the user to tell glibc that he's > using ARMv7 and then configure the runtime environment to disable some > parts of that architecture. If he doesn't want Thumb, he should > configure glibc for an architecture that doesn't include it. > > p. Even though it configure the toolchain to not emit thumb, if user tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will still emit thumb instructions because of the optimized assembly implementations. And the expectation imho is to if user explicit builds with -marm the resulting library should not user thumb instructions. In fact, the whole idea of current code is indeed to prevent thumb instructions in such cases, it is just using the *wrong* preprocessor to check it (and the wrong assumption from the arm-features.h include kinda confirm it). ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-12 20:49 ` Adhemerval Zanella @ 2018-04-13 9:56 ` Phil Blundell 2018-04-13 11:56 ` Adhemerval Zanella 2018-04-13 12:06 ` Ramana Radhakrishnan 0 siblings, 2 replies; 23+ messages in thread From: Phil Blundell @ 2018-04-13 9:56 UTC (permalink / raw) To: Adhemerval Zanella, Ramana Radhakrishnan; +Cc: GNU C Library On Thu, 2018-04-12 at 17:49 -0300, Adhemerval Zanella wrote: > Even though it configure the toolchain to not emit thumb, if user > tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will > still emit thumb instructions because of the optimized assembly > implementations. > And the expectation imho is to if user explicit builds with -marm > the resulting library should not user thumb instructions. I think the precedent on other architectures is that glibc will use the instruction set appropriate to the target triple it was given. For example, if you configure glibc for i686-linux-gnu then it will use CMOV instructions, and setting -march=i586 in CFLAGS won't prevent this. I continue to feel that the scenario mentioned in the bug report you linked to (configuring for armv7 but disabling Thumb in the kernel) is just silly and we should not be indicating to users that this is supported. T32 is an integral part of ARMv7 (indeed, the M profile doesn't support A32 at all) and if you take it out then the resulting architecture is no longer ARMv7. It seems undesirable to force all the ARMv7-optimised assembly routines to also provide an ARM-only version even if the resulting performance is the same or worse than the Thumb implementation. This code is never going to get tested in practice (witness the fact that you found it's been broken for several years) and it's just a liability. > In fact, the whole idea of current code is indeed to prevent thumb > instructions in such cases That's true. It's not entirely clear to me why Roland made that change in the first place but I think it was roughly contemporaneous with the NaCl port and I'm sort of guessing it was something to do with that. Does anybody else know/remember? p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 9:56 ` Phil Blundell @ 2018-04-13 11:56 ` Adhemerval Zanella 2018-04-13 12:06 ` Ramana Radhakrishnan 1 sibling, 0 replies; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-13 11:56 UTC (permalink / raw) To: Phil Blundell, Ramana Radhakrishnan; +Cc: GNU C Library On 13/04/2018 06:56, Phil Blundell wrote: > On Thu, 2018-04-12 at 17:49 -0300, Adhemerval Zanella wrote: >> Even though it configure the toolchain to not emit thumb, if user >> tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will >> still emit thumb instructions because of the optimized assembly >> implementations. >> And the expectation imho is to if user explicit builds with -marm >> the resulting library should not user thumb instructions. > > I think the precedent on other architectures is that glibc will use the > instruction set appropriate to the target triple it was given. For > example, if you configure glibc for i686-linux-gnu then it will use > CMOV instructions, and setting -march=i586 in CFLAGS won't prevent > this. In fact to determine the sysdeps folders glibc build what machine the compiler is configured for. On ARM for instance, the base machine is determined at sysdeps/arm/preconfigure.ac by checking the __ARM_ARCH_* builtin preprocessor direct from compiler. So it does not really matter if use armv7-linux-gnueabihf is used as triple, since the preprocessor builtint can be changed by -march. And I think this is the correct way, the multiarch idea is exactly to avoid the necessity of explicit set the target ISA. And user can also tune if required by changing the CC/CFLAGS for the desirable target. > > I continue to feel that the scenario mentioned in the bug report you > linked to (configuring for armv7 but disabling Thumb in the kernel) is > just silly and we should not be indicating to users that this is > supported. T32 is an integral part of ARMv7 (indeed, the M profile > doesn't support A32 at all) and if you take it out then the resulting > architecture is no longer ARMv7. It seems undesirable to force all the > ARMv7-optimised assembly routines to also provide an ARM-only version > even if the resulting performance is the same or worse than the Thumb > implementation. This code is never going to get tested in practice > (witness the fact that you found it's been broken for several years) > and it's just a liability. Since T32 is an integral part of ARMv7, I would expect either that kernel do not provide an option to disable it or at least emulate thumb instruction if underlying hardware do not provide it (as for other various architectures for some atomic operation for instance). However the current scenario exists and I see no strong reason to not support it. We already handle the cases in generic code where we should not generate thumb (NO_THUMB macro) and the fixes I sent already are minimal. For newer implementation it is a matter of if the idea is to always support thumb so simple redirection to a previous implementation is straightforward (ifndef __thumb__ then include old implementation). But I give you this incurs in more maintainability and I do agree we should avoid such path. However what we shouldn't is simply breaking at runtime due a non-supported configuration. If the idea is to support thumb as default, we should then indicate at build time that it is required (either at configure time or at build time). > >> In fact, the whole idea of current code is indeed to prevent thumb >> instructions in such cases > > That's true. It's not entirely clear to me why Roland made that change > in the first place but I think it was roughly contemporaneous with the > NaCl port and I'm sort of guessing it was something to do with that. > Does anybody else know/remember? > > p. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 9:56 ` Phil Blundell 2018-04-13 11:56 ` Adhemerval Zanella @ 2018-04-13 12:06 ` Ramana Radhakrishnan 2018-04-13 12:44 ` Phil Blundell 1 sibling, 1 reply; 23+ messages in thread From: Ramana Radhakrishnan @ 2018-04-13 12:06 UTC (permalink / raw) To: Phil Blundell; +Cc: Adhemerval Zanella, GNU C Library On Fri, Apr 13, 2018 at 10:56 AM, Phil Blundell <pb@pbcl.net> wrote: > On Thu, 2018-04-12 at 17:49 -0300, Adhemerval Zanella wrote: >> Even though it configure the toolchain to not emit thumb, if user >> tries to optimize for armv7 (by tinkering with CC or CFLAGS) it will >> still emit thumb instructions because of the optimized assembly >> implementations. >> And the expectation imho is to if user explicit builds with -marm >> the resulting library should not user thumb instructions. > > I think the precedent on other architectures is that glibc will use the > instruction set appropriate to the target triple it was given. For > example, if you configure glibc for i686-linux-gnu then it will use > CMOV instructions, and setting -march=i586 in CFLAGS won't prevent > this. > > I continue to feel that the scenario mentioned in the bug report you > linked to (configuring for armv7 but disabling Thumb in the kernel) is > just silly and we should not be indicating to users that this is > supported. T32 is an integral part of ARMv7 (indeed, the M profile > doesn't support A32 at all) and if you take it out then the resulting > architecture is no longer ARMv7. It seems undesirable to force all the > ARMv7-optimised assembly routines to also provide an ARM-only version > even if the resulting performance is the same or worse than the Thumb > implementation. This code is never going to get tested in practice > (witness the fact that you found it's been broken for several years) > and it's just a liability. > >> In fact, the whole idea of current code is indeed to prevent thumb >> instructions in such cases > > That's true. It's not entirely clear to me why Roland made that change > in the first place but I think it was roughly contemporaneous with the > NaCl port and I'm sort of guessing it was something to do with that. > Does anybody else know/remember? That change coming in with the NaCl port makes sense to me. IIRC NaCl wanted fixed length encodings, (thus )everything to be in Arm state only : instructions that were "forbidden" / not allowed as part of the whole NaCl strategy for providing sand boxes including switching to Thumb. I also seem to remember that they had stuff that checked instructions in the binary were in a particular set and anything outside was not considered to be a safe binary. Aha - https://developer.chrome.com/native-client/reference/sandbox_internals/arm-32-bit-sandbox#arm-32-bit-sandbox regards Ramana > p. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 12:06 ` Ramana Radhakrishnan @ 2018-04-13 12:44 ` Phil Blundell 2018-04-13 14:40 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Phil Blundell @ 2018-04-13 12:44 UTC (permalink / raw) To: Ramana Radhakrishnan; +Cc: Adhemerval Zanella, GNU C Library On Fri, 2018-04-13 at 13:06 +0100, Ramana Radhakrishnan wrote: > That change coming in with the NaCl port makes sense to me. IIRC NaCl > wanted fixed length encodings, (thus )everything to be in Arm state > only : instructions that were "forbidden" / not allowed as part of > the > whole NaCl strategy for providing sand boxes including switching to > Thumb. I also seem to remember that they had stuff that checked > instructions in the binary were in a particular set and anything > outside was not considered to be a safe binary. > > Aha - https://developer.chrome.com/native-client/reference/sandbox_in > ternals/arm-32-bit-sandbox#arm-32-bit-sandbox Ah, thanks, that makes sense. In that case, given that the rest of the NaCl port was removed a year ago, maybe we should also remove this cruft. I think that'd essentially mean reverting 4f510e3aeeeb3fd974a12a71789fa9c63ab8c6dd and similar commits. p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 12:44 ` Phil Blundell @ 2018-04-13 14:40 ` Adhemerval Zanella 2018-04-13 15:07 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-13 14:40 UTC (permalink / raw) To: Phil Blundell, Ramana Radhakrishnan; +Cc: GNU C Library On 13/04/2018 09:44, Phil Blundell wrote: > On Fri, 2018-04-13 at 13:06 +0100, Ramana Radhakrishnan wrote: >> That change coming in with the NaCl port makes sense to me. IIRC NaCl >> wanted fixed length encodings, (thus )everything to be in Arm state >> only : instructions that were "forbidden" / not allowed as part of >> the >> whole NaCl strategy for providing sand boxes including switching to >> Thumb. I also seem to remember that they had stuff that checked >> instructions in the binary were in a particular set and anything >> outside was not considered to be a safe binary. >> >> Aha - https://developer.chrome.com/native-client/reference/sandbox_in >> ternals/arm-32-bit-sandbox#arm-32-bit-sandbox > > Ah, thanks, that makes sense. In that case, given that the rest of the > NaCl port was removed a year ago, maybe we should also remove this > cruft. I think that'd essentially mean reverting > 4f510e3aeeeb3fd974a12a71789fa9c63ab8c6dd and similar commits. > > p. > I am not against reverting it, but it does not help the scenario of BZ#23031. We need to define whether environments which thumb is expected to be supported but it is not enabled by compiler flags (-marm) and act accordingly by avoid building glibc in such cases. If we define that is the desirable case, we can also cleanup the ARM code path for armv7 memchr and strcmp. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 14:40 ` Adhemerval Zanella @ 2018-04-13 15:07 ` Phil Blundell 2018-04-13 16:42 ` Adhemerval Zanella 0 siblings, 1 reply; 23+ messages in thread From: Phil Blundell @ 2018-04-13 15:07 UTC (permalink / raw) To: Adhemerval Zanella, Ramana Radhakrishnan; +Cc: GNU C Library On Fri, 2018-04-13 at 11:40 -0300, Adhemerval Zanella wrote: > I am not against reverting it, but it does not help the scenario of > BZ#23031. We need to define whether environments which thumb is > expected to be supported but it is not enabled by compiler flags > (-marm) and act accordingly by avoid building glibc in such cases. My position on that remains that the scenario of BZ#23031 is simply invalid and the user's expectations are just misplaced. The fact that we've had sysdeps/arm/armv6t2/memchr.S using Thumb code since December 2011 and in those six years it's only generated one bug report seems to support the belief that there is not a large population of users trying to do this sort of thing. I don't think -marm has ever been intended to mean "don't allow any Thumb instructions in my program". The whole -marm/-mthumb thing dates from ARMv4T/Thumb-1 days when interworking couldn't be taken for granted and Thumb code often ran significantly slower than the equivalent ARM code. Under those circumstances it was necessary for the user to be able to choose what instruction encoding the compiler was going to generate. But here we are, two decades later, and the landscape is a bit different. It's not entirely clear that -marm/-mthumb are very useful options in an ARMv7 world because the user shouldn't need to care. In an ideal world the compiler would be able to predict for any given function whether T32 or A32 encoding would give the best results (either speed or space according to the selected optimisation settings) and proceed accordingly. Unfortunately in practice I don't think we're quite there yet and for critical code there's still an element of "try building it both ways round and see which one runs quickest" which means the compiler flags probably are still necessary. Users might even legitimately wish to try that experiment with glibc, so forbidding them to compile it under -marm would not obviously be the right thing to do. p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 15:07 ` Phil Blundell @ 2018-04-13 16:42 ` Adhemerval Zanella 2018-04-13 19:09 ` Phil Blundell 0 siblings, 1 reply; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-13 16:42 UTC (permalink / raw) To: Phil Blundell, Ramana Radhakrishnan; +Cc: GNU C Library On 13/04/2018 12:07, Phil Blundell wrote: > On Fri, 2018-04-13 at 11:40 -0300, Adhemerval Zanella wrote: >> I am not against reverting it, but it does not help the scenario of >> BZ#23031. We need to define whether environments which thumb is >> expected to be supported but it is not enabled by compiler flags >> (-marm) and act accordingly by avoid building glibc in such cases. > > My position on that remains that the scenario of BZ#23031 is simply > invalid and the user's expectations are just misplaced. The fact that > we've had sysdeps/arm/armv6t2/memchr.S using Thumb code since December > 2011 and in those six years it's only generated one bug report seems to > support the belief that there is not a large population of users trying > to do this sort of thing. My understanding is glibc try support kernel with missing functionalities but either using fallback implementations (either subpar with underlying issues as for old pipe2), emulation (as for copy_file_range which tries to emulate the kernel behaviour), or by just warning userland the kernel do not provide the underlying support (for instance set_robust_list). I really think just saying 'go fix your kernel' is not the correct answer, even more when the configuration used is supported upstream (it not an experimental or out-of-tree one). Also for specific case, my wild guess is using ARM code path should not shown in much difference and will prevent such possible issues. > > I don't think -marm has ever been intended to mean "don't allow any > Thumb instructions in my program". The whole -marm/-mthumb thing dates > from ARMv4T/Thumb-1 days when interworking couldn't be taken for > granted and Thumb code often ran significantly slower than the > equivalent ARM code. Under those circumstances it was necessary for > the user to be able to choose what instruction encoding the compiler > was going to generate. > > But here we are, two decades later, and the landscape is a bit > different. It's not entirely clear that -marm/-mthumb are very useful > options in an ARMv7 world because the user shouldn't need to care. In > an ideal world the compiler would be able to predict for any given > function whether T32 or A32 encoding would give the best results > (either speed or space according to the selected optimisation settings) > and proceed accordingly. Unfortunately in practice I don't think > we're quite there yet and for critical code there's still an element of > "try building it both ways round and see which one runs quickest" which > means the compiler flags probably are still necessary. Users might > even legitimately wish to try that experiment with glibc, so forbidding > them to compile it under -marm would not obviously be the right thing > to do. > > p. > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 16:42 ` Adhemerval Zanella @ 2018-04-13 19:09 ` Phil Blundell 2018-04-13 20:12 ` Adhemerval Zanella 2018-04-16 14:16 ` Richard Earnshaw (lists) 0 siblings, 2 replies; 23+ messages in thread From: Phil Blundell @ 2018-04-13 19:09 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: GNU C Library On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote: > My understanding is glibc try support kernel with missing > functionalities but either using fallback implementations (either > subpar with underlying issues as for old pipe2), emulation (as for > copy_file_range which tries to emulate the kernel behaviour), or by > just warning userland the kernel do not provide the underlying > support (for instance set_robust_list). It's true that glibc has tried (with varying levels of success) to provide compatibility implementations so that new APIs can be used on older kernels which lack some or other functionality. But this doesn't extend to working around arbitrarily broken kernel configurations. The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building for ARMv7 anyway is so close to zero as to be completely negligible. There is simply no rational reason to not have it included. It would be interesting to ask the guy who filed that bugzilla ticket whether he has turned it off deliberately, and if so why. I suspect he probably just didn't realise it was needed, rather than actively wanting it off. > I really think just saying 'go fix your kernel' is not the correct > answer, even more when the configuration used is supported upstream > (it not an experimental or out-of-tree one). There are any number of other ways in which you can break your system by configuring your kernel wrongly. The fact that the kernel config system lets you turn a given option on or off doesn't mean you can just flip switches at random and expect things to work. That said, in the particular case of CONFIG_ARM_THUMB, I think the kernel people should simply force this on when CONFIG_CPU_V7 is selected. > Also for specific case, my wild guess is using ARM code path should > not shown in much difference and will prevent such possible issues. In this particular case you're right, the ARM implementation is probably not going to perform very much worse, and now that you've fixed at least the obvious bugs I think it will probably work fine.  My concern is about the precedent it sets for the future. Right now, it clearly is not true that building glibc for ARMv7 with -marm will use only A32 instructions. Further, as far as I can tell it has never been true, so there cannot be any existing users who are depending on this behaviour. If we fix this bug in the way that you are proposing, we would be making an implicit promise that any future ARMv7-optimised assembly code is also sensitive to being compiled under -marm and will avoid Thumb2 instructions in that situation. So, all in all it seems we have a choice between: - we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB in your kernel for glibc to work on ARMv7", he enables the option, it doesn't cost him anything, and everyone moves on; or - we fix glibc to avoid Thumb2 instructions in these assembly files, which means we now have an extra variant that we have to maintain and test, we need to remember to add the same checks to any future assembly code that might be added for v7 in the future, and we essentially can't ever stop doing that for fear that users might have become dependent on it I would prefer to do the former. p. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 19:09 ` Phil Blundell @ 2018-04-13 20:12 ` Adhemerval Zanella 2018-04-16 14:16 ` Richard Earnshaw (lists) 1 sibling, 0 replies; 23+ messages in thread From: Adhemerval Zanella @ 2018-04-13 20:12 UTC (permalink / raw) To: Phil Blundell; +Cc: GNU C Library On 13/04/2018 16:09, Phil Blundell wrote: > On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote: >> My understanding is glibc try support kernel with missing >> functionalities but either using fallback implementations (either >> subpar with underlying issues as for old pipe2), emulation (as for >> copy_file_range which tries to emulate the kernel behaviour), or by >> just warning userland the kernel do not provide the underlying >> support (for instance set_robust_list). > > It's true that glibc has tried (with varying levels of success) to > provide compatibility implementations so that new APIs can be used on > older kernels which lack some or other functionality. But this doesn't > extend to working around arbitrarily broken kernel configurations. > > The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building > for ARMv7 anyway is so close to zero as to be completely negligible. > There is simply no rational reason to not have it included. It would > be interesting to ask the guy who filed that bugzilla ticket whether he > has turned it off deliberately, and if so why. I suspect he probably > just didn't realise it was needed, rather than actively wanting it off. Fair enough, looks like CONFIG_ARM_THUMB is not defined only for some old armv5t kernel config. > >> I really think just saying 'go fix your kernel' is not the correct >> answer, even more when the configuration used is supported upstream >> (it not an experimental or out-of-tree one). > > There are any number of other ways in which you can break your system > by configuring your kernel wrongly. The fact that the kernel config > system lets you turn a given option on or off doesn't mean you can just > flip switches at random and expect things to work. That said, in the > particular case of CONFIG_ARM_THUMB, I think the kernel people should > simply force this on when CONFIG_CPU_V7 is selected. I strong agree CONFIG_CPU_V7 should imply CONFIG_ARM_THUMB. > >> Also for specific case, my wild guess is using ARM code path should >> not shown in much difference and will prevent such possible issues. > > In this particular case you're right, the ARM implementation is > probably not going to perform very much worse, and now that you've > fixed at least the obvious bugs I think it will probably work fine.  > > My concern is about the precedent it sets for the future. Right now, > it clearly is not true that building glibc for ARMv7 with -marm will > use only A32 instructions. Further, as far as I can tell it has never > been true, so there cannot be any existing users who are depending on > this behaviour. If we fix this bug in the way that you are proposing, > we would be making an implicit promise that any future ARMv7-optimised > assembly code is also sensitive to being compiled under -marm and will > avoid Thumb2 instructions in that situation. > > So, all in all it seems we have a choice between: > > - we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB > in your kernel for glibc to work on ARMv7", he enables the option, it > doesn't cost him anything, and everyone moves on; or > > - we fix glibc to avoid Thumb2 instructions in these assembly files, > which means we now have an extra variant that we have to maintain and > test, we need to remember to add the same checks to any future assembly > code that might be added for v7 in the future, and we essentially can't > ever stop doing that for fear that users might have become dependent on > it > > I would prefer to do the former. > > p. > I do prefer the former and this discussion indeed shown there is little gain in maintaining two build modes for the string functions. I will resend the patch with just removing the ARM code path and make thumb as default an only build option. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen 2018-04-13 19:09 ` Phil Blundell 2018-04-13 20:12 ` Adhemerval Zanella @ 2018-04-16 14:16 ` Richard Earnshaw (lists) 1 sibling, 0 replies; 23+ messages in thread From: Richard Earnshaw (lists) @ 2018-04-16 14:16 UTC (permalink / raw) To: Phil Blundell, Adhemerval Zanella; +Cc: GNU C Library On 13/04/18 20:09, Phil Blundell wrote: > On Fri, 2018-04-13 at 13:42 -0300, Adhemerval Zanella wrote: >> My understanding is glibc try support kernel with missing >> functionalities but either using fallback implementations (either >> subpar with underlying issues as for old pipe2), emulation (as for >> copy_file_range which tries to emulate the kernel behaviour), or by >> just warning userland the kernel do not provide the underlying >> support (for instance set_robust_list). > > It's true that glibc has tried (with varying levels of success) to > provide compatibility implementations so that new APIs can be used on > older kernels which lack some or other functionality. But this doesn't > extend to working around arbitrarily broken kernel configurations. > > The cost of enabling CONFIG_ARM_THUMB in your kernel if you're building > for ARMv7 anyway is so close to zero as to be completely negligible. > There is simply no rational reason to not have it included. It would > be interesting to ask the guy who filed that bugzilla ticket whether he > has turned it off deliberately, and if so why. I suspect he probably > just didn't realise it was needed, rather than actively wanting it off. > >> I really think just saying 'go fix your kernel' is not the correct >> answer, even more when the configuration used is supported upstream >> (it not an experimental or out-of-tree one). > > There are any number of other ways in which you can break your system > by configuring your kernel wrongly. The fact that the kernel config > system lets you turn a given option on or off doesn't mean you can just > flip switches at random and expect things to work. That said, in the > particular case of CONFIG_ARM_THUMB, I think the kernel people should > simply force this on when CONFIG_CPU_V7 is selected. > >> Also for specific case, my wild guess is using ARM code path should >> not shown in much difference and will prevent such possible issues. > > In this particular case you're right, the ARM implementation is > probably not going to perform very much worse, and now that you've > fixed at least the obvious bugs I think it will probably work fine.  > > My concern is about the precedent it sets for the future. Right now, > it clearly is not true that building glibc for ARMv7 with -marm will > use only A32 instructions. Further, as far as I can tell it has never > been true, so there cannot be any existing users who are depending on > this behaviour. If we fix this bug in the way that you are proposing, > we would be making an implicit promise that any future ARMv7-optimised > assembly code is also sensitive to being compiled under -marm and will > avoid Thumb2 instructions in that situation. > > So, all in all it seems we have a choice between: > > - we just tell this one guy "sorry, you have to enable CONFIG_ARM_THUMB > in your kernel for glibc to work on ARMv7", he enables the option, it > doesn't cost him anything, and everyone moves on; or > +1 for all of the above :-) > - we fix glibc to avoid Thumb2 instructions in these assembly files, > which means we now have an extra variant that we have to maintain and > test, we need to remember to add the same checks to any future assembly > code that might be added for v7 in the future, and we essentially can't > ever stop doing that for fear that users might have become dependent on > it > > I would prefer to do the former. It takes a lot of work to tune the routines written in assembler across a range of processors. Needlessly repeating that for perverse kernel configurations is just make work. R. ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2018-04-16 14:16 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-13 15:10 [PATCH 4/4] arm: Enable ARM mode for armv6 strlen Wilco Dijkstra 2018-04-13 15:43 ` Wilco Dijkstra 2018-04-13 15:48 ` Florian Weimer -- strict thread matches above, loose matches on Subject: below -- 2018-04-11 21:16 [PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode Adhemerval Zanella 2018-04-11 21:16 ` [PATCH 4/4] arm: Enable ARM mode for armv6 strlen Adhemerval Zanella 2018-04-11 22:12 ` Phil Blundell 2018-04-12 12:42 ` Adhemerval Zanella 2018-04-12 15:33 ` Phil Blundell 2018-04-12 16:16 ` Adhemerval Zanella 2018-04-12 19:20 ` Adhemerval Zanella 2018-04-12 19:28 ` Phil Blundell [not found] ` <CAJA7tRahZ7L=zBs2z+zgCR=RTuccipdjBXhprjxybErebUwv1A@mail.gmail.com> 2018-04-12 19:41 ` Adhemerval Zanella 2018-04-12 20:31 ` Phil Blundell 2018-04-12 20:49 ` Adhemerval Zanella 2018-04-13 9:56 ` Phil Blundell 2018-04-13 11:56 ` Adhemerval Zanella 2018-04-13 12:06 ` Ramana Radhakrishnan 2018-04-13 12:44 ` Phil Blundell 2018-04-13 14:40 ` Adhemerval Zanella 2018-04-13 15:07 ` Phil Blundell 2018-04-13 16:42 ` Adhemerval Zanella 2018-04-13 19:09 ` Phil Blundell 2018-04-13 20:12 ` Adhemerval Zanella 2018-04-16 14:16 ` Richard Earnshaw (lists)
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).