diff mbox series

[v3,13/13] aarch64: fix _mcount for pac-ret

Message ID 3d422103bcd42547829b53fa07d3891e5bfabba3.1589552055.git.szabolcs.nagy@arm.com
State Superseded
Headers
Series aarch64: branch protection support |

Commit Message

Szabolcs Nagy May 15, 2020, 2:40 p.m. UTC 
  gcc -pg with -mbranch-protection=pac-ret passes signed return address
to _mcount, so _mcount now has to always strip pac from the frompc
since that's from user code that may be built with pac-ret.

This is a backward incompatible _mcount abi change introduced by
return address signing support in gcc-7.

TODO: fix -pg on the gcc side?
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791
---
 sysdeps/aarch64/machine-gmon.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Adhemerval Zanella Netto May 26, 2020, 11:33 a.m. UTC | #1 
On 15/05/2020 11:40, Szabolcs Nagy wrote:
> gcc -pg with -mbranch-protection=pac-ret passes signed return address
> to _mcount, so _mcount now has to always strip pac from the frompc
> since that's from user code that may be built with pac-ret.
> 
> This is a backward incompatible _mcount abi change introduced by
> return address signing support in gcc-7.

Which change are you referring about specifically?

Besides it, LGTM.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> 
> TODO: fix -pg on the gcc side?
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791
> ---
>  sysdeps/aarch64/machine-gmon.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h
> index 730a23b781..328cbdda16 100644
> --- a/sysdeps/aarch64/machine-gmon.h
> +++ b/sysdeps/aarch64/machine-gmon.h
> @@ -30,5 +30,5 @@ static inline void mcount_internal (u_long frompc, u_long selfpc)
>  #define MCOUNT                                                    \
>  void __mcount (void *frompc)                                      \
>  {                                                                 \
> -  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
> +  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \
>  }
>
Szabolcs Nagy May 26, 2020, 6:38 p.m. UTC | #2 
The 05/26/2020 08:33, Adhemerval Zanella via Libc-alpha wrote:
> 
> 
> On 15/05/2020 11:40, Szabolcs Nagy wrote:
> > gcc -pg with -mbranch-protection=pac-ret passes signed return address
> > to _mcount, so _mcount now has to always strip pac from the frompc
> > since that's from user code that may be built with pac-ret.
> > 
> > This is a backward incompatible _mcount abi change introduced by
> > return address signing support in gcc-7.
> 
> Which change are you referring about specifically?

gcc-7 introduced -msigned-return-address (which was
later deprecated by -mbranch-protection=pac-ret) and
the code generation with -pg is not compatible with
existing _mcount runtime implementations (because
now _mcount needs unconditional xpac)

i'm still trying to see if this _mcount change is
needed or we can change gcc to do the right thing
(and not pass signed address to _mcount), but i
need internal consensus about such abi break first.

> 
> Besides it, LGTM.
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> > 
> > TODO: fix -pg on the gcc side?
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791
> > ---
> >  sysdeps/aarch64/machine-gmon.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h
> > index 730a23b781..328cbdda16 100644
> > --- a/sysdeps/aarch64/machine-gmon.h
> > +++ b/sysdeps/aarch64/machine-gmon.h
> > @@ -30,5 +30,5 @@ static inline void mcount_internal (u_long frompc, u_long selfpc)
> >  #define MCOUNT                                                    \
> >  void __mcount (void *frompc)                                      \
> >  {                                                                 \
> > -  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
> > +  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \
> >  }
> > 

--
Adhemerval Zanella Netto May 26, 2020, 7:13 p.m. UTC | #3 
On 26/05/2020 15:38, Szabolcs Nagy wrote:
> The 05/26/2020 08:33, Adhemerval Zanella via Libc-alpha wrote:
>>
>>
>> On 15/05/2020 11:40, Szabolcs Nagy wrote:
>>> gcc -pg with -mbranch-protection=pac-ret passes signed return address
>>> to _mcount, so _mcount now has to always strip pac from the frompc
>>> since that's from user code that may be built with pac-ret.
>>>
>>> This is a backward incompatible _mcount abi change introduced by
>>> return address signing support in gcc-7.
>>
>> Which change are you referring about specifically?
> 
> gcc-7 introduced -msigned-return-address (which was
> later deprecated by -mbranch-protection=pac-ret) and
> the code generation with -pg is not compatible with
> existing _mcount runtime implementations (because
> now _mcount needs unconditional xpac)

Thanks, could you add this explanation on commit message as
well?

> 
> i'm still trying to see if this _mcount change is
> needed or we can change gcc to do the right thing
> (and not pass signed address to _mcount), but i
> need internal consensus about such abi break first.
> 
>>
>> Besides it, LGTM.
>>
>> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>>
>>>
>>> TODO: fix -pg on the gcc side?
>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94791
>>> ---
>>>  sysdeps/aarch64/machine-gmon.h | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h
>>> index 730a23b781..328cbdda16 100644
>>> --- a/sysdeps/aarch64/machine-gmon.h
>>> +++ b/sysdeps/aarch64/machine-gmon.h
>>> @@ -30,5 +30,5 @@ static inline void mcount_internal (u_long frompc, u_long selfpc)
>>>  #define MCOUNT                                                    \
>>>  void __mcount (void *frompc)                                      \
>>>  {                                                                 \
>>> -  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
>>> +  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \
>>>  }
>>>
>
diff mbox series

Patch

diff --git a/sysdeps/aarch64/machine-gmon.h b/sysdeps/aarch64/machine-gmon.h
index 730a23b781..328cbdda16 100644
--- a/sysdeps/aarch64/machine-gmon.h
+++ b/sysdeps/aarch64/machine-gmon.h
@@ -30,5 +30,5 @@  static inline void mcount_internal (u_long frompc, u_long selfpc)
 #define MCOUNT                                                    \
 void __mcount (void *frompc)                                      \
 {                                                                 \
-  mcount_internal ((u_long) frompc, (u_long) RETURN_ADDRESS (0)); \
+  mcount_internal ((u_long) strip_pac (frompc), (u_long) RETURN_ADDRESS (0)); \
 }