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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: lld/ELF/Arch/AArch64.cpp:388
+    // derivation offset.
+    if (rel.sym && rel.sym->isTagged() &&
+        (rel.addend < 0 ||
----------------
MaskRay wrote:
> rel.sym is guaranteed to be non-null. The nullness check can be removed
Surprisingly, this causes failures in other tests if removed:

```
  lld :: ELF/aarch64-call26-thunk.s
  lld :: ELF/aarch64-cortex-a53-843419-large.s
  lld :: ELF/aarch64-cortex-a53-843419-thunk.s
  lld :: ELF/aarch64-jump26-thunk.s
  lld :: ELF/aarch64-range-thunk-extension-plt32.s
```

For now, leaving as-is.


================
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())))
----------------
MaskRay wrote:
> Consider using a single unsigned comparison and omitting the zero comparison.
(see same comment below about unsigned comparison)


================
Comment at: lld/ELF/Arch/AArch64.cpp:986
+  assert(config->emachine == EM_AARCH64);
+  assert(sec && sec->type == SHT_AARCH64_MEMTAG_GLOBALS_STATIC);
+
----------------
MaskRay wrote:
> We use const references to assert non-nullness.
Made the parameter byref, assuming you also mean the same about non-const refs.


================
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});
----------------
MaskRay wrote:
> `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())`, ...
I've gone ahead and fleshed out the comment, but I really don't think that relying on "signed integer when converted to unsigned is so large as to be out of range" is a great idea. While it may reduce one arithmetic operation, it decreases readability as the intent is no longer evident from the code (no matter how well it's described in the comments).


================
Comment at: lld/ELF/SyntheticSections.h:532
+    if (config->writeAddends && (expr != R_ADDEND || addend != 0) &&
+        !sym.isTagged())
       sec.addReloc({expr, addendRelType, offsetInSec, addend, &sym});
----------------
MaskRay wrote:
> 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`.
Removed this condition here, and banned `--apply-dynamic-relocs` when using MTE globals.

We could make it so `--apply-dynamic-relocs` works in all the places where it *can*, but this seems really prone to confusion ("why do I have the correct relocation here but not there? Oh, it's because of the MTE globals relocs!" isn't completely obvious).


================
Comment at: lld/ELF/SyntheticSections.h:1272
+  bool isNeeded() const override {
+    return !config->isStatic && !symbols.empty();
+  }
----------------
MaskRay wrote:
> 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.
MTE globals are not supported for fully static executables, there needs to be a dynamic loader that processes relocations.

Is there a better way to describe this in code? Maybe pulling `needsInterpSection` out of Writer.cpp?


================
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
----------------
MaskRay wrote:
> Do you mean this?
Yeah, ish. `[0-9a-f]+` is fine though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152921