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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/Arch/AArch64.cpp:485-486
+
+const uint8_t AArch64BtiPac::NopData[] = { 0x1f, 0x20, 0x03, 0xd5 }; // nop
+const uint8_t AArch64BtiPac::BtiData[] = { 0x5f, 0x24, 0x03, 0xd5 }; // bti c
+
----------------
They are used only twice. We inline all other instructions. Maybe we should inline them and remove these varaibles?


================
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
----------------
I'm not sure whether it is intentional or a bug to not use `BtiNeeded`. If it's intentional, can you add a comment?


================
Comment at: ELF/Arch/AArch64.cpp:515
+    memcpy(Buf, BtiData, sizeof(BtiData));
+    Pre = sizeof(BtiData);
+  }
----------------
Looks like you can add sizeof(BtiData) to Buf and remove Pre.


================
Comment at: ELF/Arch/AArch64.cpp:566
+    memcpy(Buf + Pre + sizeof(AddrInst), StdBr, sizeof(StdBr));
+  if (Pre == 0)
+    // We didn't add the BTI c instruction so round out size with NOP.
----------------
Ditto -- do you need `Pre`? `Pre` is 0 only when `!BtiNeeded`, so you can use that condition instead.


================
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;
----------------
It looks a bit odd that a `--force` option only warns and is not a hard error. Is this intentional?


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

https://reviews.llvm.org/D62609