Make more macro checks ARMv8-M baseline proof

[Richard Earnshaw (lists)]

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.

R.