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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc marked 6 inline comments as done.
pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:164
 
+  void *maybeUntagPointer(void *Ptr) {
+    if (Primary.SupportsMemoryTagging)
----------------
cryptoad wrote:
> 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.
Yeah, the `Maybe` at the beginning is more consistent with the rest of LLVM. For now I'll put it at the end and we can change it later.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:510
 
+  static_assert(!PrimaryT::SupportsMemoryTagging ||
+                MinAlignment >= archMemoryTagGranuleSize(), "");
----------------
cryptoad wrote:
> 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.
The rest of LLVM uses `static_assert` directly. I'll change this one to `COMPILER_CHECK` but maybe this could be changed over to being consistent with LLVM as well?


================
Comment at: compiler-rt/lib/scudo/standalone/linux.cpp:39
+#ifdef ANDROID_EXPERIMENTAL_MTE
+#include <bionic/mte_kernel.h>
+#endif
----------------
cryptoad wrote:
> Does this work when compiled on Android but outside of Bionic?
Yes, as long as you're building scudo as part of the Android platform (the header:
https://android.googlesource.com/platform/bionic/+/900d07d6a1f3e1eca8cdbb3b1db1ceeec0acc9e2/libc/platform/bionic/mte_kernel.h
is deliberately not exposed in the NDK). Unfortunately this means that the MTE tests can't run as part of check-scudo, but the situation will hopefully improve once we no longer need `ANDROID_EXPERIMENTAL_MTE`.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:23
+
+inline constexpr bool archSupportsMemoryTagging() { return true; }
+inline constexpr size_t archMemoryTagGranuleSize() { return 16; }
----------------
cryptoad wrote:
> If you could use the `INLINE` macros for consistency please.
Sure, again `inline` is what the rest of LLVM uses.


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:24
+inline constexpr bool archSupportsMemoryTagging() { return true; }
+inline constexpr size_t archMemoryTagGranuleSize() { return 16; }
+
----------------
cryptoad wrote:
> 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?
Probably not in the inline asm parts but that only leaves a couple of places. The MTE-specific part of the code is small enough that I reckon that it's clear enough to just write out the constant.


================
Comment at: compiler-rt/lib/scudo/standalone/primary64.h:43
+template <class SizeClassMapT, uptr RegionSizeLog,
+          bool MaySupportMemoryTagging>
+class SizeClassAllocator64 {
----------------
cryptoad wrote:
> Maybe default to `false`?
Yeah, that's a good idea, I'll do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762