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

[Peter Smith via Phabricator via llvm-commits]

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

Thanks for the comments I'll upload a new patch shortly.



================
Comment at: ELF/Arch/AArch64.cpp:511
+  size_t Pre = 0;
+  if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_BTI) {
+    // PltHeader is called indirectly by Plt[N]. Prefix PltData with a BTI C
----------------
ruiu wrote:
> I'm not sure whether it is intentional or a bug to not use `BtiNeeded`. If it's intentional, can you add a comment?
I've split out BtiNeeded into BtiHeader and BtiEntry to make the intention explicit. In summary:
- BtiHeader is always needed if the FEATURE_1_BTI bit is set.
- BtiEntry is needed for non-shared libraries if the FEATURE_1_BTI bit is set.
- PacEntry is always needed if the FEATURE_1_PAC bit is set.

There is no PacHeader as we don't need to alter the header file.


================
Comment at: ELF/Arch/AArch64.cpp:515
+    memcpy(Buf, BtiData, sizeof(BtiData));
+    Pre = sizeof(BtiData);
+  }
----------------
ruiu wrote:
> Looks like you can add sizeof(BtiData) to Buf and remove Pre.
I've updated the patch to do that. Pre also needs to be added to the Plt address so the getAArchPage(Plt + 8 + Pre) gets the right value.


================
Comment at: ELF/Driver.cpp:1621
+    if (Config->ForceBTI && !(Features & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) {
+      warn(toString(F) + ": --force-bti: file does not have BTI property");
+      Features |= GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
----------------
ruiu wrote:
> It looks a bit odd that a `--force` option only warns and is not a hard error. Is this intentional?
Yes it is intentional (behaves the same way in ld.bfd). It is intended for early adopters/experimenters with a lot of assembler files or even binary objects that do not have the appropriate .note.gnu.property, yet they are able to declare them compatible with BTI (not many functions in assembly are called indirectly). For example we have some people experimenting with bringing up Android with BTI enabled and they preferred a warning so that they could get it up and running without having to track each last assembler file down. I think over time, when the hardware is closer and more projects have decided to use BTI an option like --require-bti could be added that would behave like --require-cet.



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

https://reviews.llvm.org/D62609