[PATCH] Replace insns in ARMv6-M setjmp impl with flag-setting variants

[PATCH] Replace insns in ARMv6-M setjmp impl with flag-setting variants

[Christos Gentsos]

In the ARMv6-M Thumb instruction set the MOV, ADD and SUB instructions
don't support flag-preserving variants for their 8-bit immediate
forms.
---
 newlib/libc/machine/arm/setjmp.S | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/newlib/libc/machine/arm/setjmp.S b/newlib/libc/machine/arm/setjmp.S
index 1ba711d5..b8511779 100644
--- a/newlib/libc/machine/arm/setjmp.S
+++ b/newlib/libc/machine/arm/setjmp.S
@@ -74,11 +74,11 @@ SYM (setjmp):
 	mov	r5, sp
 	mov	r6, lr
 	stmia	r0!, {r1, r2, r3, r4, r5, r6}
-	sub	r0, r0, #40
+	subs	r0, r0, #40
 	/* Restore callee-saved low regs.  */
 	ldmia	r0!, {r4, r5, r6, r7}
 	/* Return zero.  */
-	mov	r0, #0
+	movs	r0, #0
 	bx lr
 
 .thumb_func
@@ -86,7 +86,7 @@ SYM (setjmp):
 	TYPE (longjmp)
 SYM (longjmp):
 	/* Restore High regs.  */
-	add	r0, r0, #16
+	adds	r0, r0, #16
 	ldmia	r0!, {r2, r3, r4, r5, r6}
 	mov	r8, r2
 	mov	r9, r3
@@ -95,12 +95,12 @@ SYM (longjmp):
 	mov	sp, r6
 	ldmia	r0!, {r3} /* lr */
 	/* Restore low regs.  */
-	sub	r0, r0, #40
+	subs	r0, r0, #40
 	ldmia	r0!, {r4, r5, r6, r7}
 	/* Return the result argument, or 1 if it is zero.  */
 	mov	r0, r1
 	bne	1f
-	mov	r0, #1
+	movs	r0, #1
 1:
 	bx	r3
 
-- 
2.23.0

Re: [PATCH] Replace insns in ARMv6-M setjmp impl with flag-setting variants

[Richard Earnshaw (lists)]

On 30/09/2019 16:15, Christos Gentsos wrote:
> In the ARMv6-M Thumb instruction set the MOV, ADD and SUB instructions
> don't support flag-preserving variants for their 8-bit immediate
> forms.
> ---
>   newlib/libc/machine/arm/setjmp.S | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/newlib/libc/machine/arm/setjmp.S b/newlib/libc/machine/arm/setjmp.S
> index 1ba711d5..b8511779 100644
> --- a/newlib/libc/machine/arm/setjmp.S
> +++ b/newlib/libc/machine/arm/setjmp.S
> @@ -74,11 +74,11 @@ SYM (setjmp):
>   	mov	r5, sp
>   	mov	r6, lr
>   	stmia	r0!, {r1, r2, r3, r4, r5, r6}
> -	sub	r0, r0, #40
> +	subs	r0, r0, #40
>   	/* Restore callee-saved low regs.  */
>   	ldmia	r0!, {r4, r5, r6, r7}
>   	/* Return zero.  */
> -	mov	r0, #0
> +	movs	r0, #0
>   	bx lr
>   
>   .thumb_func
> @@ -86,7 +86,7 @@ SYM (setjmp):
>   	TYPE (longjmp)
>   SYM (longjmp):
>   	/* Restore High regs.  */
> -	add	r0, r0, #16
> +	adds	r0, r0, #16
>   	ldmia	r0!, {r2, r3, r4, r5, r6}
>   	mov	r8, r2
>   	mov	r9, r3
> @@ -95,12 +95,12 @@ SYM (longjmp):
>   	mov	sp, r6
>   	ldmia	r0!, {r3} /* lr */
>   	/* Restore low regs.  */
> -	sub	r0, r0, #40
> +	subs	r0, r0, #40
>   	ldmia	r0!, {r4, r5, r6, r7}
>   	/* Return the result argument, or 1 if it is zero.  */
>   	mov	r0, r1
>   	bne	1f
> -	mov	r0, #1
> +	movs	r0, #1
>   1:
>   	bx	r3
>   
> 

Sorry for the delay responding to this one, I wanted to look at this in 
more detail...

Having done so, I don't think this is correct.  The code you are looking 
at here is written in legacy syntax, rather than unified syntax.  In 
legacy syntax there was no need to specify the flag-clobbering behaviour 
of instructions.

For example, the longjump code does not make sense otherwise as there is 
no explicit comparison operation, yet there is a conditional branch near 
the end:

 > 	sub	r0, r0, #40
 >   	ldmia	r0!, {r4, r5, r6, r7}
 >   	/* Return the result argument, or 1 if it is zero.  */
 >   	mov	r0, r1  // <<<< implicit flag set ....
 >   	bne	1f	// <<<< ... only makes sense in legacy syntax
 > 	mov	r0, #1
 >   1:

How did you build and test this?

R.

Re: [PATCH] Replace insns in ARMv6-M setjmp impl with flag-setting variants

[Christos Gentsos]

Oh I see, you're totally right.

I was trying to use Clang to compile newlib, I wasn't aware that its
assembler didn't fully support the legacy ARM syntax - it was trying to
parse it as unified syntax code. Using GCC it compiles just fine.

The longjmp example you brought up also makes perfect sense, it seems
that the code I used my newlib build with only worked because it wasn't
using it.

Thanks so much for your detailed reply and for the time you took to look
into this!

Cheers,
Christos

On Mon, Oct 07 2019 at 15:42:21 +0100, Richard Earnshaw (lists) wrote:

> On 30/09/2019 16:15, Christos Gentsos wrote:
>> In the ARMv6-M Thumb instruction set the MOV, ADD and SUB instructions
>> don't support flag-preserving variants for their 8-bit immediate
>> forms.
>> ---
>>   newlib/libc/machine/arm/setjmp.S | 10 +++++-----
>>   1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/newlib/libc/machine/arm/setjmp.S b/newlib/libc/machine/arm/setjmp.S
>> index 1ba711d5..b8511779 100644
>> --- a/newlib/libc/machine/arm/setjmp.S
>> +++ b/newlib/libc/machine/arm/setjmp.S
>> @@ -74,11 +74,11 @@ SYM (setjmp):
>>   	mov	r5, sp
>>   	mov	r6, lr
>>   	stmia	r0!, {r1, r2, r3, r4, r5, r6}
>> -	sub	r0, r0, #40
>> +	subs	r0, r0, #40
>>   	/* Restore callee-saved low regs.  */
>>   	ldmia	r0!, {r4, r5, r6, r7}
>>   	/* Return zero.  */
>> -	mov	r0, #0
>> +	movs	r0, #0
>>   	bx lr
>>   
>>   .thumb_func
>> @@ -86,7 +86,7 @@ SYM (setjmp):
>>   	TYPE (longjmp)
>>   SYM (longjmp):
>>   	/* Restore High regs.  */
>> -	add	r0, r0, #16
>> +	adds	r0, r0, #16
>>   	ldmia	r0!, {r2, r3, r4, r5, r6}
>>   	mov	r8, r2
>>   	mov	r9, r3
>> @@ -95,12 +95,12 @@ SYM (longjmp):
>>   	mov	sp, r6
>>   	ldmia	r0!, {r3} /* lr */
>>   	/* Restore low regs.  */
>> -	sub	r0, r0, #40
>> +	subs	r0, r0, #40
>>   	ldmia	r0!, {r4, r5, r6, r7}
>>   	/* Return the result argument, or 1 if it is zero.  */
>>   	mov	r0, r1
>>   	bne	1f
>> -	mov	r0, #1
>> +	movs	r0, #1
>>   1:
>>   	bx	r3
>>   
>> 
>
> Sorry for the delay responding to this one, I wanted to look at this in 
> more detail...
>
> Having done so, I don't think this is correct.  The code you are looking 
> at here is written in legacy syntax, rather than unified syntax.  In 
> legacy syntax there was no need to specify the flag-clobbering behaviour 
> of instructions.
>
> For example, the longjump code does not make sense otherwise as there is 
> no explicit comparison operation, yet there is a conditional branch near 
> the end:
>
>  > 	sub	r0, r0, #40
>  >   	ldmia	r0!, {r4, r5, r6, r7}
>  >   	/* Return the result argument, or 1 if it is zero.  */
>  >   	mov	r0, r1  // <<<< implicit flag set ....
>  >   	bne	1f	// <<<< ... only makes sense in legacy syntax
>  > 	mov	r0, #1
>  >   1:
>
> How did you build and test this?
>
> R.

Re: [PATCH] Replace insns in ARMv6-M setjmp impl with flag-setting variants

[Emmanuel Blot]

Hi Christos,

On 9 Oct 2019, at 15:27, Christos Gentsos wrote:

> Oh I see, you're totally right.
>
> I was trying to use Clang to compile newlib, I wasn't aware that its
> assembler didn't fully support the legacy ARM syntax - it was trying 
> to
> parse it as unified syntax code. Using GCC it compiles just fine.

FWIW, there are a few issues to build newlib with clang for ARMv6-M and 
ARMv7-(E)M, but I’ve been building and using it with several clang 
versions (with the help of a couple of patches) with no noticeable 
issues on baremetal targets.

Emmanuel.