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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Just a few small comments so far. I've got up to Relocations.cpp. Hope to carry on later this week.



================
Comment at: lld/ELF/Arch/AArch64.cpp:387
+    if (rel.sym && rel.sym->isTagged() &&
+        (rel.addend < 0 || rel.addend >= rel.sym->getSize())) {
+      write64(loc, -1 * rel.addend);
----------------
A small nit. LLD tends to prefer no curly brackets when there is only one statement.


================
Comment at: lld/ELF/Arch/AArch64.cpp:668
   Symbol &sym = *adrpRel.sym;
+  // Tagged symbols have upper address bits that are added by the dynamic
+  // loader, and thus need the full 64-bit GOT entry. Do not relax such symbols.
----------------
IIUC this will change an ADRP, ADD into an ADR, which is just a shorter ranged ADRP/ADD. This relaxation doesn't change the destination address like tryRelaxAdrpLdr might.

. 


================
Comment at: lld/ELF/Arch/AArch64.cpp:780
     switch (rel.expr) {
     case R_AARCH64_GOT_PAGE_PC:
       if (i + 1 < size &&
----------------
To help prevent future relaxations from not considering MTE, It may be worth moving the sym.isTagged() to here with a general condition similar to needsGot in relocations.cpp.

For example don't relax if the symbol is tagged and is relative to a GOT reference.

Alternatively could be worth a comment in the Relaxer class to highlight the restriction.


================
Comment at: lld/ELF/Driver.cpp:354
-  
+
   if (config->tocOptimize && config->emachine != EM_PPC64)
     error("--toc-optimize is only supported on PowerPC64 targets");
----------------
Not sure what this difference is from. If it has come from clang-format it may be worth NFC committing it separately to avoid it getting associated with this patch.


================
Comment at: lld/ELF/Driver.cpp:757
 
-  if (!config->androidMemtagHeap && !config->androidMemtagStack) {
-    error("when using --android-memtag-mode, at least one of "
----------------
To check my understanding; this is being deleted because protecting globals is independent of heap and stack tagging.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152921