[PATCH] D64173: Basic MTE stack tagging instrumentation.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis marked 2 inline comments as done.
eugenis added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:254
+      Info.Tag = NextTag;
+      NextTag = (NextTag + 1) % 16;
+    }
----------------
ostannard wrote:
> 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?
> One option would be to change this to "(NextTag + 1) % 14" 

We could do something fancy like this:
0, 1, ..., 7, 15, 14, ..., 8, (repeat).
This way adjacent tags with never math up to 8 excluded tags.

But I have plans to reorder stack slots in the backend to put more of them within ADDG immediate range from the base tagged pointer. That would make the results of this change less predictable.

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

Either 0 or 1 (this thread's SP tag).



================
Comment at: llvm/lib/Target/AArch64/AArch64StackTagging.cpp:268
+  if (NumInterestingAllocas > 1) {
+    auto *DTWP = getAnalysisIfAvailable<DominatorTreeWrapperPass>();
+    if (DTWP) {
----------------
ostannard wrote:
> I think this could just use getAnalysis, since we're going to compute it anyway if it's not available.
In the old pass manager, that would require registering DT as a dependency in getAnalysisUsage, which would cause it to be recomputed every time, even if the function does not have the sanitize_memtag attribute.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64173