[1/2] Add Thumb2 support to ARM assembler memcpy

Submitted by Andre McCurdy on Sept. 13, 2019, 6:44 p.m.

Details

Message ID 20190913184432.29753-1-armccurdy@gmail.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Andre McCurdy Sept. 13, 2019, 6:44 p.m.
For Thumb2 compatibility, replace two instances of a single
instruction "orr with a variable shift" with the two instruction
equivalent. Neither of the replacements are in a performance critical
loop.
---
 src/string/arm/memcpy.c    |  2 +-
 src/string/arm/memcpy_le.S | 17 ++++++++++-------
 2 files changed, 11 insertions(+), 8 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/string/arm/memcpy.c b/src/string/arm/memcpy.c
index f703c9bd..041614f4 100644
--- a/src/string/arm/memcpy.c
+++ b/src/string/arm/memcpy.c
@@ -1,3 +1,3 @@ 
-#if __ARMEB__ || __thumb__
+#if __ARMEB__
 #include "../memcpy.c"
 #endif
diff --git a/src/string/arm/memcpy_le.S b/src/string/arm/memcpy_le.S
index 9cfbcb2a..64bc5f9e 100644
--- a/src/string/arm/memcpy_le.S
+++ b/src/string/arm/memcpy_le.S
@@ -1,4 +1,4 @@ 
-#if !__ARMEB__ && !__thumb__
+#if !__ARMEB__
 
 /*
  * Copyright (C) 2008 The Android Open Source Project
@@ -40,8 +40,9 @@ 
  * This file has been modified from the original for use in musl libc.
  * The main changes are: addition of .type memcpy,%function to make the
  * code safely callable from thumb mode, adjusting the return
- * instructions to be compatible with pre-thumb ARM cpus, and removal
- * of prefetch code that is not compatible with older cpus.
+ * instructions to be compatible with pre-thumb ARM cpus, removal of
+ * prefetch code that is not compatible with older cpus and support for
+ * building as thumb 2.
  */
 
 .syntax unified
@@ -241,8 +242,9 @@  non_congruent:
 	beq     2f
 	ldr     r5, [r1], #4
 	sub     r2, r2, #4
-	orr     r4, r3, r5,             lsl lr
-	mov     r3, r5,                 lsr r12
+	mov     r4, r5, lsl lr
+	orr     r4, r4, r3
+	mov     r3, r5, lsr r12
 	str     r4, [r0], #4
 	cmp     r2, #4
 	bhs     1b
@@ -348,8 +350,9 @@  less_than_thirtytwo:
 
 1:      ldr     r5, [r1], #4
 	sub     r2, r2, #4
-	orr     r4, r3, r5,             lsl lr
-	mov     r3,     r5,                     lsr r12
+	mov     r4, r5, lsl lr
+	orr     r4, r4, r3
+	mov     r3, r5, lsr r12
 	str     r4, [r0], #4
 	cmp     r2, #4
 	bhs     1b

Comments

Rich Felker Jan. 15, 2020, 4:35 p.m.
On Fri, Sep 13, 2019 at 11:44:31AM -0700, Andre McCurdy wrote:
> For Thumb2 compatibility, replace two instances of a single
> instruction "orr with a variable shift" with the two instruction
> equivalent. Neither of the replacements are in a performance critical
> loop.
> ---
>  src/string/arm/memcpy.c    |  2 +-
>  src/string/arm/memcpy_le.S | 17 ++++++++++-------
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/src/string/arm/memcpy.c b/src/string/arm/memcpy.c
> index f703c9bd..041614f4 100644
> --- a/src/string/arm/memcpy.c
> +++ b/src/string/arm/memcpy.c
> @@ -1,3 +1,3 @@
> -#if __ARMEB__ || __thumb__
> +#if __ARMEB__
>  #include "../memcpy.c"
>  #endif
> diff --git a/src/string/arm/memcpy_le.S b/src/string/arm/memcpy_le.S
> index 9cfbcb2a..64bc5f9e 100644
> --- a/src/string/arm/memcpy_le.S
> +++ b/src/string/arm/memcpy_le.S
> @@ -1,4 +1,4 @@
> -#if !__ARMEB__ && !__thumb__
> +#if !__ARMEB__
>  
>  /*
>   * Copyright (C) 2008 The Android Open Source Project
> @@ -40,8 +40,9 @@
>   * This file has been modified from the original for use in musl libc.
>   * The main changes are: addition of .type memcpy,%function to make the
>   * code safely callable from thumb mode, adjusting the return
> - * instructions to be compatible with pre-thumb ARM cpus, and removal
> - * of prefetch code that is not compatible with older cpus.
> + * instructions to be compatible with pre-thumb ARM cpus, removal of
> + * prefetch code that is not compatible with older cpus and support for
> + * building as thumb 2.
>   */
>  
>  .syntax unified
> @@ -241,8 +242,9 @@ non_congruent:
>  	beq     2f
>  	ldr     r5, [r1], #4
>  	sub     r2, r2, #4
> -	orr     r4, r3, r5,             lsl lr
> -	mov     r3, r5,                 lsr r12
> +	mov     r4, r5, lsl lr
> +	orr     r4, r4, r3
> +	mov     r3, r5, lsr r12
>  	str     r4, [r0], #4
>  	cmp     r2, #4
>  	bhs     1b

This is outside of loops and not a hot path, 

> @@ -348,8 +350,9 @@ less_than_thirtytwo:
>  
>  1:      ldr     r5, [r1], #4
>  	sub     r2, r2, #4
> -	orr     r4, r3, r5,             lsl lr
> -	mov     r3,     r5,                     lsr r12
> +	mov     r4, r5, lsl lr
> +	orr     r4, r4, r3
> +	mov     r3, r5, lsr r12
>  	str     r4, [r0], #4
>  	cmp     r2, #4
>  	bhs     1b

This one is in a loop, but perhaps not terribly critical to
performance. We could keep old version with #if !__thumb__ but I doubt
it matters, and it looks like hardly anyone is using pre-thumb2 ARM
anymore anyway; a show-stopping bug went uncaught for over a year in
other things for v6.

One cosmetic fix I'd like to make when applying this is keeping the
old gratuitously-ugly formatting just so the actual change isn't
obscured by the formatting-only change on an adjacent line. I can
handle that though.

Rich
Andre McCurdy Jan. 15, 2020, 6:49 p.m.
On Wed, Jan 15, 2020 at 8:36 AM Rich Felker <dalias@libc.org> wrote:
> On Fri, Sep 13, 2019 at 11:44:31AM -0700, Andre McCurdy wrote:
> > For Thumb2 compatibility, replace two instances of a single
> > instruction "orr with a variable shift" with the two instruction
> > equivalent. Neither of the replacements are in a performance critical
> > loop.
> > ---
> >  src/string/arm/memcpy.c    |  2 +-
> >  src/string/arm/memcpy_le.S | 17 ++++++++++-------
> >  2 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/string/arm/memcpy.c b/src/string/arm/memcpy.c
> > index f703c9bd..041614f4 100644
> > --- a/src/string/arm/memcpy.c
> > +++ b/src/string/arm/memcpy.c
> > @@ -1,3 +1,3 @@
> > -#if __ARMEB__ || __thumb__
> > +#if __ARMEB__
> >  #include "../memcpy.c"
> >  #endif
> > diff --git a/src/string/arm/memcpy_le.S b/src/string/arm/memcpy_le.S
> > index 9cfbcb2a..64bc5f9e 100644
> > --- a/src/string/arm/memcpy_le.S
> > +++ b/src/string/arm/memcpy_le.S
> > @@ -1,4 +1,4 @@
> > -#if !__ARMEB__ && !__thumb__
> > +#if !__ARMEB__
> >
> >  /*
> >   * Copyright (C) 2008 The Android Open Source Project
> > @@ -40,8 +40,9 @@
> >   * This file has been modified from the original for use in musl libc.
> >   * The main changes are: addition of .type memcpy,%function to make the
> >   * code safely callable from thumb mode, adjusting the return
> > - * instructions to be compatible with pre-thumb ARM cpus, and removal
> > - * of prefetch code that is not compatible with older cpus.
> > + * instructions to be compatible with pre-thumb ARM cpus, removal of
> > + * prefetch code that is not compatible with older cpus and support for
> > + * building as thumb 2.
> >   */
> >
> >  .syntax unified
> > @@ -241,8 +242,9 @@ non_congruent:
> >       beq     2f
> >       ldr     r5, [r1], #4
> >       sub     r2, r2, #4
> > -     orr     r4, r3, r5,             lsl lr
> > -     mov     r3, r5,                 lsr r12
> > +     mov     r4, r5, lsl lr
> > +     orr     r4, r4, r3
> > +     mov     r3, r5, lsr r12
> >       str     r4, [r0], #4
> >       cmp     r2, #4
> >       bhs     1b
>
> This is outside of loops and not a hot path,
>
> > @@ -348,8 +350,9 @@ less_than_thirtytwo:
> >
> >  1:      ldr     r5, [r1], #4
> >       sub     r2, r2, #4
> > -     orr     r4, r3, r5,             lsl lr
> > -     mov     r3,     r5,                     lsr r12
> > +     mov     r4, r5, lsl lr
> > +     orr     r4, r4, r3
> > +     mov     r3, r5, lsr r12
> >       str     r4, [r0], #4
> >       cmp     r2, #4
> >       bhs     1b
>
> This one is in a loop, but perhaps not terribly critical to
> performance.

Yes, it's in a loop, but I can confirm it's not a performance critical one.

> We could keep old version with #if !__thumb__ but I doubt
> it matters, and it looks like hardly anyone is using pre-thumb2 ARM
> anymore anyway; a show-stopping bug went uncaught for over a year in
> other things for v6.

I was meaning to ask about that after seeing your recent commit in
master. My primary target is pre-thumb2 armv6 and I hadn't noticed any
problems...

> One cosmetic fix I'd like to make when applying this is keeping the
> old gratuitously-ugly formatting just so the actual change isn't
> obscured by the formatting-only change on an adjacent line. I can
> handle that though.
>
> Rich
Rich Felker Jan. 15, 2020, 7:24 p.m.
On Wed, Jan 15, 2020 at 10:49:03AM -0800, Andre McCurdy wrote:
> On Wed, Jan 15, 2020 at 8:36 AM Rich Felker <dalias@libc.org> wrote:
> > On Fri, Sep 13, 2019 at 11:44:31AM -0700, Andre McCurdy wrote:
> > > For Thumb2 compatibility, replace two instances of a single
> > > instruction "orr with a variable shift" with the two instruction
> > > equivalent. Neither of the replacements are in a performance critical
> > > loop.
> > > ---
> > >  src/string/arm/memcpy.c    |  2 +-
> > >  src/string/arm/memcpy_le.S | 17 ++++++++++-------
> > >  2 files changed, 11 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/string/arm/memcpy.c b/src/string/arm/memcpy.c
> > > index f703c9bd..041614f4 100644
> > > --- a/src/string/arm/memcpy.c
> > > +++ b/src/string/arm/memcpy.c
> > > @@ -1,3 +1,3 @@
> > > -#if __ARMEB__ || __thumb__
> > > +#if __ARMEB__
> > >  #include "../memcpy.c"
> > >  #endif
> > > diff --git a/src/string/arm/memcpy_le.S b/src/string/arm/memcpy_le.S
> > > index 9cfbcb2a..64bc5f9e 100644
> > > --- a/src/string/arm/memcpy_le.S
> > > +++ b/src/string/arm/memcpy_le.S
> > > @@ -1,4 +1,4 @@
> > > -#if !__ARMEB__ && !__thumb__
> > > +#if !__ARMEB__
> > >
> > >  /*
> > >   * Copyright (C) 2008 The Android Open Source Project
> > > @@ -40,8 +40,9 @@
> > >   * This file has been modified from the original for use in musl libc.
> > >   * The main changes are: addition of .type memcpy,%function to make the
> > >   * code safely callable from thumb mode, adjusting the return
> > > - * instructions to be compatible with pre-thumb ARM cpus, and removal
> > > - * of prefetch code that is not compatible with older cpus.
> > > + * instructions to be compatible with pre-thumb ARM cpus, removal of
> > > + * prefetch code that is not compatible with older cpus and support for
> > > + * building as thumb 2.
> > >   */
> > >
> > >  .syntax unified
> > > @@ -241,8 +242,9 @@ non_congruent:
> > >       beq     2f
> > >       ldr     r5, [r1], #4
> > >       sub     r2, r2, #4
> > > -     orr     r4, r3, r5,             lsl lr
> > > -     mov     r3, r5,                 lsr r12
> > > +     mov     r4, r5, lsl lr
> > > +     orr     r4, r4, r3
> > > +     mov     r3, r5, lsr r12
> > >       str     r4, [r0], #4
> > >       cmp     r2, #4
> > >       bhs     1b
> >
> > This is outside of loops and not a hot path,
> >
> > > @@ -348,8 +350,9 @@ less_than_thirtytwo:
> > >
> > >  1:      ldr     r5, [r1], #4
> > >       sub     r2, r2, #4
> > > -     orr     r4, r3, r5,             lsl lr
> > > -     mov     r3,     r5,                     lsr r12
> > > +     mov     r4, r5, lsl lr
> > > +     orr     r4, r4, r3
> > > +     mov     r3, r5, lsr r12
> > >       str     r4, [r0], #4
> > >       cmp     r2, #4
> > >       bhs     1b
> >
> > This one is in a loop, but perhaps not terribly critical to
> > performance.
> 
> Yes, it's in a loop, but I can confirm it's not a performance critical one.

Thanks.

> > We could keep old version with #if !__thumb__ but I doubt
> > it matters, and it looks like hardly anyone is using pre-thumb2 ARM
> > anymore anyway; a show-stopping bug went uncaught for over a year in
> > other things for v6.
> 
> I was meaning to ask about that after seeing your recent commit in
> master. My primary target is pre-thumb2 armv6 and I hadn't noticed any
> problems...

I wonder if there was some magical mechanism by which the anticipated
crash failed to trigger. It certainly triggered in the other affected
arch, sh2, though. If you happen to look at it and find out what was
going on, let us know on the list.

Rich
Andre McCurdy Jan. 15, 2020, 8:20 p.m.
On Wed, Jan 15, 2020 at 11:24 AM Rich Felker <dalias@libc.org> wrote:
> On Wed, Jan 15, 2020 at 10:49:03AM -0800, Andre McCurdy wrote:
> > On Wed, Jan 15, 2020 at 8:36 AM Rich Felker <dalias@libc.org> wrote:
> > > On Fri, Sep 13, 2019 at 11:44:31AM -0700, Andre McCurdy wrote:
> > > > For Thumb2 compatibility, replace two instances of a single
> > > > instruction "orr with a variable shift" with the two instruction
> > > > equivalent. Neither of the replacements are in a performance critical
> > > > loop.
> > > > ---
> > > >  src/string/arm/memcpy.c    |  2 +-
> > > >  src/string/arm/memcpy_le.S | 17 ++++++++++-------
> > > >  2 files changed, 11 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/src/string/arm/memcpy.c b/src/string/arm/memcpy.c
> > > > index f703c9bd..041614f4 100644
> > > > --- a/src/string/arm/memcpy.c
> > > > +++ b/src/string/arm/memcpy.c
> > > > @@ -1,3 +1,3 @@
> > > > -#if __ARMEB__ || __thumb__
> > > > +#if __ARMEB__
> > > >  #include "../memcpy.c"
> > > >  #endif
> > > > diff --git a/src/string/arm/memcpy_le.S b/src/string/arm/memcpy_le.S
> > > > index 9cfbcb2a..64bc5f9e 100644
> > > > --- a/src/string/arm/memcpy_le.S
> > > > +++ b/src/string/arm/memcpy_le.S
> > > > @@ -1,4 +1,4 @@
> > > > -#if !__ARMEB__ && !__thumb__
> > > > +#if !__ARMEB__
> > > >
> > > >  /*
> > > >   * Copyright (C) 2008 The Android Open Source Project
> > > > @@ -40,8 +40,9 @@
> > > >   * This file has been modified from the original for use in musl libc.
> > > >   * The main changes are: addition of .type memcpy,%function to make the
> > > >   * code safely callable from thumb mode, adjusting the return
> > > > - * instructions to be compatible with pre-thumb ARM cpus, and removal
> > > > - * of prefetch code that is not compatible with older cpus.
> > > > + * instructions to be compatible with pre-thumb ARM cpus, removal of
> > > > + * prefetch code that is not compatible with older cpus and support for
> > > > + * building as thumb 2.
> > > >   */
> > > >
> > > >  .syntax unified
> > > > @@ -241,8 +242,9 @@ non_congruent:
> > > >       beq     2f
> > > >       ldr     r5, [r1], #4
> > > >       sub     r2, r2, #4
> > > > -     orr     r4, r3, r5,             lsl lr
> > > > -     mov     r3, r5,                 lsr r12
> > > > +     mov     r4, r5, lsl lr
> > > > +     orr     r4, r4, r3
> > > > +     mov     r3, r5, lsr r12
> > > >       str     r4, [r0], #4
> > > >       cmp     r2, #4
> > > >       bhs     1b
> > >
> > > This is outside of loops and not a hot path,
> > >
> > > > @@ -348,8 +350,9 @@ less_than_thirtytwo:
> > > >
> > > >  1:      ldr     r5, [r1], #4
> > > >       sub     r2, r2, #4
> > > > -     orr     r4, r3, r5,             lsl lr
> > > > -     mov     r3,     r5,                     lsr r12
> > > > +     mov     r4, r5, lsl lr
> > > > +     orr     r4, r4, r3
> > > > +     mov     r3, r5, lsr r12
> > > >       str     r4, [r0], #4
> > > >       cmp     r2, #4
> > > >       bhs     1b
> > >
> > > This one is in a loop, but perhaps not terribly critical to
> > > performance.
> >
> > Yes, it's in a loop, but I can confirm it's not a performance critical one.
>
> Thanks.
>
> > > We could keep old version with #if !__thumb__ but I doubt
> > > it matters, and it looks like hardly anyone is using pre-thumb2 ARM
> > > anymore anyway; a show-stopping bug went uncaught for over a year in
> > > other things for v6.
> >
> > I was meaning to ask about that after seeing your recent commit in
> > master. My primary target is pre-thumb2 armv6 and I hadn't noticed any
> > problems...
>
> I wonder if there was some magical mechanism by which the anticipated
> crash failed to trigger. It certainly triggered in the other affected
> arch, sh2, though. If you happen to look at it and find out what was
> going on, let us know on the list.

For ARM, testing libc.auxv is guarded by a test on __hwcap, so if
__hwcap is also being initialised after __set_thread_area() then
__set_thread_area() will never access libc.auxv ?
Rich Felker Jan. 15, 2020, 8:38 p.m.
On Wed, Jan 15, 2020 at 12:20:08PM -0800, Andre McCurdy wrote:
> On Wed, Jan 15, 2020 at 11:24 AM Rich Felker <dalias@libc.org> wrote:
> > On Wed, Jan 15, 2020 at 10:49:03AM -0800, Andre McCurdy wrote:
> > > On Wed, Jan 15, 2020 at 8:36 AM Rich Felker <dalias@libc.org> wrote:
> > > > On Fri, Sep 13, 2019 at 11:44:31AM -0700, Andre McCurdy wrote:
> > > > > For Thumb2 compatibility, replace two instances of a single
> > > > > instruction "orr with a variable shift" with the two instruction
> > > > > equivalent. Neither of the replacements are in a performance critical
> > > > > loop.
> > > > > ---
> > > > >  src/string/arm/memcpy.c    |  2 +-
> > > > >  src/string/arm/memcpy_le.S | 17 ++++++++++-------
> > > > >  2 files changed, 11 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/src/string/arm/memcpy.c b/src/string/arm/memcpy.c
> > > > > index f703c9bd..041614f4 100644
> > > > > --- a/src/string/arm/memcpy.c
> > > > > +++ b/src/string/arm/memcpy.c
> > > > > @@ -1,3 +1,3 @@
> > > > > -#if __ARMEB__ || __thumb__
> > > > > +#if __ARMEB__
> > > > >  #include "../memcpy.c"
> > > > >  #endif
> > > > > diff --git a/src/string/arm/memcpy_le.S b/src/string/arm/memcpy_le.S
> > > > > index 9cfbcb2a..64bc5f9e 100644
> > > > > --- a/src/string/arm/memcpy_le.S
> > > > > +++ b/src/string/arm/memcpy_le.S
> > > > > @@ -1,4 +1,4 @@
> > > > > -#if !__ARMEB__ && !__thumb__
> > > > > +#if !__ARMEB__
> > > > >
> > > > >  /*
> > > > >   * Copyright (C) 2008 The Android Open Source Project
> > > > > @@ -40,8 +40,9 @@
> > > > >   * This file has been modified from the original for use in musl libc.
> > > > >   * The main changes are: addition of .type memcpy,%function to make the
> > > > >   * code safely callable from thumb mode, adjusting the return
> > > > > - * instructions to be compatible with pre-thumb ARM cpus, and removal
> > > > > - * of prefetch code that is not compatible with older cpus.
> > > > > + * instructions to be compatible with pre-thumb ARM cpus, removal of
> > > > > + * prefetch code that is not compatible with older cpus and support for
> > > > > + * building as thumb 2.
> > > > >   */
> > > > >
> > > > >  .syntax unified
> > > > > @@ -241,8 +242,9 @@ non_congruent:
> > > > >       beq     2f
> > > > >       ldr     r5, [r1], #4
> > > > >       sub     r2, r2, #4
> > > > > -     orr     r4, r3, r5,             lsl lr
> > > > > -     mov     r3, r5,                 lsr r12
> > > > > +     mov     r4, r5, lsl lr
> > > > > +     orr     r4, r4, r3
> > > > > +     mov     r3, r5, lsr r12
> > > > >       str     r4, [r0], #4
> > > > >       cmp     r2, #4
> > > > >       bhs     1b
> > > >
> > > > This is outside of loops and not a hot path,
> > > >
> > > > > @@ -348,8 +350,9 @@ less_than_thirtytwo:
> > > > >
> > > > >  1:      ldr     r5, [r1], #4
> > > > >       sub     r2, r2, #4
> > > > > -     orr     r4, r3, r5,             lsl lr
> > > > > -     mov     r3,     r5,                     lsr r12
> > > > > +     mov     r4, r5, lsl lr
> > > > > +     orr     r4, r4, r3
> > > > > +     mov     r3, r5, lsr r12
> > > > >       str     r4, [r0], #4
> > > > >       cmp     r2, #4
> > > > >       bhs     1b
> > > >
> > > > This one is in a loop, but perhaps not terribly critical to
> > > > performance.
> > >
> > > Yes, it's in a loop, but I can confirm it's not a performance critical one.
> >
> > Thanks.
> >
> > > > We could keep old version with #if !__thumb__ but I doubt
> > > > it matters, and it looks like hardly anyone is using pre-thumb2 ARM
> > > > anymore anyway; a show-stopping bug went uncaught for over a year in
> > > > other things for v6.
> > >
> > > I was meaning to ask about that after seeing your recent commit in
> > > master. My primary target is pre-thumb2 armv6 and I hadn't noticed any
> > > problems...
> >
> > I wonder if there was some magical mechanism by which the anticipated
> > crash failed to trigger. It certainly triggered in the other affected
> > arch, sh2, though. If you happen to look at it and find out what was
> > going on, let us know on the list.
> 
> For ARM, testing libc.auxv is guarded by a test on __hwcap, so if
> __hwcap is also being initialised after __set_thread_area() then
> __set_thread_area() will never access libc.auxv ?

Ahh, that makes sense. And that exposes another bug: __hwcap needs to
be initialized here, but isn't. Without it, wrong backends will be
used and it will crash on kernels that rip out kuser_helper for
hardening purposes. I'll fix that too. Thanks for finding this!

Rich