[PATCH] D129326: [scudo] Pass MapPlatformData to all map/unmap calls

Dominic Chen via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 8 15:29:04 PDT 2022


ddcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/primary32.h:518
+  [[no_unique_address]] MapPlatformData StashMapData[MaxStashedRegions] = {};
+  [[no_unique_address]] MapPlatformData RegionsMapData[NumRegions] = {};
 };
----------------
cryptoad wrote:
> ddcc wrote:
> > cryptoad wrote:
> > > You might want to be careful with an array of `NumRegions` elements as this can become real large real fast.
> > > On Linux for now this isn't a problem as the `MapPlatformData` is empty, and Fuchsia doesn't support the 32-bit primary.
> > > But this is risky in terms of memory footprint.
> > Yeah, it does look to be a lot of overhead. I'm not using primary32.h at all except in testing, but my platform does require MapPlatformData be passed into all memory mapping system calls. I could just disable primary32.h in testing, but then building that header file would just be broken.
> A side point: I might be wrong but I think `StashMapData` redundant as the map data of stashed region could be stored in the overall `RegionMapData`.
> 
> Can you judge from your tests/compilation if the sizes of the binaries change at all?
I don't have an exact binary size change because we combine multiple tests into a single (64-bit) binary, but we define MapPlatformData to be uintptr_t[2], and for TestConfig1 from primary_test.cpp that uses SizeClassAllocator32, it seems to have a RegionSize of 0x40000. So RegionSize * sizeof(MapPlatformData) gives at least 4194304 bytes.

I'm not super familiar with this code, but when I implemented this originally, it wasn't immediately obvious. unmapTestOnly first unmaps everything in the RegionsStash, so I'd need the corresponding PlatformMapData object for each entry. From reading allocateRegionSlow and allocateRegion, it seems like entries in the stash should always be duplicate of an entry in StashMapData, since if allocateRegionSlow succeeds in placing a new entry in RegionsStash, it should always be able to pass it up to allocateRegion, which adds it to PossibleRegions? But is it possible to race between unmapTestOnly and allocateRegion/allocateRegionSlow? The former never seems to take RegionsStashMutex, so I don't know exactly what the mutex covers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129326



More information about the llvm-commits mailing list