[PATCH] D158574: [AArch64][ELF][llvm-readobj] Support ELF AUTH constants

[Daniil Kovalev via Phabricator via llvm-commits]

kovdan01 added a comment.

TODO:

- yaml2obj is for some reason broken when using AARCH64_DYNAMIC_TAG instead of DYNAMIC_TAG
- more tests for new relocation types



================
Comment at: lld/ELF/Config.h:435
+
+  SmallVector<uint8_t, 0> aarch64PauthAbiTag;
+  SmallVector<uint8_t, 0> gnuPropAarch64Pauth;
----------------
MaskRay wrote:
> Variables unrelated to command-line options are generally placed in `Ctx`.
Done, thanks. I suppose we also might want to move `andFeatures` to Ctx - I initially placed this new field here because `andFeatures` was here. I'll submit a small subsequent patch regarding that.


================
Comment at: lld/ELF/Driver.cpp:2600
+  config->gnuPropAarch64Pauth = ctx.objectFiles.front()->gnuPropAarch64Pauth;
+  for (ELFFileBase *f : ctx.objectFiles) {
+    StringRef f1 = ctx.objectFiles.front()->getName();
----------------
MaskRay wrote:
> move f1 outside (prefer `[0].` to `front()->`)  and use `ArrayRef(ctx.objectFiles).slice(1)`
Regarding `[0].` instead of `front()->` - not relevant anymore after '-z pauth-report' implementation. We now use the first non-empty pauth abi tag as a reference instead of the first one.

Regarding `f11 - done, thanks.


================
Comment at: lld/ELF/Driver.cpp:2605
+                           const SmallVector<uint8_t, 0> &d2) {
+      if (d1.empty() ^ d2.empty())
+        fatal((d1.empty() ? f1 : f2) +
----------------
MaskRay wrote:
> `^` => `!=`
Changed, thanks! I don't know why I used such a strange way of comparing booleans here :)


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

Done, thanks

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

I'm not sure what you meant here. Please let me know if my changes are not what you expected.


================
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.
----------------
MaskRay wrote:
> I am not sure the cast is a bad idea. This is the idiomatic way. `Elf_Nhdr` recognizes endianness and has packed data members. 
Yes, you a right, removed the comment. Thanks!


================
Comment at: lld/ELF/SyntheticSections.cpp:375
+
+Aarch64PauthAbiTag::Aarch64PauthAbiTag()
+    : SyntheticSection(llvm::ELF::SHF_ALLOC, llvm::ELF::SHT_NOTE,
----------------
MaskRay wrote:
> The section name convention is `AArch64*` instead of `Aarch64*`
Changed, thanks


================
Comment at: lld/ELF/SyntheticSections.cpp:384
+void Aarch64PauthAbiTag::writeTo(uint8_t *buf) {
+  assert(isNeeded());
+  const SmallVector<uint8_t, 0> &data = config->aarch64PauthAbiTag;
----------------
MaskRay wrote:
> unneeded
Done, thanks


================
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
----------------
MaskRay wrote:
> Since we `cd %t`, all the `%t` prefix of temporary object files can be omitted. We ensure all temporary files are in `%t` now.
Ah, forgot to change that. Fixed, thanks


================
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
----------------
MaskRay wrote:
> cp gnu11.o gnu12.o
Definitely better, fixed


================
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
----------------
MaskRay wrote:
> a non-shared object can be named `.so`, but it's confusing. Suggest `-shared`
Fixed, thanks


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5361
+template <class ELFT>
+static bool printARMNote(raw_ostream &OS, uint32_t NoteType,
+                         ArrayRef<uint8_t> Desc) {
----------------
peter.smith wrote:
> I recommend printAArch64Note as ARM in all caps tends to refer to the ARM (AArch32 backend). Even if the vendor/owner name is ARM the company.
OK, it makes sense. Changed ARM->AArch64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158574