[PATCH] D128958: Add assembler plumbing for sanitize_memtag

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCDirectives.h:49
+  MCSA_WeakDefAutoPrivate,      ///< .weak_def_can_be_hidden (MachO)
+  MCSA_Tagged                   ///< .tagged (ELF)
 };
----------------
So that next time a value is added, no need to touch the `MCSA_Tagged` line.


================
Comment at: llvm/include/llvm/MC/MCELFObjectWriter.h:143
+
+  // On AArch64, returns a new section to be added to the ELF object that
+  // contains relocations used to describe every symbol that should have memory
----------------
Use imperative sentences. (Omitting s is much more common)


================
Comment at: llvm/lib/MC/ELFObjectWriter.cpp:1087
 
+  MCSectionELF *MemtagRelocs = nullptr;
+  for (const MCSymbol &Sym : Asm.symbols()) {
----------------
The chunk of code doesn't belong here. See `finalizeCGProfile`


================
Comment at: llvm/lib/MC/MCParser/AsmParser.cpp:5607
   DirectiveKindMap[".lto_set_conditional"] = DK_LTO_SET_CONDITIONAL;
+  DirectiveKindMap[".tagged"] = DK_TAGGED;
 }
----------------
The name `.tagged` is general. Worth using a name more specific to memtag.


================
Comment at: llvm/lib/MC/MCSymbolELF.cpp:201
+bool MCSymbolELF::isMemoryTagged() const {
+  return getFlags() & (0x1 << ELF_IsMemoryTagged_Shift);
+}
----------------
you may omit `0x`


================
Comment at: llvm/lib/MC/MCSymbolELF.cpp:207
+  if (Tagged)
+    setFlags(OtherFlags | (0x1 << ELF_IsMemoryTagged_Shift));
+  else
----------------
you may omit `0x`


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:37
 
+  MCSectionELF* getMemtagRelocsSection(MCContext &Ctx) const override;
+
----------------



================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFObjectWriter.cpp:459
+MCSectionELF* AArch64ELFObjectWriter::getMemtagRelocsSection(MCContext &Ctx) const {
+  MCSectionELF *Sec = Ctx.getELFSection(".memtag.globals", ELF::SHT_ANDROID_MEMTAG_GLOBALS, 0);
+  return Sec;
----------------
Omit the variable. Just return


================
Comment at: llvm/test/CodeGen/AArch64/global-tagging.ll:3
+; RUN: FileCheck %s --input-file=%t.S --check-prefix=CHECK-ASM
+; RUN: llvm-mc -filetype=obj %t.S -triple=aarch64-linux-android31 -o %t.o
+; RUN: llvm-objdump -r %t.o | FileCheck %s --check-prefix=CHECK-RELOCS
----------------
llvm-mc tests belong to test/MC/AArch64


================
Comment at: llvm/test/CodeGen/AArch64/global-tagging.ll:4
+; RUN: llvm-mc -filetype=obj %t.S -triple=aarch64-linux-android31 -o %t.o
+; RUN: llvm-objdump -r %t.o | FileCheck %s --check-prefix=CHECK-RELOCS
+
----------------
`llvm-readelf -r` is better when the number of relocations is to be checked.


================
Comment at: llvm/test/CodeGen/AArch64/global-tagging.ll:6
+
+; RUN: obj2yaml %t.o > %t.yaml
+; RUN: FileCheck %s --input-file=%t.yaml --check-prefix=CHECK-YAML
----------------
Check `D107949` how I add yaml2obj/obj2yaml/llvm-readobj/llvm-objdump. You can use that as the first patch, and llvm-mc support as the second.


================
Comment at: llvm/test/CodeGen/AArch64/global-tagging.ll:12
+; CHECK-RELOCS: RELOCATION RECORDS FOR [.memtag.globals]:
+; CHECK-RELOCS-NEXT: OFFSET TYPE VALUE
+; CHECK-YAML: Sections:
----------------
It usually reads better if for a particular check prefix, the content is aligned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128958