Re: Make more macro checks ARMv8-M baseline proof

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 10/04/2019 16:12, Christophe Lyon wrote:
> On Wed, 10 Apr 2019 at 15:22, Richard Earnshaw (lists)
> <Richard.Earnshaw@arm.com> wrote:
>>
>> On 10/04/2019 13:52, Christophe Lyon wrote:
>>> On Wed, 10 Apr 2019 at 14:38, Richard Earnshaw (lists)
>>> <Richard.Earnshaw@arm.com> wrote:
>>>>
>>>> On 01/04/2019 18:33, Christophe Lyon wrote:
>>>>> I forgot that the suitable format in this project is via git
>>>>> format-patch, so here is the same patch in that format.
>>>>>
>>>>> Christophe
>>>>>
>>>>> On Mon, 1 Apr 2019 at 16:42, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> After Thomas' patch
>>>>>> (https://sourceware.org/ml/newlib/2016/msg00017.html), it seems that
>>>>>> some changes committed to libgloss are missing in newlib.
>>>>>> I noticed the problem when building a toolchain where GCC is
>>>>>> configured --with-cpu=cortex-m{0,23}.
>>>>>>
>>>>>> This small patch brings the necessary changes from libgloss to newlib.
>>>>>> With this, the GCC toolchain build completes.
>>>>>>
>>>>>> OK?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Christophe
>>>>>>
>>>>>> 0001-Make-more-macro-checks-ARMv8-M-baseline-proof.patch
>>>>>>
>>>>>> From 1ea9c597edbece03bce084afe33b45f1ee9ff67f Mon Sep 17 00:00:00 2001
>>>>>> From: Christophe Lyon <christophe.lyon@linaro.org>
>>>>>> Date: Mon, 1 Apr 2019 17:30:53 +0000
>>>>>> Subject: [PATCH 1/1] Make more macro checks ARMv8-M baseline proof.
>>>>>>
>>>>>> Commit 69f4c4029183fb26d2fcae00790881620c1978a3 improved most
>>>>>> macro checks to be ARMv8-M baseline proof, but missed a few
>>>>>> occurrences which otherwise fail to build when using a CPU setting
>>>>>> such as cortex-m0 or cortex-m23. This patch brings the same
>>>>>> changes as the ones that were committed to libgloss at that time.
>>>>>>
>>>>>>      newlib:
>>>>>>      * libc/sys/arm/crt0.S: Use THUMB1_ONLY rather than
>>>>>>      __ARM_ARCH_6M__.
>>>>>>      * libc/sys/arm/trap.S: Use PREFER_THUMB rather than __thumb2__.
>>>>>> ---
>>>>>>  newlib/libc/sys/arm/crt0.S | 8 ++++----
>>>>>>  newlib/libc/sys/arm/trap.S | 3 ++-
>>>>>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/newlib/libc/sys/arm/crt0.S b/newlib/libc/sys/arm/crt0.S
>>>>>> index 64d4259..8c9f7be 100644
>>>>>> --- a/newlib/libc/sys/arm/crt0.S
>>>>>> +++ b/newlib/libc/sys/arm/crt0.S
>>>>>> @@ -85,7 +85,7 @@
>>>>>>
>>>>>>      /*  Stack limit is at end of data.  */
>>>>>>      /*  Allow slop for stack overflow handling and small frames.  */
>>>>>> -#ifdef __ARM_ARCH_6M__
>>>>>> +#ifdef THUMB1_ONLY
>>>>>>      ldr     r0, .LC2
>>>>>>      adds    r0, #128
>>>>>>      adds    r0, #128
>>>>>> @@ -137,7 +137,7 @@
>>>>>>      beq     .LC27
>>>>>>
>>>>>>      /*  Allow slop for stack overflow handling and small frames.  */
>>>>>> -#ifdef __ARM_ARCH_6M__
>>>>>> +#ifdef THUMB1_ONLY
>>>>>>      adds    r2, #128
>>>>>>      adds    r2, #128
>>>>>>      mov     sl, r2
>>>>>> @@ -164,7 +164,7 @@
>>>>>>  #ifdef __thumb2__
>>>>>>      it      eq
>>>>>>  #endif
>>>>>> -#ifdef __ARM_ARCH_6M__
>>>>>> +#ifdef THUMB1_ONLY
>>>>>>      bne     .LC28
>>>>>>      ldr     r3, .LC0
>>>>>>  .LC28:
>>>>>> @@ -219,7 +219,7 @@
>>>>>>         this default 64k is enough for the program being executed.
>>>>>>         However, it ensures that this simple crt0 world will not
>>>>>>         immediately cause an overflow event:  */
>>>>>> -#ifdef __ARM_ARCH_6M__
>>>>>> +#ifdef THUMB1_ONLY
>>>>>>      movs    r2, #64
>>>>>>      lsls    r2, r2, #10
>>>>>>      subs    r2, r3, r2
>>>>
>>>> The above are all OK.
>>>>
>>>>>> diff --git a/newlib/libc/sys/arm/trap.S b/newlib/libc/sys/arm/trap.S
>>>>>> index 21b6937..d854b57 100644
>>>>>> --- a/newlib/libc/sys/arm/trap.S
>>>>>> +++ b/newlib/libc/sys/arm/trap.S
>>>>>> @@ -1,5 +1,6 @@
>>>>>> +#include "arm.h"
>>>>>>          /* Run-time exception support */
>>>>>> -#if !defined(__thumb2__)
>>>>>> +#ifndef PREFER_THUMB
>>>>>>  #include "swi.h"
>>>>>>
>>>>>>  /* .text is used instead of .section .text so it works with arm-aout too.  */
>>>>
>>>> I'm not sure I understand this hunk.  PREFER_THUMB is no the same as
>>>> __thumb2__, so I think this needs more clarification.  Why would it be
>>>> right to omit the contents of this file entirely?  what provides this
>>>> API if this doesn't?
>>>>
>>>> I'm not sure I like the PREFER_THUMB macro anyway: preference is not the
>>>> same as 'must be' or 'can only be'; so this is all somewhat confusing.
>>>
>>> Well, this is exactly the same change as the one applied by commit
>>> 69f4c4029183fb26d2fcae00790881620c1978a3 to libgloss/arm/trap.S.
>>>
>>> Do you mean both trap.S could/should be different, or that commit
>>> 69f4c4029183fb26d2fcae00790881620c1978a3 is wrong?
>>>
>>> Christophe
>>>
>>
>> I didn't review that patch :-(
>>
>> The effect of Thomas' patch was to remove this API on systems that built
>> newlib in Thumb1 mode for use on Armv4t through Armv6 (ie on systems
>> with both Thumb1 and ARM ISAs); that was probably unintentional.
>>
>> But in fact, I think even the original patch from Paul Brook was
>> probably not correct as the effect was to remove this API on A-profile
>> targets based solely on which ISA was selected for building the library.
>>
>> Does this all matter in reality?  Probably not.  Looking in more detail
>> I see the code in this function is to support the old APCS chunked stack
>> variant, which dates back to the Acorn days; GCC hasn't really supported
>> that since forever, and it certainly won't work in EABI environments.
>>
>> So a better patch would be to change this to
>>
>> #ifndef __ARM_EABI__
>>
>> and to make the same change in libgloss.
>>
> 
> Thanks for the clarification, I wouldn't have inferred the APCS part myself.
> 
> Here are two patches: the 1st one contains the part you already approved,
> the 2nd one contains the __ARM_EABI__ change for both newlib and libgloss.
> 
> (I split the change into two parts because now they are not strictly related)
> 
> OK?
> 

Yes, both changes are fine.

R.

> Thanks,
> 
> Christophe
> 
>> R.
>>
>> 0002-arm-Include-code-in-trap.S-for-APCS-only.patch
>>
>> From 390afa88e3d03ce4838f377978a9d7f2c9d468a7 Mon Sep 17 00:00:00 2001
>> From: Christophe Lyon <christophe.lyon@linaro.org>
>> Date: Wed, 10 Apr 2019 15:04:13 +0000
>> Subject: [PATCH 2/2] [arm] Include code in trap.S for APCS only.
>>
>> The code in trap.S is to support the old APCS chunked stack variant,
>> which dates back to the Acorn days, so put it under #ifndef
>> __ARM_EABI__.
>>
>> 	* libgloss/arm/trap.S: Use __ARM_EABI rather than PREFER_THUMB.
>> 	* newlib/libc/sys/arm/trap.S: Use __ARM_EABI rather than
>> 	__thumb2__.
>> ---
>>  libgloss/arm/trap.S        | 2 +-
>>  newlib/libc/sys/arm/trap.S | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libgloss/arm/trap.S b/libgloss/arm/trap.S
>> index d854b57..845ad01 100644
>> --- a/libgloss/arm/trap.S
>> +++ b/libgloss/arm/trap.S
>> @@ -1,6 +1,6 @@
>>  #include "arm.h"
>>          /* Run-time exception support */
>> -#ifndef PREFER_THUMB
>> +#ifndef __ARM_EABI__
>>  #include "swi.h"
>>  
>>  /* .text is used instead of .section .text so it works with arm-aout too.  */
>> diff --git a/newlib/libc/sys/arm/trap.S b/newlib/libc/sys/arm/trap.S
>> index 21b6937..681b3db 100644
>> --- a/newlib/libc/sys/arm/trap.S
>> +++ b/newlib/libc/sys/arm/trap.S
>> @@ -1,5 +1,5 @@
>>          /* Run-time exception support */
>> -#if !defined(__thumb2__)
>> +#ifndef __ARM_EABI__
>>  #include "swi.h"
>>  
>>  /* .text is used instead of .section .text so it works with arm-aout too.  */
>>
>> 0001-Make-more-macro-checks-ARMv8-M-baseline-proof.patch
>>
>> From 2c57cf3aa28614ba2529c1f45bc45fb971121535 Mon Sep 17 00:00:00 2001
>> From: Christophe Lyon <christophe.lyon@linaro.org>
>> Date: Mon, 1 Apr 2019 17:30:53 +0000
>> Subject: [PATCH 1/2] Make more macro checks ARMv8-M baseline proof.
>>
>> Commit 69f4c4029183fb26d2fcae00790881620c1978a3 improved most
>> macro checks to be ARMv8-M baseline proof, but missed a few
>> occurrences which otherwise fail to build when using a CPU setting
>> such as cortex-m0 or cortex-m23. This patch brings the same
>> changes as the ones that were committed to libgloss at that time.
>>
>> 	newlib:
>> 	* libc/sys/arm/crt0.S: Use THUMB1_ONLY rather than
>> 	__ARM_ARCH_6M__.
>> ---
>>  newlib/libc/sys/arm/crt0.S | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/newlib/libc/sys/arm/crt0.S b/newlib/libc/sys/arm/crt0.S
>> index 64d4259..8c9f7be 100644
>> --- a/newlib/libc/sys/arm/crt0.S
>> +++ b/newlib/libc/sys/arm/crt0.S
>> @@ -85,7 +85,7 @@
>>  
>>  	/*  Stack limit is at end of data.  */
>>  	/*  Allow slop for stack overflow handling and small frames.  */
>> -#ifdef __ARM_ARCH_6M__
>> +#ifdef THUMB1_ONLY
>>  	ldr	r0, .LC2
>>  	adds	r0, #128
>>  	adds	r0, #128
>> @@ -137,7 +137,7 @@
>>  	beq	.LC27
>>  
>>  	/*  Allow slop for stack overflow handling and small frames.  */
>> -#ifdef __ARM_ARCH_6M__
>> +#ifdef THUMB1_ONLY
>>  	adds	r2, #128
>>  	adds	r2, #128
>>  	mov	sl, r2
>> @@ -164,7 +164,7 @@
>>  #ifdef __thumb2__
>>  	it	eq
>>  #endif	
>> -#ifdef __ARM_ARCH_6M__
>> +#ifdef THUMB1_ONLY
>>  	bne	.LC28
>>  	ldr	r3, .LC0
>>  .LC28:
>> @@ -219,7 +219,7 @@
>>  	   this default 64k is enough for the program being executed.
>>  	   However, it ensures that this simple crt0 world will not
>>  	   immediately cause an overflow event:  */
>> -#ifdef __ARM_ARCH_6M__
>> +#ifdef THUMB1_ONLY
>>  	movs	r2, #64
>>  	lsls	r2, r2, #10
>>  	subs	r2, r3, r2