Re: Make more macro checks ARMv8-M baseline proof

On Thu, 11 Apr 2019 at 15:35, Richard Earnshaw (lists)
<[email protected]> wrote:
>
> On 10/04/2019 16:12, Christophe Lyon wrote:
> > On Wed, 10 Apr 2019 at 15:22, Richard Earnshaw (lists)
> > <[email protected]> wrote:
> >>
> >> On 10/04/2019 13:52, Christophe Lyon wrote:
> >>> On Wed, 10 Apr 2019 at 14:38, Richard Earnshaw (lists)
> >>> <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>>>>> 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, pushed.

>
> > 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 <[email protected]>
> >> 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 <[email protected]>
> >> 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
>
>