| Message ID | 1523481378-16290-4-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] arm: Fix armv7 neon memchr on ARM mode | expand |
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.
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.
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.
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.
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.
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.
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
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.
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).
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.
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. >
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. >
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.
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.
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.
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. >
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.
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.
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.
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