Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Christophe Lyon]

On 20 January 2017 at 12:54, Jiong Wang  wrote:
> On 20/01/17 10:30, Christophe Lyon wrote:
>>
>> error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
>> function)
>>fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
>> ^
>
>
> Hi Christophe, could you please confirm you svn revision please?
>
> I do have done bootstrap and regression on both x86 and aarch64 before
> commit this patch.  I had forgotten to "svn add" one header file, but
> add
> it
> later.
>
 The failures started with r244673, and are still present with r244687.
 When did you add the missing file?
>>>
>>>
>>> It was r244674, https://gcc.gnu.org/ml/gcc-cvs/2017-01/msg00689.html,  so
>>> should have been included in your code.  The faliure looks strange to me
>>> then,  I will svn up and re-start a fresh bootstrap on AArch64.
>>>
>> The file is present in my git clone.
>> I'm not bootstrapping on AArch64, I'm building a cross-compiler on x86_64,
>> but it shouldn't matter.
>
>
> Hi Christophe,
>
>   Thanks, I reproduced this on cross linux environment, the reason is the
> header file is not included because of the inhabit_libc guard, while the
> unwinder header file should always be included.
>
>I will committed the attached patch as obvious, once I finished a fresh
> bootstrap, cross elf, cross linux.
>

After you committed this (r244710), my cross build for aarch64-linux-gnu
now passes.

I'm now left with the build failure reported by Andrew on aarch64(_be)-none-elf.

Thanks for the fix.

Christophe.

>Thanks.
>
> libgcc/
>
> 2017-01-20  Jiong Wang  
>
> * config/aarch64/linux-unwind.h: Always include aarch64-unwind.h.
>
>

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Richard Earnshaw (lists)]

On 20/01/17 11:54, Jiong Wang wrote:
> On 20/01/17 10:30, Christophe Lyon wrote:
>> error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
>> function)
>>fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
>> ^
>
> Hi Christophe, could you please confirm you svn revision please?
>
> I do have done bootstrap and regression on both x86 and aarch64 before
> commit this patch.  I had forgotten to "svn add" one header file,
> but add
> it
> later.
>
 The failures started with r244673, and are still present with r244687.
 When did you add the missing file?
>>>
>>> It was r244674,
>>> https://gcc.gnu.org/ml/gcc-cvs/2017-01/msg00689.html,  so
>>> should have been included in your code.  The faliure looks strange to me
>>> then,  I will svn up and re-start a fresh bootstrap on AArch64.
>>>
>> The file is present in my git clone.
>> I'm not bootstrapping on AArch64, I'm building a cross-compiler on
>> x86_64,
>> but it shouldn't matter.
> 
> Hi Christophe,
> 
>   Thanks, I reproduced this on cross linux environment, the reason is
> the header file is not included because of the inhabit_libc guard, while
> the unwinder header file should always be included.
> 
>I will committed the attached patch as obvious, once I finished a
> fresh bootstrap, cross elf, cross linux.
> 

If this survives a cross-build, please just commit it.  It's very
unlikely that a native build will throw up any problems.

R.

>Thanks.
> 
> libgcc/
> 
> 2017-01-20  Jiong Wang  
> 
> * config/aarch64/linux-unwind.h: Always include aarch64-unwind.h.
> 
> 
> 
> k.patch
> 
> 
> diff --git a/libgcc/config/aarch64/linux-unwind.h 
> b/libgcc/config/aarch64/linux-unwind.h
> index a8fa1d5..70e5a8a 100644
> --- a/libgcc/config/aarch64/linux-unwind.h
> +++ b/libgcc/config/aarch64/linux-unwind.h
> @@ -20,11 +20,13 @@
> see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
> .  */
>  
> +/* Always include AArch64 unwinder header file.  */
> +#include "config/aarch64/aarch64-unwind.h"
> +
>  #ifndef inhibit_libc
>  
>  #include 
>  #include 
> -#include "config/aarch64/aarch64-unwind.h"
>  
>  
>  /* Since insns are always stored LE, on a BE system the opcodes will
> 

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Jiong Wang]

On 20/01/17 10:30, Christophe Lyon wrote:

error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
function)
   fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
^


Hi Christophe, could you please confirm you svn revision please?

I do have done bootstrap and regression on both x86 and aarch64 before
commit this patch.  I had forgotten to "svn add" one header file, but add
it
later.


The failures started with r244673, and are still present with r244687.
When did you add the missing file?


It was r244674, https://gcc.gnu.org/ml/gcc-cvs/2017-01/msg00689.html,  so
should have been included in your code.  The faliure looks strange to me
then,  I will svn up and re-start a fresh bootstrap on AArch64.


The file is present in my git clone.
I'm not bootstrapping on AArch64, I'm building a cross-compiler on x86_64,
but it shouldn't matter.


Hi Christophe,

  Thanks, I reproduced this on cross linux environment, the reason is the 
header file is not included because of the inhabit_libc guard, while the 
unwinder header file should always be included.

   I will committed the attached patch as obvious, once I finished a fresh 
bootstrap, cross elf, cross linux.

   Thanks.

libgcc/

2017-01-20  Jiong Wang  

* config/aarch64/linux-unwind.h: Always include aarch64-unwind.h.


diff --git a/libgcc/config/aarch64/linux-unwind.h b/libgcc/config/aarch64/linux-unwind.h
index a8fa1d5..70e5a8a 100644
--- a/libgcc/config/aarch64/linux-unwind.h
+++ b/libgcc/config/aarch64/linux-unwind.h
@@ -20,11 +20,13 @@
see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
.  */
 
+/* Always include AArch64 unwinder header file.  */
+#include "config/aarch64/aarch64-unwind.h"
+
 #ifndef inhibit_libc
 
 #include 
 #include 
-#include "config/aarch64/aarch64-unwind.h"
 
 
 /* Since insns are always stored LE, on a BE system the opcodes will

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Christophe Lyon]

On 20 January 2017 at 11:18, Jiong Wang  wrote:
>
>
> On 20/01/17 10:11, Christophe Lyon wrote:
>>
>>

 /tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:
 In function 'execute_cfa_program':


 /tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:1193:17:
 error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
 function)
   fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
^
>>>
>>>
>>> Hi Christophe, could you please confirm you svn revision please?
>>>
>>> I do have done bootstrap and regression on both x86 and aarch64 before
>>> commit this patch.  I had forgotten to "svn add" one header file, but add
>>> it
>>> later.
>>>
>> The failures started with r244673, and are still present with r244687.
>> When did you add the missing file?
>
>
> It was r244674, https://gcc.gnu.org/ml/gcc-cvs/2017-01/msg00689.html,  so
> should have been included in your code.  The faliure looks strange to me
> then,  I will svn up and re-start a fresh bootstrap on AArch64.
>

The file is present in my git clone.
I'm not bootstrapping on AArch64, I'm building a cross-compiler on x86_64,
but it shouldn't matter.

>>
>>> Thanks.
>>>
 Christophe
>>>
>>>
>

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Jiong Wang]

On 20/01/17 10:11, Christophe Lyon wrote:



/tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:
In function 'execute_cfa_program':

/tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:1193:17:
error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
function)
  fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
   ^


Hi Christophe, could you please confirm you svn revision please?

I do have done bootstrap and regression on both x86 and aarch64 before
commit this patch.  I had forgotten to "svn add" one header file, but add it
later.


The failures started with r244673, and are still present with r244687.
When did you add the missing file?


It was r244674, https://gcc.gnu.org/ml/gcc-cvs/2017-01/msg00689.html,  
so should have been included in your code.  The faliure looks strange to 
me then,  I will svn up and re-start a fresh bootstrap on AArch64.





Thanks.


Christophe

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Christophe Lyon]

On 20 January 2017 at 10:44, Jiong Wang  wrote:
>
>
> On 20/01/17 08:41, Christophe Lyon wrote:
>>
>> Hi Jiong,
>>
>> On 19 January 2017 at 15:46, Jiong Wang  wrote:
>>>
>>> Thanks for the review.
>>>
>>> On 19/01/17 14:18, Richard Earnshaw (lists) wrote:


>
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index
>
> 8085a42ace15d53f4cb0c6681717012d906a6d47..cf640135275deb76b820f8209fa51eacfd64c4a2
> 100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -136,6 +136,8 @@ struct _Unwind_Context
>   #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
> /* Context which has version/args_size/by_value fields.  */
>   #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
> +  /* Bit reserved on AArch64, return address has been signed with A
> key.
> */
> +#define RA_A_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)


 Why is this here?   It appears to only be used within the
 AArch64-specific header file.
>>>
>>>
>>> I was putting it here so that when we allocate the next general purpose
>>> bit,
>>> we
>>> know clearly that bit 3 is allocated to AArch64 already, and the new
>>> general
>>> bit
>>> needs to go to the next one.  This can avoid bit collision.
>>>
> ...
>
> +/* Frob exception handler's address kept in TARGET before installing
> into
> +   CURRENT context.  */
> +
> +static void *
> +uw_frob_return_addr (struct _Unwind_Context *current,
> + struct _Unwind_Context *target)
> +{
> +  void *ret_addr = __builtin_frob_return_addr (target->ra);
> +#ifdef MD_POST_FROB_EH_HANDLER_ADDR
> +  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
> +#endif
> +  return ret_addr;
> +}
> +


 I think this function should be marked inline.  The optimizers would
 probably inline it anyway, but it seems wrong for us to rely on that.
>>>
>>>
>>> Thanks, fixed.
>>>
>>> Does the updated patch looks OK to you know?
>>>
>>> libgcc/
>>>
>>> 2017-01-19  Jiong Wang  
>>>
>>>
>>>  * config/aarch64/aarch64-unwind.h: New file.
>>>  (DWARF_REGNUM_AARCH64_RA_STATE): Define.
>>>  (MD_POST_EXTRACT_ROOT_ADDR): Define.
>>>  (MD_POST_EXTRACT_FRAME_ADDR): Define.
>>>  (MD_POST_FROB_EH_HANDLER_ADDR): Define.
>>>  (MD_FROB_UPDATE_CONTEXT): Define.
>>>  (aarch64_post_extract_frame_addr): New function.
>>>  (aarch64_post_frob_eh_handler_addr): New function.
>>>  (aarch64_frob_update_context): New function.
>>>  * config/aarch64/linux-unwind.h: Include aarch64-unwind.h
>>>  * config.host (aarch64*-*-elf, aarch64*-*-rtems*,
>>> aarch64*-*-freebsd*):
>>>  Initialize md_unwind_header to include aarch64-unwind.h.
>>>  * unwind-dw2.c (struct _Unwind_Context): Define
>>> "RA_A_SIGNED_BIT".
>>>  (execute_cfa_program): Multiplex DW_CFA_GNU_window_save for
>>> __aarch64__.
>>>  (uw_update_context): Honor MD_POST_EXTRACT_FRAME_ADDR.
>>>  (uw_init_context_1): Honor MD_POST_EXTRACT_ROOT_ADDR.
>>>  (uw_frob_return_addr): New function.
>>>  (_Unwind_DebugHook): Use uw_frob_return_addr.
>>>
>> Since you committed this (r244673), GCC fails to build for AArch64:
>>
>> /tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:
>> In function 'execute_cfa_program':
>>
>> /tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:1193:17:
>> error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
>> function)
>>  fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
>>   ^
>
>
> Hi Christophe, could you please confirm you svn revision please?
>
> I do have done bootstrap and regression on both x86 and aarch64 before
> commit this patch.  I had forgotten to "svn add" one header file, but add it
> later.
>

The failures started with r244673, and are still present with r244687.
When did you add the missing file?

> Thanks.
>
>> Christophe
>
>

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Jiong Wang]

On 20/01/17 08:41, Christophe Lyon wrote:

Hi Jiong,

On 19 January 2017 at 15:46, Jiong Wang  wrote:

Thanks for the review.

On 19/01/17 14:18, Richard Earnshaw (lists) wrote:




diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index
8085a42ace15d53f4cb0c6681717012d906a6d47..cf640135275deb76b820f8209fa51eacfd64c4a2
100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -136,6 +136,8 @@ struct _Unwind_Context
  #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
/* Context which has version/args_size/by_value fields.  */
  #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
+  /* Bit reserved on AArch64, return address has been signed with A key.
*/
+#define RA_A_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)


Why is this here?   It appears to only be used within the
AArch64-specific header file.


I was putting it here so that when we allocate the next general purpose bit,
we
know clearly that bit 3 is allocated to AArch64 already, and the new general
bit
needs to go to the next one.  This can avoid bit collision.


...

+/* Frob exception handler's address kept in TARGET before installing
into
+   CURRENT context.  */
+
+static void *
+uw_frob_return_addr (struct _Unwind_Context *current,
+ struct _Unwind_Context *target)
+{
+  void *ret_addr = __builtin_frob_return_addr (target->ra);
+#ifdef MD_POST_FROB_EH_HANDLER_ADDR
+  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
+#endif
+  return ret_addr;
+}
+


I think this function should be marked inline.  The optimizers would
probably inline it anyway, but it seems wrong for us to rely on that.


Thanks, fixed.

Does the updated patch looks OK to you know?

libgcc/

2017-01-19  Jiong Wang  


 * config/aarch64/aarch64-unwind.h: New file.
 (DWARF_REGNUM_AARCH64_RA_STATE): Define.
 (MD_POST_EXTRACT_ROOT_ADDR): Define.
 (MD_POST_EXTRACT_FRAME_ADDR): Define.
 (MD_POST_FROB_EH_HANDLER_ADDR): Define.
 (MD_FROB_UPDATE_CONTEXT): Define.
 (aarch64_post_extract_frame_addr): New function.
 (aarch64_post_frob_eh_handler_addr): New function.
 (aarch64_frob_update_context): New function.
 * config/aarch64/linux-unwind.h: Include aarch64-unwind.h
 * config.host (aarch64*-*-elf, aarch64*-*-rtems*,
aarch64*-*-freebsd*):
 Initialize md_unwind_header to include aarch64-unwind.h.
 * unwind-dw2.c (struct _Unwind_Context): Define "RA_A_SIGNED_BIT".
 (execute_cfa_program): Multiplex DW_CFA_GNU_window_save for
__aarch64__.
 (uw_update_context): Honor MD_POST_EXTRACT_FRAME_ADDR.
 (uw_init_context_1): Honor MD_POST_EXTRACT_ROOT_ADDR.
 (uw_frob_return_addr): New function.
 (_Unwind_DebugHook): Use uw_frob_return_addr.


Since you committed this (r244673), GCC fails to build for AArch64:
/tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:
In function 'execute_cfa_program':
/tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:1193:17:
error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
function)
 fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
  ^


Hi Christophe, could you please confirm you svn revision please?

I do have done bootstrap and regression on both x86 and aarch64 before 
commit this patch.  I had forgotten to "svn add" one header file, but 
add it later.


Thanks.


Christophe

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Christophe Lyon]

Hi Jiong,

On 19 January 2017 at 15:46, Jiong Wang  wrote:
> Thanks for the review.
>
> On 19/01/17 14:18, Richard Earnshaw (lists) wrote:
>>
>>
>>>
>>>
>>> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
>>> index
>>> 8085a42ace15d53f4cb0c6681717012d906a6d47..cf640135275deb76b820f8209fa51eacfd64c4a2
>>> 100644
>>> --- a/libgcc/unwind-dw2.c
>>> +++ b/libgcc/unwind-dw2.c
>>> @@ -136,6 +136,8 @@ struct _Unwind_Context
>>>  #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
>>>/* Context which has version/args_size/by_value fields.  */
>>>  #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
>>> +  /* Bit reserved on AArch64, return address has been signed with A key.
>>> */
>>> +#define RA_A_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)
>>
>>
>> Why is this here?   It appears to only be used within the
>> AArch64-specific header file.
>
>
> I was putting it here so that when we allocate the next general purpose bit,
> we
> know clearly that bit 3 is allocated to AArch64 already, and the new general
> bit
> needs to go to the next one.  This can avoid bit collision.
>
>>
>>> ...
>>>
>>> +/* Frob exception handler's address kept in TARGET before installing
>>> into
>>> +   CURRENT context.  */
>>> +
>>> +static void *
>>> +uw_frob_return_addr (struct _Unwind_Context *current,
>>> + struct _Unwind_Context *target)
>>> +{
>>> +  void *ret_addr = __builtin_frob_return_addr (target->ra);
>>> +#ifdef MD_POST_FROB_EH_HANDLER_ADDR
>>> +  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
>>> +#endif
>>> +  return ret_addr;
>>> +}
>>> +
>>
>>
>> I think this function should be marked inline.  The optimizers would
>> probably inline it anyway, but it seems wrong for us to rely on that.
>
>
> Thanks, fixed.
>
> Does the updated patch looks OK to you know?
>
> libgcc/
>
> 2017-01-19  Jiong Wang  
>
>
> * config/aarch64/aarch64-unwind.h: New file.
> (DWARF_REGNUM_AARCH64_RA_STATE): Define.
> (MD_POST_EXTRACT_ROOT_ADDR): Define.
> (MD_POST_EXTRACT_FRAME_ADDR): Define.
> (MD_POST_FROB_EH_HANDLER_ADDR): Define.
> (MD_FROB_UPDATE_CONTEXT): Define.
> (aarch64_post_extract_frame_addr): New function.
> (aarch64_post_frob_eh_handler_addr): New function.
> (aarch64_frob_update_context): New function.
> * config/aarch64/linux-unwind.h: Include aarch64-unwind.h
> * config.host (aarch64*-*-elf, aarch64*-*-rtems*,
> aarch64*-*-freebsd*):
> Initialize md_unwind_header to include aarch64-unwind.h.
> * unwind-dw2.c (struct _Unwind_Context): Define "RA_A_SIGNED_BIT".
> (execute_cfa_program): Multiplex DW_CFA_GNU_window_save for
> __aarch64__.
> (uw_update_context): Honor MD_POST_EXTRACT_FRAME_ADDR.
> (uw_init_context_1): Honor MD_POST_EXTRACT_ROOT_ADDR.
> (uw_frob_return_addr): New function.
> (_Unwind_DebugHook): Use uw_frob_return_addr.
>

Since you committed this (r244673), GCC fails to build for AArch64:
/tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:
In function 'execute_cfa_program':
/tmp/8132498_6.tmpdir/aci-gcc-fsf/sources/gcc-fsf/gccsrc/libgcc/unwind-dw2.c:1193:17:
error: 'DWARF_REGNUM_AARCH64_RA_STATE' undeclared (first use in this
function)
fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
 ^

Christophe

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Richard Earnshaw (lists)]

On 19/01/17 14:46, Jiong Wang wrote:
> Thanks for the review.
> 
> On 19/01/17 14:18, Richard Earnshaw (lists) wrote:
>>
>>>
>>>
>>> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
>>> index
>>> 8085a42ace15d53f4cb0c6681717012d906a6d47..cf640135275deb76b820f8209fa51eacfd64c4a2
>>> 100644
>>> --- a/libgcc/unwind-dw2.c
>>> +++ b/libgcc/unwind-dw2.c
>>> @@ -136,6 +136,8 @@ struct _Unwind_Context
>>>  #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
>>>/* Context which has version/args_size/by_value fields.  */
>>>  #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
>>> +  /* Bit reserved on AArch64, return address has been signed with A
>>> key.  */
>>> +#define RA_A_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)
>>
>> Why is this here?   It appears to only be used within the
>> AArch64-specific header file.
> 
> I was putting it here so that when we allocate the next general purpose
> bit, we
> know clearly that bit 3 is allocated to AArch64 already, and the new
> general bit
> needs to go to the next one.  This can avoid bit collision.
> 

Fair enough.
>>
>>> ...
>>>
>>> +/* Frob exception handler's address kept in TARGET before installing
>>> into
>>> +   CURRENT context.  */
>>> +
>>> +static void *
>>> +uw_frob_return_addr (struct _Unwind_Context *current,
>>> + struct _Unwind_Context *target)
>>> +{
>>> +  void *ret_addr = __builtin_frob_return_addr (target->ra);
>>> +#ifdef MD_POST_FROB_EH_HANDLER_ADDR
>>> +  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
>>> +#endif
>>> +  return ret_addr;
>>> +}
>>> +
>>
>> I think this function should be marked inline.  The optimizers would
>> probably inline it anyway, but it seems wrong for us to rely on that.
> 
> Thanks, fixed.
> 
> Does the updated patch looks OK to you know?
> 
> libgcc/
> 
> 2017-01-19  Jiong Wang  
> 
> * config/aarch64/aarch64-unwind.h: New file.
> (DWARF_REGNUM_AARCH64_RA_STATE): Define.
> (MD_POST_EXTRACT_ROOT_ADDR): Define.
> (MD_POST_EXTRACT_FRAME_ADDR): Define.
> (MD_POST_FROB_EH_HANDLER_ADDR): Define.
> (MD_FROB_UPDATE_CONTEXT): Define.
> (aarch64_post_extract_frame_addr): New function.
> (aarch64_post_frob_eh_handler_addr): New function.
> (aarch64_frob_update_context): New function.
> * config/aarch64/linux-unwind.h: Include aarch64-unwind.h
> * config.host (aarch64*-*-elf, aarch64*-*-rtems*,
> aarch64*-*-freebsd*):
> Initialize md_unwind_header to include aarch64-unwind.h.
> * unwind-dw2.c (struct _Unwind_Context): Define "RA_A_SIGNED_BIT".
> (execute_cfa_program): Multiplex DW_CFA_GNU_window_save for
> __aarch64__.
> (uw_update_context): Honor MD_POST_EXTRACT_FRAME_ADDR.
> (uw_init_context_1): Honor MD_POST_EXTRACT_ROOT_ADDR.
> (uw_frob_return_addr): New function.
> (_Unwind_DebugHook): Use uw_frob_return_addr.
> 

OK.

R.

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Jiong Wang]

Thanks for the review.

On 19/01/17 14:18, Richard Earnshaw (lists) wrote:





diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
index 
8085a42ace15d53f4cb0c6681717012d906a6d47..cf640135275deb76b820f8209fa51eacfd64c4a2
 100644
--- a/libgcc/unwind-dw2.c
+++ b/libgcc/unwind-dw2.c
@@ -136,6 +136,8 @@ struct _Unwind_Context
 #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
   /* Context which has version/args_size/by_value fields.  */
 #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
+  /* Bit reserved on AArch64, return address has been signed with A key.  */
+#define RA_A_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)


Why is this here?   It appears to only be used within the
AArch64-specific header file.


I was putting it here so that when we allocate the next general purpose bit, we
know clearly that bit 3 is allocated to AArch64 already, and the new general bit
needs to go to the next one.  This can avoid bit collision.




...

+/* Frob exception handler's address kept in TARGET before installing into
+   CURRENT context.  */
+
+static void *
+uw_frob_return_addr (struct _Unwind_Context *current,
+ struct _Unwind_Context *target)
+{
+  void *ret_addr = __builtin_frob_return_addr (target->ra);
+#ifdef MD_POST_FROB_EH_HANDLER_ADDR
+  ret_addr = MD_POST_FROB_EH_HANDLER_ADDR (current, target, ret_addr);
+#endif
+  return ret_addr;
+}
+


I think this function should be marked inline.  The optimizers would
probably inline it anyway, but it seems wrong for us to rely on that.


Thanks, fixed.

Does the updated patch looks OK to you know?

libgcc/

2017-01-19  Jiong Wang  

* config/aarch64/aarch64-unwind.h: New file.
(DWARF_REGNUM_AARCH64_RA_STATE): Define.
(MD_POST_EXTRACT_ROOT_ADDR): Define.
(MD_POST_EXTRACT_FRAME_ADDR): Define.
(MD_POST_FROB_EH_HANDLER_ADDR): Define.
(MD_FROB_UPDATE_CONTEXT): Define.
(aarch64_post_extract_frame_addr): New function.
(aarch64_post_frob_eh_handler_addr): New function.
(aarch64_frob_update_context): New function.
* config/aarch64/linux-unwind.h: Include aarch64-unwind.h
* config.host (aarch64*-*-elf, aarch64*-*-rtems*, aarch64*-*-freebsd*):
Initialize md_unwind_header to include aarch64-unwind.h.
* unwind-dw2.c (struct _Unwind_Context): Define "RA_A_SIGNED_BIT".
(execute_cfa_program): Multiplex DW_CFA_GNU_window_save for __aarch64__.
(uw_update_context): Honor MD_POST_EXTRACT_FRAME_ADDR.
(uw_init_context_1): Honor MD_POST_EXTRACT_ROOT_ADDR.
(uw_frob_return_addr): New function.
(_Unwind_DebugHook): Use uw_frob_return_addr.

diff --git a/libgcc/config.host b/libgcc/config.host
index 6f2e458e74e776a6b7a310919558bcca76389232..540bfa9635802adabb36a2d1b7cf3416462c59f3 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -331,11 +331,13 @@ aarch64*-*-elf | aarch64*-*-rtems*)
 	extra_parts="$extra_parts crtfastmath.o"
 	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
 	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
+	md_unwind_header=aarch64/aarch64-unwind.h
 	;;
 aarch64*-*-freebsd*)
 	extra_parts="$extra_parts crtfastmath.o"
 	tmake_file="${tmake_file} ${cpu_type}/t-aarch64"
 	tmake_file="${tmake_file} ${cpu_type}/t-softfp t-softfp t-crtfm"
+	md_unwind_header=aarch64/aarch64-unwind.h
 	;;
 aarch64*-*-linux*)
 	extra_parts="$extra_parts crtfastmath.o"
diff --git a/libgcc/config/aarch64/aarch64-unwind.h b/libgcc/config/aarch64/aarch64-unwind.h
new file mode 100644
index ..a43d965b358f3e830b85fc42c7bceacf7d41a671
--- /dev/null
+++ b/libgcc/config/aarch64/aarch64-unwind.h
@@ -0,0 +1,87 @@
+/* Copyright (C) 2017 Free Software Foundation, Inc.
+   Contributed by ARM Ltd.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+for more details.
+
+Under Section 7 of GPL version 3, you are granted additional
+permissions described in the GCC Runtime Library Exception, version
+3.1, as published by the Free Software Foundation.
+
+You should have received a copy of the GNU General Public License and
+a copy of the GCC Runtime Library Exception along with this program;
+see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+.  */
+
+#ifndef AARCH64_UNWIND_H
+#define AARCH64_UNWIND_H
+
+#define DWARF_REGNUM_AARCH64_RA_STATE 34
+
+#define MD_POST_EXTRACT_ROOT_ADDR(addr)  __builtin_aarch64_xpaclri (addr)
+#define MD_POST_EXTRACT_FRAME_ADDR(context, fs, addr) \
+  

Re: [Ping~]Re: [5/5][libgcc] Runtime support for AArch64 return address signing (needs new target macros)

[Richard Earnshaw (lists)]

On 18/01/17 17:07, Jiong Wang wrote:
> On 12/01/17 18:10, Jiong Wang wrote:
>> On 06/01/17 11:47, Jiong Wang wrote:
>>> This is the update on libgcc unwinder support according to new DWARF
>>> proposal.
>>>
>>> As Joseph commented, duplication of unwind-dw2.c is not encouraged in
>>> libgcc,
>>> But from this patch, you can see there are a few places we need to
>>> modify for
>>> AArch64 in unwind-aarch64.c, so the file duplication approach is
>>> acceptable?
>>>
>>>
>>> libgcc/
>>>
>>> 2017-01-06  Jiong Wang  
>>>
>>> * config/aarch64/unwind-aarch64.c
>>> (DWARF_REGNUM_AARCH64_RA_STATE,
>>> RA_A_SIGNED_BIT): New macros.
>>> (execute_cfa_program): Multiplex DW_CFA_GNU_window_save on
>>> AArch64.
>>> (uw_frame_state_for): Clear bit[0] of
>>> DWARF_REGNUM_AARCH64_RA_STATE.
>>> (uw_update_context): Authenticate return address according to
>>> DWARF_REGNUM_AARCH64_RA_STATE.
>>> (uw_init_context_1): Strip signature of seed address.
>>> (uw_install_context): Re-authenticate EH handler's address.
>>>
>> Ping~
>>
>> For comparision, I have also attached the patch using the target macros.
>>
>> Four new target macros are introduced:
>>
>>   MD_POST_EXTRACT_ROOT_ADDR
>>   MD_POST_EXTRACT_FRAME_ADDR
>>   MD_POST_FROB_EH_HANDLER_ADDR
>>   MD_POST_INIT_CONTEXT
>>
>> MD_POST_EXTRACT_ROOT_ADDR is to do target private post processing on
>> the address
>> inside _Unwind* functions, they are serving as root address to start the
>> unwinding.  MD_POST_EXTRACT_FRAME_ADDR is to do target private post
>> processing
>> on th address inside the real user program which throws the exceptions.
>>
>> MD_POST_FROB_EH_HANDLER_ADDR is to do target private frob on the EH
>> handler's
>> address before we install it into current context.
>>
>> MD_POST_INIT_CONTEXT it to do target private initialization on the
>> context
>> structure after common initialization.
>>
>> One "__aarch64__" macro check is needed to multiplex DW_CFA_window_save.
> 
> Ping ~
> 
> Could global reviewers or libgcc maintainers please give a review on the
> generic
> part change?
> 
> One small change is I removed MD_POST_INIT_CONTEXT as I found there is
> MD_FROB_UPDATE_CONTEXT which serve the same purpose.  I still need to
> define
> 
>MD_POST_EXTRACT_ROOT_ADDR
>MD_POST_EXTRACT_FRAME_ADDR
>MD_POST_FROB_EH_HANDLER_ADDR
> 
> And do one __aarch64__ check to multiplexing DW_CFA_GNU_window_save.
> 
> Thanks.
> 
> libgcc/ChangeLog:
> 
> 2017-01-18  Jiong Wang  
> 
> * config/aarch64/aarch64-unwind.h: New file.
> (DWARF_REGNUM_AARCH64_RA_STATE): Define.
> (MD_POST_EXTRACT_ROOT_ADDR): Define.
> (MD_POST_EXTRACT_FRAME_ADDR): Define.
> (MD_POST_FROB_EH_HANDLER_ADDR): Define.
> (MD_FROB_UPDATE_CONTEXT): Define.
> (aarch64_post_extract_frame_addr): New function.
> (aarch64_post_frob_eh_handler_addr): New function.
> (aarch64_frob_update_context): New function.
> * config/aarch64/linux-unwind.h: Include aarch64-unwind.h
> * config.host (aarch64*-*-elf, aarch64*-*-rtems*,
> aarch64*-*-freebsd*):
> Initialize md_unwind_header to include aarch64-unwind.h.
> * unwind-dw2.c (struct _Unwind_Context): Define "RA_A_SIGNED_BIT".
> (execute_cfa_program): Multiplex DW_CFA_GNU_window_save for
> __aarch64__.
> (uw_update_context): Honor MD_POST_EXTRACT_FRAME_ADDR.
> (uw_init_context_1): Honor MD_POST_EXTRACT_ROOT_ADDR.
> (uw_frob_return_addr): New function.
> (_Unwind_DebugHook): Use uw_frob_return_addr.
> 
> 

Comments inline.

> 1.patch
> 
> 
> diff --git a/libgcc/unwind-dw2.c b/libgcc/unwind-dw2.c
> index 
> 8085a42ace15d53f4cb0c6681717012d906a6d47..cf640135275deb76b820f8209fa51eacfd64c4a2
>  100644
> --- a/libgcc/unwind-dw2.c
> +++ b/libgcc/unwind-dw2.c
> @@ -136,6 +136,8 @@ struct _Unwind_Context
>  #define SIGNAL_FRAME_BIT ((~(_Unwind_Word) 0 >> 1) + 1)
>/* Context which has version/args_size/by_value fields.  */
>  #define EXTENDED_CONTEXT_BIT ((~(_Unwind_Word) 0 >> 2) + 1)
> +  /* Bit reserved on AArch64, return address has been signed with A key.  */
> +#define RA_A_SIGNED_BIT ((~(_Unwind_Word) 0 >> 3) + 1)

Why is this here?   It appears to only be used within the
AArch64-specific header file.

>_Unwind_Word flags;
>/* 0 for now, can be increased when further fields are added to
>   struct _Unwind_Context.  */
> @@ -1185,6 +1187,11 @@ execute_cfa_program (const unsigned char *insn_ptr,
> break;
>  
>   case DW_CFA_GNU_window_save:
> +#ifdef __aarch64__
> +   /* This CFA is multiplexed with Sparc.  On AArch64 it's used to toggle
> +  return address signing status.  */
> +   fs->regs.reg[DWARF_REGNUM_AARCH64_RA_STATE].loc.offset ^= 1;
> +#else
> /* ??? Hardcoded for SPARC register window configuration.  */
> if (__LIBGCC_DWARF_FRAME_REGISTERS__ >= 32)
>   for