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

Tue Apr 23 07:51:29 PDT 2019

cryptoad marked 2 inline comments as done.
cryptoad 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);
----------------
morehouse wrote:
> 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?
The Combined will likely be the best place.

Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787