[PATCH] D64173: Basic MTE stack tagging instrumentation.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis marked an inline comment as done.
eugenis added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:129
+  bool IsInteresting =
+      AI.getAllocatedType()->isSized() && AI.isStaticAlloca() &&
+      // alloca() may be called with 0 size, ignore it.
----------------
ostannard wrote:
> Is there a fundamental reason we can't tag non-static allocas? Or could/will that be added by a later patch?
It will be done in a later patch. Added a FIXME.



================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:140
+
+AllocaInst *AArch64StackTagging::findAllocaForLifetime(Value *V) {
+  SmallVector<Value *, 4> Objs;
----------------
ostannard wrote:
> This function isn't used anywhere.
removed


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:213
+      if (auto *AI = dyn_cast<AllocaInst>(I)) {
+	Allocas[AI].AI = AI;
+	continue;
----------------
ostannard wrote:
> Indentation, are there tabs in this file?
Indeed, that was a tab. Reformatted the entire file.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:247
+    unsigned NewAlignment = std::max(Info.AI->getAlignment(), MinAlignment);
+    Info.AI->setAlignment(NewAlignment);
+
----------------
ostannard wrote:
> Do we still need to change the alignment for allocas we're not going to tag?
I've uploaded the new version that pads allocas with [N x i8] in addition to realigning it.
This way, I believe, untagged allocas do not need to be aligned.

Padding is necessary anyway because STG is technically a memory access of size 16; if alloca size is less than that, BasicAA may conclude that this is impossible and they do not alias.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64173