[PATCH] D62609: [LLD][ELF][AArch64] Support for BTI and PAC

[Peter Smith via Phabricator via llvm-commits]

peter.smith marked 3 inline comments as done.
peter.smith added a comment.

In D62609#1528520 <https://reviews.llvm.org/D62609#1528520>, @ruiu wrote:

> Peter, do you want me to separate code to read AndFeatures from https://reviews.llvm.org/D59780 and submit?


Yes please, that would decouple this patch from the CET specific work.



================
Comment at: ELF/Arch/AArch64.cpp:543
+
+void AArch64BtiPac::writePltBti(uint8_t *Buf, uint64_t GotPltEntryAddr,
+                                uint64_t PltEntryAddr, int32_t Index,
----------------
MaskRay wrote:
> peter.smith wrote:
> > MaskRay wrote:
> > > Have you considered the alternative: rather than define 4 writePlt*, define a unified one:
> > > 
> > > ```
> > > if Bti
> > >   write bti c
> > > write adrp/ldr/add
> > > if Pac
> > >   write autiax16x17
> > > write br x17
> > > write padding nop(s)
> > > ```
> > I did try that at first, the disadvantage, particularly for the BTI is that the offsets for the relocations change and it gets a bit messy. Not overly so, but more difficult to follow on first reading. I'm happy to do that if you'd prefer?
> I do prefer a unified PLT as because this is a big chunk of repetition (4x). While reading the code, I couldn't help comparing the 4 functions just to find the difference is bti/autia1716. Putting them in one place may help.
> 
> But I'd like to wait for others to comment on this.
I'll send an updated patch with a single unified PLT. If it is too complicated then we can always go back to the previous version.


================
Comment at: ELF/Driver.cpp:1607
 // This function returns the merged feature flags. If 0, we cannot enable CET.
-template <class ELFT> static uint32_t getX86Features() {
-  if (Config->EMachine != EM_386 && Config->EMachine != EM_X86_64)
+// This is also the case with AARCH64's BTI and PAC which use the a similar
+// GNU_PROPERTY_AARCH64_FEATURE_1_AND mechanism.
----------------
MaskRay wrote:
> `the a`, typo?
Yes, thanks for spotting.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62609/new/

https://reviews.llvm.org/D62609