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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added a comment.

Initial thoughts - I'll take a look at the stack depot / crash handler stuff but wanted to get a first round back to you ASAP.



================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:39
+// level 10000).
+extern "C" size_t android_unsafe_frame_pointer_chase(scudo::uptr *buf,
+                                                     size_t num_entries);
----------------
QQ - do we want to use per-plaftorm implementations, or do we want to cargo-cult the FP unwinder here? We'd then be able to provide stack traces (even without MTE) to all platforms. My gut says per-platform implementations should be the special case, not the generic. I don't feel strongly about this, but think it's worth raising the question.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:236
+#if SCUDO_ANDROID && __ANDROID_API__ == 10000
+    enum { kStackSize = 64 };
+    uptr Stack[kStackSize];
----------------
Why not `constexpr`?


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:241
+    // + 2 to discard collectStackTrace() frame and allocator function frame.
+    return Depot.insert(Stack + Min<uptr>(2, Size), Stack + Size);
+#else
----------------
If we discard two frames, we should probably collect two more in `android_unsafe_frame_pointer_chase `, right? The `64` here is more for ensuring the size of the depot doesn't grow too large, rather than how much stack space we alloca...


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:385
           resizeTaggedChunk(PrevEnd, TaggedUserPtr + Size, BlockEnd);
+          if (ZeroContents && Size) {
+            // Clear any stack metadata that may have previously been stored in
----------------
Zeroing the contents seems too important to be default off now that we store the previous tag (and thus the previous tag is accessible through an uninitialized-heap read). IMHO this should be done by default for MTE.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:724
+  void setTrackAllocationStacks(bool Track) {
+    Options.TrackAllocationStacks = Track;
+  }
----------------
`TrackAllocationStacks` is also used to track deallocation stacks. Behaviour seems reasonable, but consider a more descriptive name?


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:1016
+  void storeAllocationStackMaybe(void *Ptr) {
+    if (!UNLIKELY(Options.TrackAllocationStacks))
+      return;
----------------
Why `UNLIKELY` here? (and `storeDeallocationStackMaybe`)


================
Comment at: compiler-rt/lib/scudo/standalone/memtag.h:185
+inline uint8_t extractTag(uptr Ptr) {
+  return Ptr >> 56;
+}
----------------
Mask TBI bits here? We don't need them, and we should allow users to have them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77283