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

[Nick Clifton]

Hi Sam,

> 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 ?


> 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).


> 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 ?

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....)


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.

 > 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.


> @@ -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

And so on for the other changes in dw2gencfi.c.  I realise that this
is a pain to do, but I would like to keep generic code generic, and
make it clear where target specific overrides are happening.


Cheers
  Nick