[PATCH] D77283: scudo: Add support for diagnosing memory errors when memory tagging is enabled.

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added a comment.

Some initial comments



================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:35
 
+#if SCUDO_ANDROID && __ANDROID_API__ == 10000
+// This function is not part of the NDK so it does not appear in any public
----------------
I am not sure how `__ANDROID_API__` works, should it be `>=`? (same for all the others)


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:237
+    enum { kStackSize = 64 };
+    uptr Stack[kStackSize];
+    uptr Size = android_unsafe_frame_pointer_chase(Stack, kStackSize);
----------------
Style wise, the whole code base avoids the `kConstant` naming.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:388
+            // the chunk data.
+            memset(TaggedPtr, 0, 16);
+          }
----------------
Should the `16` be a constant?


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:751
+
+    auto GetGranule = [&](uptr Addr, const char **Data, uint8_t *Tag) -> bool {
+      if (Addr < MemoryAddr || Addr >= MemoryAddr + MemorySize)
----------------
Not sure if the lambda should abide by the usual style, eg `getGranule`, up to you.


================
Comment at: compiler-rt/lib/scudo/standalone/stack_depot.h:12
+
+#include "mutex.h"
+
----------------
Style on that file went the g3 way rather than the LLVM way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77283