[ARM/FDPIC,v5,13/21,ARM] FDPIC: Force LSB bit for PC in Cortex-M architecture

Message ID 20190515124006.25840-14-christophe.lyon@st.com
State Superseded
Headers show
Series
  • FDPIC ABI for ARM
Related show

Commit Message

Christophe Lyon May 15, 2019, 12:39 p.m.
Without this, when we are unwinding across a signal frame we can jump
to an even address which leads to an exception.

This is needed in __gnu_persnality_sigframe_fdpic() when restoring the
PC from the signal frame since the PC saved by the kernel has the LSB
bit set to zero.

2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>
	Mickaël Guêné <mickael.guene@st.com>

	libgcc/
	* config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m
	architecture.

Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

-- 
2.6.3

Comments

Kyrill Tkachov Aug. 29, 2019, 3:32 p.m. | #1
Hi Christophe,

On 5/15/19 1:39 PM, Christophe Lyon wrote:
> Without this, when we are unwinding across a signal frame we can jump

> to an even address which leads to an exception.

>

> This is needed in __gnu_persnality_sigframe_fdpic() when restoring the

> PC from the signal frame since the PC saved by the kernel has the LSB

> bit set to zero.

>

> 2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>

>         Mickaël Guêné <mickael.guene@st.com>

>

>         libgcc/

>         * config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m

>         architecture.

>

> Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

>

> diff --git a/libgcc/config/arm/unwind-arm.c 

> b/libgcc/config/arm/unwind-arm.c

> index 9ba73e7..ba47150 100644

> --- a/libgcc/config/arm/unwind-arm.c

> +++ b/libgcc/config/arm/unwind-arm.c

> @@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set 

> (_Unwind_Context *context,

>          return _UVRSR_FAILED;

>

>        vrs->core.r[regno] = *(_uw *) valuep;

> +#if defined(__ARM_ARCH_7M__)

> +      /* Force LSB bit since we always run thumb code.  */

> +      if (regno == 15)

> +       vrs->core.r[regno] |= 1;

> +#endif


Hmm, this looks quite specific. There are other architectures that are 
thumb-only too (6-M, 7E-M etc).

Would checking for __thumb__ be better?

Thanks,

Kyrill


>        return _UVRSR_OK;

>

>      case _UVRSC_VFP:

> -- 

> 2.6.3

>
Christophe Lyon Sept. 5, 2019, 8:30 a.m. | #2
On Thu, 29 Aug 2019 at 17:32, Kyrill Tkachov
<kyrylo.tkachov@foss.arm.com> wrote:
>

> Hi Christophe,

>

> On 5/15/19 1:39 PM, Christophe Lyon wrote:

> > Without this, when we are unwinding across a signal frame we can jump

> > to an even address which leads to an exception.

> >

> > This is needed in __gnu_persnality_sigframe_fdpic() when restoring the

> > PC from the signal frame since the PC saved by the kernel has the LSB

> > bit set to zero.

> >

> > 2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>

> >         Mickaël Guêné <mickael.guene@st.com>

> >

> >         libgcc/

> >         * config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m

> >         architecture.

> >

> > Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

> >

> > diff --git a/libgcc/config/arm/unwind-arm.c

> > b/libgcc/config/arm/unwind-arm.c

> > index 9ba73e7..ba47150 100644

> > --- a/libgcc/config/arm/unwind-arm.c

> > +++ b/libgcc/config/arm/unwind-arm.c

> > @@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set

> > (_Unwind_Context *context,

> >          return _UVRSR_FAILED;

> >

> >        vrs->core.r[regno] = *(_uw *) valuep;

> > +#if defined(__ARM_ARCH_7M__)

> > +      /* Force LSB bit since we always run thumb code.  */

> > +      if (regno == 15)

> > +       vrs->core.r[regno] |= 1;

> > +#endif

>

> Hmm, this looks quite specific. There are other architectures that are

> thumb-only too (6-M, 7E-M etc).

>

> Would checking for __thumb__ be better?

>

Right.
The attached updated patch also uses R_PC instead of 15.

Christophe

> Thanks,

>

> Kyrill

>

>

> >        return _UVRSR_OK;

> >

> >      case _UVRSC_VFP:

> > --

> > 2.6.3

> >
commit d50dfb233059bc5a110117047fe8f60d6580f095
Author: Christophe Lyon <christophe.lyon@linaro.org>
Date:   Thu Feb 8 14:52:02 2018 +0100

    [ARM] FDPIC: Force LSB bit for PC in Cortex-M architecture
    
    Without this, when we are unwinding across a signal frame we can jump
    to an even address which leads to an exception.
    
    This is needed in __gnu_persnality_sigframe_fdpic() when restoring the
    PC from the signal frame since the PC saved by the kernel has the LSB
    bit set to zero.
    
    2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>
    	Mickaël Guêné <mickael.guene@st.com>
    
    	libgcc/
    	* config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle thumb-only
    	architecture.
    
    Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

diff --git a/libgcc/config/arm/unwind-arm.c b/libgcc/config/arm/unwind-arm.c
index 9ba73e7..8313ee0 100644
--- a/libgcc/config/arm/unwind-arm.c
+++ b/libgcc/config/arm/unwind-arm.c
@@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set (_Unwind_Context *context,
 	return _UVRSR_FAILED;
 
       vrs->core.r[regno] = *(_uw *) valuep;
+#if defined(__thumb__)
+      /* Force LSB bit since we always run thumb code.  */
+      if (regno == R_PC)
+	vrs->core.r[regno] |= 1;
+#endif
       return _UVRSR_OK;
 
     case _UVRSC_VFP:
Christophe Lyon Sept. 5, 2019, 8:31 a.m. | #3
Sorry, I forgot again to cc: Ian.

Thanks,

Christophe

On Thu, 5 Sep 2019 at 10:30, Christophe Lyon <christophe.lyon@linaro.org> wrote:
>

> On Thu, 29 Aug 2019 at 17:32, Kyrill Tkachov

> <kyrylo.tkachov@foss.arm.com> wrote:

> >

> > Hi Christophe,

> >

> > On 5/15/19 1:39 PM, Christophe Lyon wrote:

> > > Without this, when we are unwinding across a signal frame we can jump

> > > to an even address which leads to an exception.

> > >

> > > This is needed in __gnu_persnality_sigframe_fdpic() when restoring the

> > > PC from the signal frame since the PC saved by the kernel has the LSB

> > > bit set to zero.

> > >

> > > 2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>

> > >         Mickaël Guêné <mickael.guene@st.com>

> > >

> > >         libgcc/

> > >         * config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m

> > >         architecture.

> > >

> > > Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

> > >

> > > diff --git a/libgcc/config/arm/unwind-arm.c

> > > b/libgcc/config/arm/unwind-arm.c

> > > index 9ba73e7..ba47150 100644

> > > --- a/libgcc/config/arm/unwind-arm.c

> > > +++ b/libgcc/config/arm/unwind-arm.c

> > > @@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set

> > > (_Unwind_Context *context,

> > >          return _UVRSR_FAILED;

> > >

> > >        vrs->core.r[regno] = *(_uw *) valuep;

> > > +#if defined(__ARM_ARCH_7M__)

> > > +      /* Force LSB bit since we always run thumb code.  */

> > > +      if (regno == 15)

> > > +       vrs->core.r[regno] |= 1;

> > > +#endif

> >

> > Hmm, this looks quite specific. There are other architectures that are

> > thumb-only too (6-M, 7E-M etc).

> >

> > Would checking for __thumb__ be better?

> >

> Right.

> The attached updated patch also uses R_PC instead of 15.

>

> Christophe

>

> > Thanks,

> >

> > Kyrill

> >

> >

> > >        return _UVRSR_OK;

> > >

> > >      case _UVRSC_VFP:

> > > --

> > > 2.6.3

> > >
Kyrill Tkachov Sept. 5, 2019, 9:03 a.m. | #4
Hi Christophe,

On 9/5/19 9:30 AM, Christophe Lyon wrote:
> On Thu, 29 Aug 2019 at 17:32, Kyrill Tkachov

> <kyrylo.tkachov@foss.arm.com> wrote:

>> Hi Christophe,

>>

>> On 5/15/19 1:39 PM, Christophe Lyon wrote:

>>> Without this, when we are unwinding across a signal frame we can jump

>>> to an even address which leads to an exception.

>>>

>>> This is needed in __gnu_persnality_sigframe_fdpic() when restoring the

>>> PC from the signal frame since the PC saved by the kernel has the LSB

>>> bit set to zero.

>>>

>>> 2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>

>>>          Mickaël Guêné <mickael.guene@st.com>

>>>

>>>          libgcc/

>>>          * config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m

>>>          architecture.

>>>

>>> Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

>>>

>>> diff --git a/libgcc/config/arm/unwind-arm.c

>>> b/libgcc/config/arm/unwind-arm.c

>>> index 9ba73e7..ba47150 100644

>>> --- a/libgcc/config/arm/unwind-arm.c

>>> +++ b/libgcc/config/arm/unwind-arm.c

>>> @@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set

>>> (_Unwind_Context *context,

>>>           return _UVRSR_FAILED;

>>>

>>>         vrs->core.r[regno] = *(_uw *) valuep;

>>> +#if defined(__ARM_ARCH_7M__)

>>> +      /* Force LSB bit since we always run thumb code.  */

>>> +      if (regno == 15)

>>> +       vrs->core.r[regno] |= 1;

>>> +#endif

>> Hmm, this looks quite specific. There are other architectures that are

>> thumb-only too (6-M, 7E-M etc).

>>

>> Would checking for __thumb__ be better?

>>

> Right.

> The attached updated patch also uses R_PC instead of 15.



Looks ok to me but we'll need to make sure this doesn't break non-FDPIC 
targets now.

A bootstrap and test of an arm-none-linux-gnueabihf targeting thumb 
should do it.

Thanks,

Kyrill


>

> Christophe

>

>> Thanks,

>>

>> Kyrill

>>

>>

>>>         return _UVRSR_OK;

>>>

>>>       case _UVRSC_VFP:

>>> --

>>> 2.6.3

>>>
Ian Lance Taylor Sept. 5, 2019, 8:55 p.m. | #5
Christophe Lyon <christophe.lyon@linaro.org> writes:

> Sorry, I forgot again to cc: Ian.


As far as I'm concerned, it's fine for architecture maintainers to
approve changes to architecture-specific files in libgcc.

Ian



> On Thu, 5 Sep 2019 at 10:30, Christophe Lyon <christophe.lyon@linaro.org> wrote:

>>

>> On Thu, 29 Aug 2019 at 17:32, Kyrill Tkachov

>> <kyrylo.tkachov@foss.arm.com> wrote:

>> >

>> > Hi Christophe,

>> >

>> > On 5/15/19 1:39 PM, Christophe Lyon wrote:

>> > > Without this, when we are unwinding across a signal frame we can jump

>> > > to an even address which leads to an exception.

>> > >

>> > > This is needed in __gnu_persnality_sigframe_fdpic() when restoring the

>> > > PC from the signal frame since the PC saved by the kernel has the LSB

>> > > bit set to zero.

>> > >

>> > > 2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>

>> > >         Mickaël Guêné <mickael.guene@st.com>

>> > >

>> > >         libgcc/

>> > >         * config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m

>> > >         architecture.

>> > >

>> > > Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

>> > >

>> > > diff --git a/libgcc/config/arm/unwind-arm.c

>> > > b/libgcc/config/arm/unwind-arm.c

>> > > index 9ba73e7..ba47150 100644

>> > > --- a/libgcc/config/arm/unwind-arm.c

>> > > +++ b/libgcc/config/arm/unwind-arm.c

>> > > @@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set

>> > > (_Unwind_Context *context,

>> > >          return _UVRSR_FAILED;

>> > >

>> > >        vrs->core.r[regno] = *(_uw *) valuep;

>> > > +#if defined(__ARM_ARCH_7M__)

>> > > +      /* Force LSB bit since we always run thumb code.  */

>> > > +      if (regno == 15)

>> > > +       vrs->core.r[regno] |= 1;

>> > > +#endif

>> >

>> > Hmm, this looks quite specific. There are other architectures that are

>> > thumb-only too (6-M, 7E-M etc).

>> >

>> > Would checking for __thumb__ be better?

>> >

>> Right.

>> The attached updated patch also uses R_PC instead of 15.

>>

>> Christophe

>>

>> > Thanks,

>> >

>> > Kyrill

>> >

>> >

>> > >        return _UVRSR_OK;

>> > >

>> > >      case _UVRSC_VFP:

>> > > --

>> > > 2.6.3

>> > >
Christophe Lyon Sept. 9, 2019, 8:58 a.m. | #6
On Thu, 5 Sep 2019 at 11:03, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>

> Hi Christophe,

>

> On 9/5/19 9:30 AM, Christophe Lyon wrote:

> > On Thu, 29 Aug 2019 at 17:32, Kyrill Tkachov

> > <kyrylo.tkachov@foss.arm.com> wrote:

> >> Hi Christophe,

> >>

> >> On 5/15/19 1:39 PM, Christophe Lyon wrote:

> >>> Without this, when we are unwinding across a signal frame we can jump

> >>> to an even address which leads to an exception.

> >>>

> >>> This is needed in __gnu_persnality_sigframe_fdpic() when restoring the

> >>> PC from the signal frame since the PC saved by the kernel has the LSB

> >>> bit set to zero.

> >>>

> >>> 2019-XX-XX  Christophe Lyon  <christophe.lyon@st.com>

> >>>          Mickaël Guêné <mickael.guene@st.com>

> >>>

> >>>          libgcc/

> >>>          * config/arm/unwind-arm.c (_Unwind_VRS_Set): Handle v7m

> >>>          architecture.

> >>>

> >>> Change-Id: Ie84de548226bcf1751e19a09e8f091fb3013ccea

> >>>

> >>> diff --git a/libgcc/config/arm/unwind-arm.c

> >>> b/libgcc/config/arm/unwind-arm.c

> >>> index 9ba73e7..ba47150 100644

> >>> --- a/libgcc/config/arm/unwind-arm.c

> >>> +++ b/libgcc/config/arm/unwind-arm.c

> >>> @@ -199,6 +199,11 @@ _Unwind_VRS_Result _Unwind_VRS_Set

> >>> (_Unwind_Context *context,

> >>>           return _UVRSR_FAILED;

> >>>

> >>>         vrs->core.r[regno] = *(_uw *) valuep;

> >>> +#if defined(__ARM_ARCH_7M__)

> >>> +      /* Force LSB bit since we always run thumb code.  */

> >>> +      if (regno == 15)

> >>> +       vrs->core.r[regno] |= 1;

> >>> +#endif

> >> Hmm, this looks quite specific. There are other architectures that are

> >> thumb-only too (6-M, 7E-M etc).

> >>

> >> Would checking for __thumb__ be better?

> >>

> > Right.

> > The attached updated patch also uses R_PC instead of 15.

>

>

> Looks ok to me but we'll need to make sure this doesn't break non-FDPIC

> targets now.

>

> A bootstrap and test of an arm-none-linux-gnueabihf targeting thumb

> should do it.

>


Bootstrap of the whole series OK, modulo the problems with the tests
discussed in patch 20.
(some tests became unsupported on arm-linux-gnueabihf with thumb target)

Christophe

> Thanks,

>

> Kyrill

>

>

> >

> > Christophe

> >

> >> Thanks,

> >>

> >> Kyrill

> >>

> >>

> >>>         return _UVRSR_OK;

> >>>

> >>>       case _UVRSC_VFP:

> >>> --

> >>> 2.6.3

> >>>

Patch

diff --git a/libgcc/config/arm/unwind-arm.c b/libgcc/config/arm/unwind-arm.c
index 9ba73e7..ba47150 100644
--- a/libgcc/config/arm/unwind-arm.c
+++ b/libgcc/config/arm/unwind-arm.c
@@ -199,6 +199,11 @@  _Unwind_VRS_Result _Unwind_VRS_Set (_Unwind_Context *context,
 	return _UVRSR_FAILED;
 
       vrs->core.r[regno] = *(_uw *) valuep;
+#if defined(__ARM_ARCH_7M__)
+      /* Force LSB bit since we always run thumb code.  */
+      if (regno == 15)
+	vrs->core.r[regno] |= 1;
+#endif
       return _UVRSR_OK;
 
     case _UVRSC_VFP: