[PATCH] D64173: Basic MTE stack tagging instrumentation.

[Oliver Stannard (Linaro) via Phabricator via llvm-commits]

ostannard added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:66
+    SmallVector<IntrinsicInst *, 2> LifetimeEnd;
+    int Tag;
+  };
----------------
You are using -1 as a sentinel value for non-tagged allocations, I think a comment about that here would be useful.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:93
+  Function *F;
+  Function *SetTagFunc;
+  const DataLayout *DL;
----------------
This is only ever written, and shadowed in tagAlloca and untagAlloca.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:115
+uint64_t
+AArch64StackTagging::getAllocaSizeInBytes(const AllocaInst &AI) const {
+  uint64_t ArraySize = 1;
----------------
Could this just use AllocaInst::getAllocationSizeInBits, which implements the same logic?


================
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.
----------------
Is there a fundamental reason we can't tag non-static allocas? Or could/will that be added by a later patch?


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


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


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:247
+    unsigned NewAlignment = std::max(Info.AI->getAlignment(), MinAlignment);
+    Info.AI->setAlignment(NewAlignment);
+
----------------
Do we still need to change the alignment for allocas we're not going to tag?


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:254
+      Info.Tag = NextTag;
+      NextTag = (NextTag + 1) % 16;
+    }
----------------
This is more of a suggestion for future improvement, doesn't need to be fixed in this patch:

I think picking consecutive tags could result in the same tag for two adjacent allocations, if exactly one tag is disabled by GCR_EL1.Exclude (which I expect to be common, to reserve one tag for otherwise un-tagged memory. In this case, it is likely that the tag picked by "ADDG ... #15" and "ADDG ... #0" will be the same, because the former will have skipped over the excluded tag, resulting in an addition of 16 % 16 == 0.

One option would be to change this to "(NextTag + 1) % 14" (exact value to be bike-shedded), which would avoid generating adjacent tags if up to two bits are set in CR_EL1.Exclude.

Do you happen to know what the linux kernel (or any other OS) plans to set GCR_EL1.Exclude to?


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:268
+  if (NumInterestingAllocas > 1) {
+    auto *DTWP = getAnalysisIfAvailable<DominatorTreeWrapperPass>();
+    if (DTWP) {
----------------
I think this could just use getAnalysis, since we're going to compute it anyway if it's not available.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64173