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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 14 inline comments as done.
cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/secondary.h:98
+                             roundUpTo((Size - AlignmentHint), PageSize) +
+                             PageSize;
+      DCHECK_LE(NewMapEnd, MapEnd);
----------------
morehouse wrote:
> When is `NewMapEnd < MapEnd`?
In order to be able to have an aligned address, we allocate Size+Alignment from the Combined (not checked-in yet), which matches the Primary behavior (and current sanitizer_common code) as explained in the comment.

At this point there is a chance that the address we get, with adjustments made for headers and whatnot, is already aligned to the requested alignment. This means we have about alignment bytes extra that can be unmap, at the end of the mapping. In that situation, we have NewMapEnd < MapEnd.
Another scenario being our aligned mapping being in the middle of the mapped area, in this situation we can also end up with spurious bytes towards the end that can be unmapped.


================
Comment at: lib/scudo/standalone/secondary.h:104
+        MapEnd = NewMapEnd;
+      }
+    }
----------------
morehouse wrote:
> So we potentially shrink the committed region for large alignments but did not allocate extra beforehand.  Does this mean the actual region size may be less than the requested size?
We technically allocated extra from the Combined (see function main comment).


================
Comment at: lib/scudo/standalone/secondary.h:190
+
+  template <typename F> void iterateOverBlocks(F Callback) const {
+    for (LargeBlock::Header *H = Tail; H != nullptr; H = H->Prev)
----------------
vitalybuka wrote:
> cryptoad wrote:
> > vitalybuka wrote:
> > > Wouldn't be better if we lock inside of iterateOverBlocks
> > See the comment above. Due to how the APIs are used in Android (disable/iterate/enable), I mirrored the behavior here.
> Do Android blocks users as well?
Checked with cferris@: the expected behavior for disable is to pause any further allocation/deallocation request until enable is called.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787