[PATCH] D70762: scudo: Add initial memory tagging support.

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added a comment.

Thank you Peter this is awesome!



================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:164
 
+  void *maybeUntagPointer(void *Ptr) {
+    if (Primary.SupportsMemoryTagging)
----------------
nit on naming: I usually put the verb 1st and `Maybe` last.
I might have been wrong with regard to LLVM style and can change the other names, but I'd like to keep the naming scheme consistent.
Let me know which of the 2 is preferable.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:510
 
+  static_assert(!PrimaryT::SupportsMemoryTagging ||
+                MinAlignment >= archMemoryTagGranuleSize(), "");
----------------
I you could use `COMPILER_CHECK`, as it is used in other places.
It ends up being a `static_assert` but it feel more consistent.


================
Comment at: compiler-rt/lib/scudo/standalone/linux.cpp:39
+#ifdef ANDROID_EXPERIMENTAL_MTE
+#include <bionic/mte_kernel.h>
+#endif
----------------
Does this work when compiled on Android but outside of Bionic?


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:23
+
+inline constexpr bool archSupportsMemoryTagging() { return true; }
+inline constexpr size_t archMemoryTagGranuleSize() { return 16; }
----------------
If you could use the `INLINE` macros for consistency please.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:24
+inline constexpr bool archSupportsMemoryTagging() { return true; }
+inline constexpr size_t archMemoryTagGranuleSize() { return 16; }
+
----------------
I assume that when you do `roundUpTo(NewSize, 16)` and all the 16-related arithmetic, it's related to the granule size.
Could it be constant'd out in the whole file?


================
Comment at: compiler-rt/lib/scudo/standalone/primary64.h:43
+template <class SizeClassMapT, uptr RegionSizeLog,
+          bool MaySupportMemoryTagging>
+class SizeClassAllocator64 {
----------------
Maybe default to `false`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762