[PATCH] D158574: [AArch64][ELF] Support PAUTH ABI marking

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

I am puzzled by the need of both the generic `.note.AARCH64-PAUTH-ABI-tag` and the GNU-centric `.note.gnu.property`.
If `.note.AARCH64-PAUTH-ABI-tag` is preferred, you can push forward `.note.AARCH64-PAUTH-ABI-tag` adoption and not bother with `.note.gnu.property`.
Why should the linker bear complexity related to `.note.gnu.property` and its interaction with `.note.AARCH64-PAUTH-ABI-tag` (error, but we could avoid this complexity but not support `.note.gnu.property` in the first place).

If there is not so such GNU hatred, I'd like that we only implement `.note.gnu.property` and only implement `.note.AARCH64-PAUTH-ABI-tag` when it is really needed, i.e. some ELF OSes using PAuth somehow uses `.note.AARCH64-PAUTH-ABI-tag`.
I'm not certain certain ELF OSes are present. The ABI can be written in a more natural way to favor `.note.AARCH64-PAUTH-ABI-tag` .



================
Comment at: lld/ELF/Config.h:435
+
+  SmallVector<uint8_t, 0> aarch64PauthAbiTag;
+  SmallVector<uint8_t, 0> gnuPropAarch64Pauth;
----------------
Variables unrelated to command-line options are generally placed in `Ctx`.


================
Comment at: lld/ELF/Driver.cpp:2600
+  config->gnuPropAarch64Pauth = ctx.objectFiles.front()->gnuPropAarch64Pauth;
+  for (ELFFileBase *f : ctx.objectFiles) {
+    StringRef f1 = ctx.objectFiles.front()->getName();
----------------
move f1 outside (prefer `[0].` to `front()->`)  and use `ArrayRef(ctx.objectFiles).slice(1)`


================
Comment at: lld/ELF/Driver.cpp:2605
+                           const SmallVector<uint8_t, 0> &d2) {
+      if (d1.empty() ^ d2.empty())
+        fatal((d1.empty() ? f1 : f2) +
----------------
`^` => `!=`


================
Comment at: lld/ELF/InputFiles.cpp:968
+  auto reportFatal = [&](const Twine &msg) {
+    fatal(toString(sec.file) + ":(" + sec.name + "): " + msg);
+  };
----------------
It's conventional to call `errorOrWarn` and continue parsing instead of calling `fatal`

`reportFatal` can be changed to return `bool` indicating whether the caller should early return.


================
Comment at: lld/ELF/InputFiles.cpp:974
+  // example, parseGnuProperty above), so use it here as a temporary solution.
+  // TODO: here and in other similar places read the ELF note header data
+  // properly independently of host's endianness.
----------------
I am not sure the cast is a bad idea. This is the idiomatic way. `Elf_Nhdr` recognizes endianness and has packed data members. 


================
Comment at: lld/test/ELF/aarch64-feature-pauth.s:5
+
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu gnu-note1.s -o %tgnu11.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu gnu-note1.s -o %tgnu12.o
----------------
Since we `cd %t`, all the `%t` prefix of temporary object files can be omitted. We ensure all temporary files are in `%t` now.


================
Comment at: lld/test/ELF/aarch64-feature-pauth.s:6
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu gnu-note1.s -o %tgnu11.o
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu gnu-note1.s -o %tgnu12.o
+# RUN: ld.lld %tgnu11.o %tgnu12.o -o %tgnuok.so
----------------
cp gnu11.o gnu12.o


================
Comment at: lld/test/ELF/aarch64-feature-pauth.s:7
+# RUN: llvm-mc -filetype=obj -triple=aarch64-linux-gnu gnu-note1.s -o %tgnu12.o
+# RUN: ld.lld %tgnu11.o %tgnu12.o -o %tgnuok.so
+# RUN: llvm-readelf -n %tgnuok.so | FileCheck --check-prefix OK1 %s
----------------
a non-shared object can be named `.so`, but it's confusing. Suggest `-shared`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158574