[PATCH] D93731: [wip] scudo: Support memory tagging in the secondary allocator.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:259
+    auto PtrInt = reinterpret_cast<uptr>(Ptr);
+    return reinterpret_cast<void *>(addFixedTag(PtrInt, 2));
   }
----------------
Do you think it will be too expensive to make the tag random and use LDG to read it in the allocator?


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:1051
   // Return the size of a chunk as requested during its allocation.
   inline uptr getSize(const void *Ptr, Chunk::UnpackedHeader *Header) {
     const uptr SizeOrUnusedBytes = Header->SizeOrUnusedBytes;
----------------
Rename the argument to "headerTaggedPtr" or something like that


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:129
+      Entry.Time = Time;
+      setMemoryPermission(Entry.CommitBase, Entry.CommitSize, MAP_NOACCESS,
+                          &Entry.Data);
----------------
Do you expect this to be faster than the other branch? If we are doing an mm syscall anyway, why not release the memory immediately?


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:464
+    const uptr NewMapEnd =
+        CommitBase + PageSize + roundUpTo(Size, PageSize) + PageSize;
     DCHECK_LE(NewMapEnd, MapEnd);
----------------
is this an unrelated bugfix?


================
Comment at: compiler-rt/lib/scudo/standalone/secondary.h:474
+  map(reinterpret_cast<void *>(CommitBase), CommitSize, "scudo:secondary",
+      MAP_RESIZABLE | MAP_MEMTAG, &Data);
+  const uptr HeaderPos =
----------------
I don't know if it is worth the cost of the extra syscall, but we only need MAP_MEMTAG on the first page of the mapping.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93731