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

[Daniil Kovalev via Phabricator via llvm-commits]

kovdan01 added inline comments.


================
Comment at: lld/ELF/Config.h:151
   uint32_t andFeatures = 0;
+  SmallVector<uint8_t, 16> aarch64PauthAbiTag;
+  SmallVector<uint8_t, 16> gnuPropAarch64Pauth;
----------------
MaskRay wrote:
> They make the offsets of following variables larger. move to the end.
> `<xxx, 0>` is more common in lld. The inline element doesn't save.
Got it, fixed


================
Comment at: lld/ELF/Driver.cpp:2595
+static void getAarch64PauthInfo() {
+  if (config->emachine != EM_AARCH64)
+    return;
----------------
MaskRay wrote:
> move the check to the caller
Done


================
Comment at: lld/ELF/Driver.cpp:2612
+              (d1.empty() ? f2 : f1) +
+              " has one. Either all or no link units must have it.");
+
----------------
MaskRay wrote:
> No trailing period. Please check the existing convention.
Fixed, also changed "one. Either" to "one; either" - multiple sentences IMHO don't look nice in such context


================
Comment at: lld/ELF/InputFiles.cpp:964
+static void parseAarch64PauthAbiTag(const InputSection &sec, ObjFile<ELFT> &f) {
+  if (config->emachine != EM_AARCH64)
+    return;
----------------
MaskRay wrote:
> move the check to the caller
Done


================
Comment at: lld/ELF/SyntheticSections.cpp:362
+
+  return 16 + std::accumulate(props.begin(), props.end(), 0,
+                              [](uint32_t sum, const Elf_Prop &e) {
----------------
MaskRay wrote:
> this needs `<numeric>`
Fixed, thanks! It looks like this file is included implicitly from other files on my machine, so didn't mention it by myself.


================
Comment at: lld/test/ELF/aarch64-feature-pauth.s:3
+
+# RUN: split-file %s %tsplit
+
----------------
MaskRay wrote:
> `%tsplit` makes temporary output filenames unnecessarily long. Just use `RUN: rm -rf %t && split-file %s %t && cd %t`
> 
> please check the existing convention.
Fixed, thanks, it definitely enhances readability


================
Comment at: lld/test/ELF/aarch64-feature-pauth.s:59
+
+# ERR6: ld.lld: error: {{.*}} has no aarch64 pauth compatibility info while {{.*}} has one. Either all or no link units must have it.
+
----------------
MaskRay wrote:
> We omit messages before `error:` as the executable may have a different name, e.g. `ld.lld.exe`. Please follow the style of some recently added tests.
Got it, fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158574