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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: ELF/Arch/AArch64.cpp:537
+void AArch64BtiPac::writePltHeader(uint8_t *Buf) const {
+  if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
+    writePltHeaderBti(Buf);
----------------
Should the condition be ANDed with `!Config->Shared`? Currently with `--force-bti` a shared object may 


================
Comment at: ELF/Arch/AArch64.cpp:543
+
+void AArch64BtiPac::writePltBti(uint8_t *Buf, uint64_t GotPltEntryAddr,
+                                uint64_t PltEntryAddr, int32_t Index,
----------------
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)
```


================
Comment at: ELF/Arch/AArch64.cpp:568
+      0x10, 0x02, 0x00, 0x91, // add  x16, x16, Offset(&(.plt.got[n]))
+      0x9f, 0x21, 0x03, 0xd5, // autiax16x17
+      0x20, 0x02, 0x1f, 0xd6, // br   x17
----------------
Is this `autia1716`?


================
Comment at: ELF/Arch/AArch64.cpp:587
+      0x10, 0x02, 0x00, 0x91, // add  x16, x16, Offset(&(.plt.got[n]))
+      0x9f, 0x21, 0x03, 0xd5, // autiax16x17
+      0x20, 0x02, 0x1f, 0xd6  // br   x17
----------------
Is this `autia1716`?


================
Comment at: ELF/Arch/AArch64.cpp:617
+
+static TargetInfo *getTargetInfo() {
+  if (Config->AndFeatures & (GNU_PROPERTY_AARCH64_FEATURE_1_BTI |
----------------
`Config->AndFeatures` hasn't been computed when `getTargetInfo` is called.

```
Target = getTarget();
...
link<ELF64LE>(Args);  // Config->AndFeatures is computed inside this function
```



================
Comment at: ELF/Driver.cpp:1624
+  if (Config->EMachine == EM_AARCH64 && Config->PacPlt)
+      Ret |= GNU_PROPERTY_AARCH64_FEATURE_1_PAC;
+
----------------
not 2-space indented.

I have a question: does `DT_AARCH64_BTI_PLT` make sense for a shared object? If not, you may apply the following here:

```
if (!Config->Shared)
  Ret &= ~GNU_PROPERTY_AARCH64_FEATURE_1_BTI;
```

If yes, in `getTargetInfo()`, you may want to check `!Config->Shared` beside `GNU_PROPERTY_AARCH64_FEATURE_1_BTI`.


================
Comment at: ELF/SyntheticSections.cpp:1348
+    if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
+      addInt(DT_AARCH64_BTI_PLT, 0);
+    if (Config->AndFeatures & GNU_PROPERTY_AARCH64_FEATURE_1_PAC)
----------------
The ABI chooeses two `DT_*` tags (and with a value of 0! not 1) instead of a single `DT_*` with 2 bits seems a bit wasteful...


================
Comment at: test/ELF/aarch64-feature-bti.s:8
+
+# We do not add BTI support when the inputs don't have the .note.gnu.property field.
+
----------------
In binary utility tests, people seem to start using `## ` to highlight comments from RUN/CHECK lines..


================
Comment at: test/ELF/aarch64-feature-bti.s:11
+# RUN: ld.lld %tno.o %t3.o --shared -o %tno.so
+# RUN: llvm-objdump -d -mattr=+bti %tno.so | FileCheck --check-prefix=NOBTI %s
+# RUN: llvm-readelf -x .got.plt %tno.so | FileCheck --check-prefix SOGOTPLT %s
----------------
If the hex bytes in the dissassembly output are not important. consider `--no-show-raw-insn`


================
Comment at: test/ELF/aarch64-feature-bti.s:38
+# Expect a bti c at the start of plt[0], the plt entries do not need bti c as
+# their address doesn't escape, so they can't be indirectly called. Expect no
+# other difference.
----------------
`doesn't escape` -> `doesn't escape a shared object`


================
Comment at: test/ELF/aarch64-feature-bti.s:76
+
+# Build an executable with all relocatable inputs having the BTI .note.property
+# We expect a bti c in front of all PLT entries as the address of a PLT entry
----------------
`.note.property` -> `.note.gnu.property`


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

https://reviews.llvm.org/D62609