[PATCH] D40754: [sanitizer] 64-bit allocator's PopulateFreeArray partial refactor

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/sanitizer_common/sanitizer_allocator_primary64.h:723
+      region->mapped_meta += meta_map_size;
     }
 
----------------
alekseyshl wrote:
> cryptoad wrote:
> > alekseyshl wrote:
> > > I'd suggest to wrap the entire metadata allocating block (from // Calculate the required space for metadata) with:
> > > 
> > >   if (kMetadataSize) {
> > >     ...
> > >   }
> > So I thought about it, but I was wondering about something: currently the check for exhaustion will be done whether or not we use metadata.
> > I wanted to leave it like that at first, because it sort of made sense.
> > But then I figure that if we are not using metadata, and are exhausted, then the initial mmap would fail anyway since we'd hit the free array.
> > It seems to be safe to place all of this within `if (kMetadataSize)`, but I wanted to raise this to your attention and get your feedback.
> Yep, now we will return false due to MapWithCallback failure and the OOM error handling will kick in. I think, it's just what we want.
Tried out wrapping everything within a `if (kMetadataSize)`.
The `SizeClassAllocator64PopulateFreeListOOM` test fails.
If we don't keep the exhaustion check out of the if, the map is successful, and we "overflow" into the free array space (it's empty and mappable).
So I think the current way is the way to go.


https://reviews.llvm.org/D40754