[PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode

[PATCH 1/4] arm: Fix armv7 neon memchr on ARM mode

[Adhemerval Zanella]

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

[PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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. 

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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.  

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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).

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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.
> 

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Ramana Radhakrishnan]

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.
>

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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.
> 

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Phil Blundell]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Adhemerval Zanella]

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.

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Richard Earnshaw (lists)]

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.

[PATCH 3/4] arm: Enable ARM mode for armv6 memchr

[Adhemerval Zanella]

Current optimized armv6t2 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.

Checked on arm-linux-gnueabihf (with -marm -march=armv6t2).

	* sysdeps/arm/armv6t2/memchr.S (NO_THUMB): Check for __thumb__
	instead.
---
 ChangeLog                    |  3 +++
 sysdeps/arm/armv6t2/memchr.S | 10 +++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/sysdeps/arm/armv6t2/memchr.S b/sysdeps/arm/armv6t2/memchr.S
index bdd385b..03b7f32 100644
--- a/sysdeps/arm/armv6t2/memchr.S
+++ b/sysdeps/arm/armv6t2/memchr.S
@@ -42,7 +42,7 @@
 	.syntax unified
 
 	.text
-#ifdef NO_THUMB
+#ifndef __thumb__
 	.arm
 #else
 	.thumb
@@ -91,7 +91,7 @@ ENTRY(memchr)
 
 15:
 	ldrd 	r4,r5, [r0],#8
-#ifndef NO_THUMB
+#ifdef __thumb__
 	subs	r6, r6, #8
 #endif
 	eor	r4,r4, r1	@ Get it so that r4,r5 have 00's where the bytes match the target
@@ -100,7 +100,7 @@ ENTRY(memchr)
 	sel	r4, r3, r7	@ bytes are 00 for none-00 bytes, or ff for 00 bytes - NOTE INVERSION
 	uadd8	r5, r5, r7	@ Parallel add 0xff - sets the GE bits for anything that wasn't 0
 	sel	r5, r4, r7	@ chained....bytes are 00 for none-00 bytes, or ff for 00 bytes - NOTE INVERSION
-#ifndef NO_THUMB
+#ifdef __thumb__
 	cbnz	r5, 60f
 #else
 	cmp	r5, #0
@@ -120,7 +120,7 @@ ENTRY(memchr)
 	and	r2,r2,#7	@ Leave the count remaining as the number after the double words have been done
 
 20:
-#ifndef NO_THUMB
+#ifdef __thumb__
 	cbz	r2, 40f		@ 0 length or hit the end already then not found
 #else
 	cmp	r2, #0
@@ -129,7 +129,7 @@ ENTRY(memchr)
 
 21:  @ Post aligned section, or just a short call
 	ldrb	r3,[r0],#1
-#ifndef NO_THUMB
+#ifdef __thumb__
 	subs	r2,r2,#1
 	eor	r3,r3,r1	@ r3 = 0 if match - doesn't break flags from sub
 	cbz	r3, 50f
-- 
2.7.4

[PATCH 2/4] arm: Fix armv7 neon strcmp on ARM mode

[Adhemerval Zanella]

Current optimized armv7 neon strcmp 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 'prepare_mask' does
not build (it sets the 'mvn' instruction to use register predicate in
shift amount).  This patch fixes it by using a S2HI plus mvn to mimic
the expected nor operation.

Checked on arm-linux-gnueabihf (with -marm and -mthumb mode).

	[BZ #23031]
	* sysdeps/arm/armv7/strcmp.S [!__thumb__] (prepare_mask): Fix build
	and logic.
---
 ChangeLog                  | 4 ++++
 sysdeps/arm/armv7/strcmp.S | 6 +++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/sysdeps/arm/armv7/strcmp.S b/sysdeps/arm/armv7/strcmp.S
index 060b865..a20b3e5 100644
--- a/sysdeps/arm/armv7/strcmp.S
+++ b/sysdeps/arm/armv7/strcmp.S
@@ -82,8 +82,7 @@
 #define data2		r3
 #define syndrome	tmp2
 
-
-#ifndef NO_THUMB
+#ifdef __thumb__
 /* This code is best on Thumb.  */
 	.thumb
 
@@ -97,7 +96,8 @@
 #else
 /* In ARM code we don't have ORN, but we can use MVN with a register shift.  */
 .macro prepare_mask mask_reg, nbits_reg
-	mvn \mask_reg, const_m1, S2HI \nbits_reg
+	S2HI \mask_reg, const_m1, \nbits_reg
+	mvn \mask_reg, \mask_reg
 .endm
 .macro apply_mask data_reg, mask_reg
 	orr \data_reg, \data_reg, \mask_reg
-- 
2.7.4

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Wilco Dijkstra]

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

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Wilco Dijkstra]

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

Re: [PATCH 4/4] arm: Enable ARM mode for armv6 strlen

[Florian Weimer]

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