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

[Christophe Lyon <christophe dot lyon at linaro dot org>]

Hi Jiong,

On 19 January 2017 at 15:46, Jiong Wang <jiong.wang@foss.arm.com> 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  <jiong.wang@arm.com>
>
>
>         * 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