[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:19
+// (greater than a page) alignments.
+void *MapAllocator::allocate(uptr Size, uptr AlignmentHint, uptr *BlockEnd) {
+  DCHECK_GT(Size, AlignmentHint);
----------------
cryptoad wrote:
> morehouse wrote:
> > Would it be simpler to do the extra allocation and alignment here instead?
> I went back and forth on how to deal with this and I have to say it's been annoying.
> Technically the secondary doesn't (shouldn't?) know about the header size of the combined.
> For N bytes aligned on A, the allocation done by the secondary includes a largeblock header, but also the block header. While this could be solved by passing the extra header bytes to the secondary, it would make it depart significantly from the primary: the primary returns a block that straddles the allocation boundary and doesn't care about anything else.
> By building it that way, we have a very similar approach between Primary & Secondary: the combined rounds up the size and request the alloc. The real problem (it's still debatable if it's an actual problem) would be on 32-bit when requesting secondary allocations with large alignments. The unmapping becomes necessary due to the sparsity of the address space. For 64-bit, this is a non-issue.\
> The resulting function might appear hackish in the end, but it ends up fitting decently in the combined.
I don't especially like the current setup since it is hard to understand, but if it's actually cleaner this way I guess we'll have to bite the bullet.

I do think some high-level design comments would be helpful to understand how the primary/secondary/combined components interact with each other.  Not sure where the best place is for that, maybe the combined allocator?


================
Comment at: lib/scudo/standalone/secondary.cc:45
+    // up on a page boundary (eg: the pointer that is returned by the C or C++
+	// allocation APIs), and our headers will live in the preceding page.
+    CommitBase = roundUpTo(MapBase + PageSize + 1, AlignmentHint) - PageSize;
----------------
The explanation in parens makes more sense right after the term "user pointer".


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787