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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Thank you for the updates and sorry for the delayed response. I took a look again.

You can ping me more frequently:) I am going to take a trip and will likely have slow response for ~2 weeks since Jul 27.



================
Comment at: lld/ELF/Arch/AArch64.cpp:388
+    // derivation offset.
+    if (rel.sym && rel.sym->isTagged() &&
+        (rel.addend < 0 ||
----------------
rel.sym is guaranteed to be non-null. The nullness check can be removed


================
Comment at: lld/ELF/Arch/AArch64.cpp:389
+    if (rel.sym && rel.sym->isTagged() &&
+        (rel.addend < 0 ||
+         rel.addend >= static_cast<int64_t>(rel.sym->getSize())))
----------------
Consider using a single unsigned comparison and omitting the zero comparison.


================
Comment at: lld/ELF/Arch/AArch64.cpp:985
+                          DenseMap<Symbol *, unsigned> &referenceCount) {
+  assert(config->emachine == EM_AARCH64);
+  assert(sec && sec->type == SHT_AARCH64_MEMTAG_GLOBALS_STATIC);
----------------
This assert is unneeded now that this code lives in AArch64.cpp


================
Comment at: lld/ELF/Arch/AArch64.cpp:986
+  assert(config->emachine == EM_AARCH64);
+  assert(sec && sec->type == SHT_AARCH64_MEMTAG_GLOBALS_STATIC);
+
----------------
We use const references to assert non-nullness.


================
Comment at: lld/ELF/Arch/AArch64.cpp:990
+  if (rels.areRelocsRel())
+    error("non-RELA relocations are not allowed with memtag globals");
+
----------------
This error is untested. We can use yaml2obj to craft a test.


================
Comment at: lld/ELF/Arch/AArch64.cpp:1033
+  for (InputFile* file : files) {
+    if (file->kind() != InputFile::BinaryKind &&
+        file->kind() != InputFile::ObjKind)
----------------
A `BinaryFile` has just one `.data` section. There can't be SHT_AARCH64_MEMTAG_GLOBALS_STATIC sections.


================
Comment at: lld/ELF/Arch/AArch64.cpp:1067
+      }
+      remainingAllowedTaggedRefs--;
+    }
----------------
llvm style prefers pre-increments.


================
Comment at: lld/ELF/Relocations.cpp:861
+    // tag should be derived from.
+    if (addend < 0 || static_cast<uint64_t>(addend) >= sym.getSize())
+      isec.relocations.push_back({expr, type, offsetInSec, addend, &sym});
----------------
`case R_AARCH64_ABS64:` in AArch64.cpp uses the signed `int64_t`, different from here.

Perhaps we can just omit the `< 0` comparison and rely on unsigned comparison? If `int64_t(addend) < 0`, `uint64_t(addend)` will be very large.

Then adjust the comment to 

// For tagged symbols with addends outside of `[0, sym.getSize())`, ...


================
Comment at: lld/ELF/SyntheticSections.h:532
+    if (config->writeAddends && (expr != R_ADDEND || addend != 0) &&
+        !sym.isTagged())
       sec.addReloc({expr, addendRelType, offsetInSec, addend, &sym});
----------------
This change is untested. We seem to need a `--apply-dynamic-relocs` test inspecting the relocated data with `llvm-readelf -x` or `llvm-objdump -s`.


================
Comment at: lld/ELF/SyntheticSections.h:1257
+                         llvm::ELF::SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC,
+                         /*alignment=*/ 4, ".memtag.globals.dynamic") {}
+  void writeTo(uint8_t *buf) override;
----------------
clang-format will remove the space


================
Comment at: lld/ELF/SyntheticSections.h:1267
+  void addSymbol(const Symbol *sym) {
+    assert(sym && sym->isDefined());
+    symbols.push_back(sym);
----------------
For new code, we use const reference to assert non-nullness.


================
Comment at: lld/ELF/SyntheticSections.h:1272
+  bool isNeeded() const override {
+    return !config->isStatic && !symbols.empty();
+  }
----------------
Why is the `!config->isStatic &&` condition?

`isConfig` can be toggled by `-Bstatic`/`-static` and `-Bdynamic`. 
`-Bstatic` can be used with a dynamically linked executable as well.


================
Comment at: lld/ELF/SyntheticSections.h:1276
+private:
+  std::vector<const Symbol *> symbols;
+};
----------------
Nit: `SmallVector<const Symbol *, 0>` is likely more efficient due to smaller size :)


================
Comment at: lld/ELF/SyntheticSections.h:1312
   std::unique_ptr<PackageMetadataNote> packageMetadataNote;
+  std::unique_ptr<MemtagDescriptors> memtagDescriptors;
   std::unique_ptr<RelocationBaseSection> relaDyn;
----------------
Place this after `memtagAndroidNote` for an alphabetical order


================
Comment at: lld/test/ELF/aarch64-memtag-globals.s:87
+# CHECK: Memtag Dynamic Entries
+# CHECK: AARCH64_MEMTAG_MODE: Synchronous (0)
+# CHECK: AARCH64_MEMTAG_HEAP: Disabled (0)
----------------
Add `-NEXT` whenever appropriate.


================
Comment at: lld/test/ELF/aarch64-memtag-globals.s:90
+# CHECK: AARCH64_MEMTAG_STACK: Disabled (0)
+# CHECK: AARCH64_MEMTAG_GLOBALS: {{.*[1-9a-f]+}}
+# CHECK: AARCH64_MEMTAG_GLOBALSSZ: 23
----------------
Do you mean this?


================
Comment at: lld/test/ELF/aarch64-memtag-globals.s:98
+
+# CHECK:     Memtag Global Descriptors:
+# CHECK-NOT: 0x[[#GLOBAL_UNTAGGED]]:
----------------
Perhaps:
```
# CHECK:      Memtag Global Descriptors:
# CHECK-NEXT:   0x[[#POINTER_TO_GLOBAL]]: 0x10
# CHECK-NEXT:   0x[[#POINTER_INSIDE_GLOBAL]]: 0x10
...
# CHECK-NOT:  {{.}}
```

We already rely on the order, so we can just use `-NEXT:` patterns to avoid `CHECK-NOT:`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152921