[PATCH] D119384: [MTE] [lld] Add --memtag-* options to synthetise ELF notes.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Generally looks good, with some nits



================
Comment at: lld/ELF/Driver.cpp:725
+
+  error("--android-memtag-mode value of \"" + memtagModeArg +
+        "\" invalid, should be one of {async, sync, none}");
----------------
`unknown --android-memtag-mode value: memtagModeArg, should be one of ...`


================
Comment at: lld/test/ELF/aarch64-memtag-android-abi.s:8
+
+# RUN: llvm-mc --filetype=obj -triple=aarch64-linux-none-gnu %s -o %t.o
+# RUN: ld.lld --android-memtag-mode=async --android-memtag-heap %t.o -o %t
----------------
"none-gnu" in -triple=aarch64-linux-none-gnu seems invalid.

Did you mean aarch64-none-linux-android?


================
Comment at: lld/test/ELF/aarch64-memtag-android-abi.s:14
+# RUN: ld.lld --android-memtag-mode=sync --android-memtag-heap %t.o -o %t
+# RUN: llvm-readelf -n %t | \
+# RUN:    FileCheck %s --check-prefixes=NOTE,HEAP,NOSTACK,SYNC
----------------
Nit: These `llvm-readelf -n %t` RUN lines do not need a wrap. They aren't so long.


================
Comment at: lld/test/ELF/aarch64-memtag-android-abi.s:55
+# NOHEAP-NEXT: Heap: Disabled
+## Stack MTE is, as of Android 12, unimplemented. However, we pre-emptively emit
+## a bit that signifies to the dynamic loader to map the primary and thread
----------------
As of Android 12 stack MTE is unimplemented. ...


================
Comment at: lld/test/ELF/aarch64-memtag-android-abi.s:58
+## stacks as PROT_MTE, in preparation for the bionic support.
+# STACK-NEXT: Stack: Enabled
+# NOSTACK-NEXT: Stack: Disabled
----------------
Align


================
Comment at: lld/test/ELF/aarch64-memtag-android-abi.s:67
+# RUN:    FileCheck %s --check-prefix=MISSING-STACK-OR-HEAP
+# 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
----------------
Just place the message on one single line. This is more friendly to diagnostic search.
We don't enforce 80-line rule to test files anyway


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119384