[PATCH] D43949: [scudo] Secondary allocator overhaul to support Windows

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator_secondary.h:108
     const uptr Ptr = UserBeg - Chunk::getHeaderSize();
-    ReservedAddressRange *StoredRange = getReservedAddressRange(Ptr);
-    *StoredRange = AddressRange;
+    LargeChunkHeader *H = LargeChunk::getHeader(Ptr);
+    H->StoredRange = AddressRange;
----------------
alekseyshl wrote:
> cryptoad wrote:
> > alekseyshl wrote:
> > > This is definitely confusing. One getHeader() expects user mem ptr, another expects chunk header ptr. Not sure what's the best way to improve it yet.
> > Initially I was going to add a comment with some chunk layout, eg:
> > ```+------------------+
> > | Guard page(s)    |
> > +------------------+
> > | Unused space*    |
> > +------------------+
> > | LargeChunkHeader |
> > +------------------+
> > | PackedHeader     |
> > +------------------+
> > | Data             |
> > +------------------+
> > | Unused space**   |
> > +------------------+
> > | Guard page(s)    |
> > +------------------+```
> > The frontend deals with the {Unp,P}ackedHeader, and the Secondary being part of the Backend deals with the LargeChunkHeader.
> > While I agree that the distinction can be tenuous to make, it makes sense to me that the 2 `getHeader` work on different pointers.
> I would agree with Frontend/Backend separation argument, should this class not be aware of and not use "Chunk::getHeaderSize()".
This is indeed the annoying part.
I was thinking about this recently: in the current state of things we need the frontend header size because the user pointer has to be aligned, so we have to know how much space the frontend header is taking. I could pass that header size as `ExtraBytes` or something to the Secondary that would still do the necessary adjustments.
I don't see a  way out without the secondary taking care of it's own alignment needs.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D43949