[PATCH] D60787: [scudo][standalone] Introduce the Secondary allocator

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/scudo/standalone/secondary.cc:18
+// parameter allows us to unmap spurious memory when dealing with larger
+// (greater than a page) alignments.
+void *MapAllocator::allocate(uptr Size, uptr AlignmentHint, uptr *BlockEnd) {
----------------
Can we make this comment more specific?   It might be helpful to mention that `allocate` may return anywhere from `Size - AlignmentHint` to `Size` bytes of memory.


================
Comment at: lib/scudo/standalone/secondary.cc:19
+// (greater than a page) alignments.
+void *MapAllocator::allocate(uptr Size, uptr AlignmentHint, uptr *BlockEnd) {
+  DCHECK_GT(Size, AlignmentHint);
----------------
Would it be simpler to do the extra allocation and alignment here instead?


================
Comment at: lib/scudo/standalone/secondary.cc:37
+    // For alignments greater than or equal to a page, the user pointer ends
+    // up on a page boundary, and our headers will live in the preceding page.
+    CommitBase = roundUpTo(MapBase + PageSize + 1, AlignmentHint) - PageSize;
----------------
What does "user pointer" mean?

For "users" of this function the comment seems false. `CommitBase` will always be page-aligned, but at the end of this function we return `CommitBase + HeaderSize`, which may not be page-aligned.


================
Comment at: lib/scudo/standalone/secondary.cc:65
+  H->BlockEnd = CommitBase + CommitSize;
+  memcpy(&H->Data, &Data, sizeof(Data));
+  {
----------------
Can this be simplified to `H->Data = Data`?


================
Comment at: lib/scudo/standalone/secondary.cc:80
+    Stats.add(StatAllocated, CommitSize);
+    Stats.add(StatMapped, CommitSize);
+  }
----------------
Should this be `StatMapped += MapEnd - MapBase`?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787