[PATCH] D152921: [lld] Synthesize metadata for MTE globals

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/AArch64.cpp:390
+         rel.addend >= static_cast<int64_t>(rel.sym->getSize())))
+      write64(loc, -1 * rel.addend);
+    else
----------------
Just use the unary operator.


================
Comment at: lld/ELF/Driver.cpp:2588
+
+  auto f = [&](auto relArray) {
+    for (const typename ELFT::Rela &rel : relArray) {
----------------
the lambda is only used once. just expand it.


================
Comment at: lld/ELF/Driver.cpp:2595
+      // STT_NOTYPE. In addition, this save us from checking untaggable symbols,
+      // like functions or TLS blocks.
+      if (sym.type != STT_OBJECT)
----------------
TLS blocks => TLS symbols


================
Comment at: lld/ELF/Driver.cpp:2604
+      }
+      referenceCount[&sym]++;
+    }
----------------
LLVM coding style prefers pre-increments


================
Comment at: lld/ELF/Driver.cpp:2959
+  // demote the symbol to be untagged.
+  if (config->emachine == EM_AARCH64 &&
+      config->androidMemtagMode != ELF::NT_MEMTAG_LEVEL_NONE) {
----------------
We probably should move the whole function and referenced function template `addTaggedSymbolReferences` to AArch64.cpp.


================
Comment at: lld/ELF/Driver.cpp:2967
+    for (InputFile* file : files) {
+      // TODO(hctim): Add bitcode (LTO) support.
+      if (file->kind() != InputFile::BinaryKind &&
----------------
Avoid referencing an individual user. When needed (actually very rarely done for lld), reference a public issue link.

In this case, bitcode files should already work. This place is after `compileBitcodeFiles`, so `ctx.objectFiles contains all relocatable object files (including extracted lazy object files) and compiled bitcode files.


================
Comment at: lld/ELF/SyntheticSections.cpp:1456
       addInt(DT_AARCH64_MEMTAG_STACK, config->androidMemtagStack);
+      if (mainPart->memtagDescriptors && mainPart->memtagDescriptors->getSize()) {
+        addInSec(DT_AARCH64_MEMTAG_GLOBALS, *mainPart->memtagDescriptors);
----------------
This is guaranteed to be non-null.


================
Comment at: lld/ELF/SyntheticSections.cpp:3926
+      continue;
+    const uint64_t vAddr = sym->getVA();
+    const uint64_t size = sym->getSize();
----------------
We typically just use `addr` instead of `vAddr`.


================
Comment at: lld/ELF/SyntheticSections.cpp:3937
+    if (size == 0)
+      error("size of the tagged symbol \"" + sym->getName() + "\" at 0x" +
+            Twine::utohexstr(vAddr) + "\" is not allowed to be zero");
----------------
When buf is null, addr is 0. If size is 0, we'd report an error without knowing the actual address. We should drop the address from the diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152921