[PATCH] D143769: [lld] [MTE] Add DT_AARCH64_MEMTAG_* dynamic entries, and small cleanup

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:739
   StringRef memtagModeArg = args.getLastArgValue(OPT_android_memtag_mode);
+
+  if (memtagModeArg.empty()) {
----------------
delete blank line


================
Comment at: lld/ELF/SyntheticSections.cpp:3853
   memcpy(buf + 12, kMemtagAndroidNoteName, sizeof(kMemtagAndroidNoteName));
-  buf += 12 + sizeof(kMemtagAndroidNoteName);
+  buf += 12 + alignTo(sizeof(kMemtagAndroidNoteName), 4);
 
----------------
hctim wrote:
> fmayer wrote:
> > should we have a static_assert that this is aligned to 4 instead? this seems like a no op
> yeah, it's a no-op.
> 
> there's also already a static_assert that the note name == 8 bytes, so adding an aligned-4 assert is probably unnecessary.
> 
> the reason why i put this here is because i wanted anyone who comes along and copy-paste implements an ELF note to not bugger up the alignment. for example, this caught me off guard when implementing an MTE globals note in the previous design. course, anyone implementing ELF notes should probably be consulting the manpage which does mention the alignment, but yeah, can be easy to miss.
> 
> WDYT?
Keeping `align` LGTM, as it documents the code without introducing runtime overhead (optimized out).


================
Comment at: lld/test/ELF/aarch64-memtag-android-abi.s:59
 # RUN:    FileCheck %s --check-prefix=MISSING-STACK-OR-HEAP
-# MISSING-STACK-OR-HEAP: when using --android-memtag-mode, at least one of --android-memtag-heap or --android-memtag-stack is required
+# MISSING-STACK-OR-HEAP:      when using --android-memtag-mode, at least one of
+# MISSING-STACK-OR-HEAP-SAME: --android-memtag-heap or --android-memtag-stack is required
----------------
Mention `warning:` for warnings. Don't use `-SAME` just to wrap lines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143769