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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

Thanks for the updates. The code-changes look good to me on my current understanding. I've made a few suggestions on the testing. I'm hoping to read through the spec in detail to see if I've missed anything, but I don't want to let that hold up the patch if I can't get round to it in a reasonable amount of time.



================
Comment at: lld/test/ELF/Inputs/aarch64-memtag-globals-1.s:12
+##    const int const_global = 0;
+##    static const int hidden_const_global = 0;
+##    static char hidden_global[12] = {};
----------------
Speaking of hidden, I'm guessing that __attribute__((visibility("hidden"))) might affect a variable being in the GOT in the same way as static. If it does then could be worth adding a test.


================
Comment at: lld/test/ELF/Inputs/aarch64-memtag-globals-1.s:51
+##        &global_extern_untagged_definition_but_tagged_import;
+
+	.text
----------------
The spec says that TLS variables are not tagged (although that could change), is it worth adding a case for TLS.


================
Comment at: lld/test/ELF/aarch64-memtag-globals.s:4
+# RUN: llvm-mc --filetype=obj -triple=aarch64-none-linux-android \
+# RUN:   %S/Inputs/aarch64-memtag-globals-1.s -o %t1.o
+# RUN: llvm-mc --filetype=obj -triple=aarch64-none-linux-android \
----------------
One thing that might be worth doing is using split-file (see aarch64-thunk-pi.s for an example ) to include instructions to create aarch64-memtag-globals-1.s and memtag-globals-2.s in the same source file. There's a chance it makes it too big to be worth the effort, but could be worth an expirement to see if it is easier to read than splitting across several files.


================
Comment at: lld/test/ELF/aarch64-memtag-globals.s:10
+## Normally relocations are printed before the symbol tables, so reorder it a
+## bit to make it easier on matching addresses of relocatiosn up with the
+## symbols.
----------------
typo in relocatiosn 


================
Comment at: lld/test/ELF/aarch64-memtag-globals.s:16
+# RUN: llvm-objdump -Dz %t.so | FileCheck %s --check-prefix=CHECK-SPECIAL-RELOCS
+
+# CHECK: Symbol table '.dynsym' contains
----------------
Worth dumping the dynamic tags to check for DT_AARCH64_MEMTAG_GLOBALS and DT_AARCH64_MEMTAG_GLOBALSSZ ?

Also could be worth dumping section headers to check for SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152921