Re: [PATCH][BINUTILS][AARCH64] Add support for pointer authentication B key

[Sam Tebbs <Sam dot Tebbs at arm dot com>]

On 11/09/2018 01:27 PM, Nick Clifton wrote:

> Hi Sam,

Hi Nick, thanks for the feedback.

>> The DWARF extensions for ARM therefore
>> propose to add a new augmentation character 'B' to the CIE augmentation string
>> and the corresponding cfi directive ".cfi_b_key_frame".
> Are these extensions documented somewhere ?  And is the documentation referenced
> in the source code so that readers can find the description ?
>

There is no public documentation for this approach as it was agreed upon 
internally with those implementing support in LLVM. I have documented 
this in gas/doc/c-aarch64.texi, would you like me to refer to that 
section from the relevant source code?

>> The pointer authentication instructions will behave as NOPs on architectures
>> that don't support them, and so a check for the architecture being assembled
>> for is not necessary since there will be no behavioural difference between
>> augmentation strings with and without the 'B' character on such architectures.
> Except that if the new .cfi_b_key_frame pseudo-op is used on an architecture
> that does not support it, the programmer might expect to see a warning message,
> yes ?  (Although reading the patch, it looks like the new pseudo-op is only
> defined for the AArch64 so other architectures cannot even use it).
>

Yes the directive will only be available when assembling for AArch64 so 
it won't be usable from other architectures.

>> Built on aarch64-linux-gnu and aarch64-none-elf, and tested on aarch64-none-elf
>> with no regressions.
> Did you also test on an non AArch64 configuration, just to be sure,  eg x86_64-pc-linux-gnu ?

I haven't actually, but will do so and report back. Would just this 
other configuration suffice?

> It would be nice if you also updated binutils/dwarf.c:read_cie() so that it explicitly
> handles the 'A' and 'B' encodings, even if there is nothing for it to do.  Just so
> that readers know that the encodings are recognised.  (And maybe unrecognised encodings
> should be reported....)

I agree, it would at least be good to insert a check there. I could 
always add a warning when encountering unrecognised encodings in a 
future patch.

> Some comments on the patch itself:
>
>> diff --git a/bfd/elf-eh-frame.c b/bfd/elf-eh-frame.c
>>   		switch (*aug++)
>>   		  {
>> +		  case 'B':
>> +		    break;
> I would feel much happier if these encoding characters were defined once in
> a header somewhere and then #included wherever they were needed.  It is not
> necessary to make this change for this patch submission, but it would be a
> nice thing to have, if not now, then in the future.

Adding a definition of the encoding character to a header could perhaps 
coincide with your suggestions below, in that I could add a macro called 
"tc_is_valid_aug_char" which the target defines. That would mean we 
could avoid having such an encoding definition in a target-agnostic 
file. Let me know what you think.

> > diff --git a/gas/dw2gencfi.h b/gas/dw2gencfi.h
>
>> +enum pointer_auth_key {
>> +  AARCH64_PAUTH_KEY_A,
>> +  AARCH64_PAUTH_KEY_B
>> +};
> I do not like having target specific enums defined in a generic
> header file.  I would prefer to see definitions like this in
> gas/config/tc-aarch64.h.
>

Using a target macro as per your suggestions below would allow this 
definition to be in tc-aarch64.h rather than in dw2gencfi.h.

>> @@ -162,6 +183,8 @@ struct fde_entry
>>     /* For out of line tables and FDEs.  */
>>     symbolS *eh_loc;
>>     int sections;
>> +  /* The pointer authentication key used.  Only used for AArch64.  */
>> +  enum pointer_auth_key pauth_key;
>>   };
> Similarly, I think that this extra field should target defined.
> Ie something like:
>
>      int sections;
>    #ifdef tc_fde_entry_extras
>      tc_fde_entry_extras
>    #endif
>    };
>
> And then define tc_fde_entry_extras in tc-aarch64.h.  This would
> also allow other backends to extend the fde_entry structure if they
> even find a need to do something similar.
>
>> diff --git a/gas/dw2gencfi.c b/gas/dw2gencfi.c
>> @@ -403,6 +403,7 @@ struct cie_entry
>>     unsigned char per_encoding;
>>     unsigned char lsda_encoding;
>>     expressionS personality;
>> +  enum pointer_auth_key pauth_key;
>>     struct cfi_insn_data *first, *last;
>>   };
> A similar comment applies here.
>
>> @@ -448,6 +433,7 @@ alloc_fde_entry (void)
>>     fde->per_encoding = DW_EH_PE_omit;
>>     fde->lsda_encoding = DW_EH_PE_omit;
>>     fde->eh_header_type = EH_COMPACT_UNKNOWN;
>> +  fde->pauth_key = AARCH64_PAUTH_KEY_A;
>>   
>>     return fde;
>>   }
> And of course this would have to be:
>
>       fde->eh_header_type = EH_COMPACT_UNKNOWN;
>    #ifdef tc_fde_entry_init
>       tc_fde_entry_init (fde)
>    #endif

I agree with your suggestions here, I'll get to work on them and update 
with a reworked patch. In the mean time please do let me know if you 
have any feedback on my comments here.

Sam