[scudo] Pass MapPlatformData to all map/unmap calls

[ddcc]

Summary

This modifies all remaining map/unmap calls in scudo so that MapPlatformData is always passed.

[ddcc]

Not sure if this approach is correct since we're not using the primary32.h allocator at all except during testing, but I'm sending it along to get some feedback and in case it's useful.

[cryptoad]

compiler-rt/lib/scudo/standalone/primary32.h

518

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.

[ddcc]

compiler-rt/lib/scudo/standalone/primary32.h

518

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.

[cryptoad]

compiler-rt/lib/scudo/standalone/primary32.h

518

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?

[ddcc]

compiler-rt/lib/scudo/standalone/primary32.h

518

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.

[vitalybuka]

Is this still relevant?

[ddcc]

Yes, we need the MapPlatformData structure to always be allocated, though we only run the primary32.h allocator in tests and not in practice, so there shouldn't be much impact.

compiler-rt/lib/scudo/standalone/common.h

 // Our platform memory mapping use is restricted to 3 scenarios:
 // - reserve memory at a random address (MAP_NOACCESS);
 // - commit memory in a previously reserved space;
 // - commit memory at a random address.
 // As such, only a subset of parameters combinations is valid, which is checked
 // by the function implementation. The Data parameter allows to pass opaque
 // platform specific data to the function.
 // Returns nullptr on error or dies if MAP_ALLOWNOMEM is not specified.
-void *map(void *Addr, uptr Size, const char *Name, uptr Flags = 0,
-          MapPlatformData *Data = nullptr);
+void *map(void *Addr, uptr Size, const char *Name, uptr Flags,
+          MapPlatformData *Data);
 
 // Indicates that we are getting rid of the whole mapping, which might have
 // further consequences on Data, depending on the platform.
 #define UNMAP_ALL (1U << 0)
 
-void unmap(void *Addr, uptr Size, uptr Flags = 0,
-           MapPlatformData *Data = nullptr);
+void unmap(void *Addr, uptr Size, uptr Flags, MapPlatformData *Data);
 
 void setMemoryPermission(uptr Addr, uptr Size, uptr Flags,
-                         MapPlatformData *Data = nullptr);
+                         MapPlatformData *Data);
 
 void releasePagesToOS(uptr BaseAddress, uptr Offset, uptr Size,
-                      MapPlatformData *Data = nullptr);
+                      MapPlatformData *Data);
 
 // Internal map & unmap fatal error. This must not call map(). SizeIfOOM shall
 // hold the requested size on an out-of-memory error, 0 otherwise.
 void NORETURN dieOnMapUnmapError(uptr SizeIfOOM = 0);
 
 // Logging related functions.
 
 void setAbortMessage(const char *Message);

compiler-rt/lib/scudo/standalone/primary32.h

for (uptr I = 0; I < NumClasses; I++) {
       // Sci->MaxRegionIndex is already initialized to 0.
       Sci->MinRegionIndex = NumRegions;
       Sci->ReleaseInfo.LastReleaseAtNs = Time;
     }
     setOption(Option::ReleaseInterval, static_cast<sptr>(ReleaseToOsInterval));
   }
 
   void unmapTestOnly() {
-    while (NumberOfStashedRegions > 0)
-      unmap(reinterpret_cast<void *>(RegionsStash[--NumberOfStashedRegions]),
-            RegionSize);
+    while (NumberOfStashedRegions > 0) {
+      NumberOfStashedRegions -= 1;
+      unmap(reinterpret_cast<void *>(RegionsStash[NumberOfStashedRegions]),
+            RegionSize, 0, &StashMapData[NumberOfStashedRegions]);
+    }
     uptr MinRegionIndex = NumRegions, MaxRegionIndex = 0;
     for (uptr I = 0; I < NumClasses; I++) {
       SizeClassInfo *Sci = getSizeClassInfo(I);
       if (Sci->MinRegionIndex < MinRegionIndex)
         MinRegionIndex = Sci->MinRegionIndex;
       if (Sci->MaxRegionIndex > MaxRegionIndex)
         MaxRegionIndex = Sci->MaxRegionIndex;
       *Sci = {};
     }
     for (uptr I = MinRegionIndex; I < MaxRegionIndex; I++)
-      if (PossibleRegions[I])
-        unmap(reinterpret_cast<void *>(I * RegionSize), RegionSize);
+      if (PossibleRegions[I]) {
+        unmap(reinterpret_cast<void *>(I * RegionSize), RegionSize, 0,
+              &RegionsMapData[I]);
+      }
     PossibleRegions.unmapTestOnly();
   }
 
   CompactPtrT compactPtr(UNUSED uptr ClassId, uptr Ptr) const {
     return static_cast<CompactPtrT>(Ptr);
   }
 
   void *decompactPtr(UNUSED uptr ClassId, CompactPtrT CompactPtr) const {
private:
   static_assert(sizeof(SizeClassInfo) % SCUDO_CACHE_LINE_SIZE == 0, "");
 
   uptr computeRegionId(uptr Mem) {
     const uptr Id = Mem >> Config::PrimaryRegionSizeLog;
     CHECK_LT(Id, NumRegions);
     return Id;
   }
 
-  uptr allocateRegionSlow() {
+  uptr allocateRegionSlow(MapPlatformData *MapData) {
     uptr MapSize = 2 * RegionSize;
     const uptr MapBase = reinterpret_cast<uptr>(
-        map(nullptr, MapSize, "scudo:primary", MAP_ALLOWNOMEM));
+        map(nullptr, MapSize, "scudo:primary", MAP_ALLOWNOMEM, MapData));
     if (!MapBase)
       return 0;
     const uptr MapEnd = MapBase + MapSize;
     uptr Region = MapBase;
     if (isAligned(Region, RegionSize)) {
       ScopedLock L(RegionsStashMutex);
-      if (NumberOfStashedRegions < MaxStashedRegions)
-        RegionsStash[NumberOfStashedRegions++] = MapBase + RegionSize;
-      else
+      if (NumberOfStashedRegions < MaxStashedRegions) {
+        RegionsStash[NumberOfStashedRegions] = MapBase + RegionSize;
+        StashMapData[NumberOfStashedRegions] = *MapData;
+        NumberOfStashedRegions += 1;
+      } else
         MapSize = RegionSize;
     } else {
       Region = roundUpTo(MapBase, RegionSize);
-      unmap(reinterpret_cast<void *>(MapBase), Region - MapBase);
+      unmap(reinterpret_cast<void *>(MapBase), Region - MapBase, 0, MapData);
       MapSize = RegionSize;
     }
     const uptr End = Region + MapSize;
     if (End != MapEnd)
-      unmap(reinterpret_cast<void *>(End), MapEnd - End);
+      unmap(reinterpret_cast<void *>(End), MapEnd - End, 0, MapData);
     return Region;
   }
 
   uptr allocateRegion(SizeClassInfo *Sci, uptr ClassId) {
     DCHECK_LT(ClassId, NumClasses);
+    MapPlatformData MapData;
     uptr Region = 0;
     {
       ScopedLock L(RegionsStashMutex);
-      if (NumberOfStashedRegions > 0)
-        Region = RegionsStash[--NumberOfStashedRegions];
+      if (NumberOfStashedRegions > 0) {
+        NumberOfStashedRegions -= 1;
+        Region = RegionsStash[NumberOfStashedRegions];
+        MapData = RegionsMapData[NumberOfStashedRegions];
+      }
+    }
+    if (!Region) {
+      MapData = {};
+      Region = allocateRegionSlow(&MapData);
     }
-    if (!Region)
-      Region = allocateRegionSlow();
     if (LIKELY(Region)) {
       // Sci->Mutex is held by the caller, updating the Min/Max is safe.
       const uptr RegionIndex = computeRegionId(Region);
       if (RegionIndex < Sci->MinRegionIndex)
         Sci->MinRegionIndex = RegionIndex;
       if (RegionIndex > Sci->MaxRegionIndex)
         Sci->MaxRegionIndex = RegionIndex;
       PossibleRegions.set(RegionIndex, static_cast<u8>(ClassId + 1U));
+      RegionsMapData[RegionIndex] = MapData;
     }
     return Region;
   }
 
   SizeClassInfo *getSizeClassInfo(uptr ClassId) {
     DCHECK_LT(ClassId, NumClasses);
     return &SizeClassInfoArray[ClassId];
   }
NOINLINE uptr releaseToOSMaybe(SizeClassInfo *Sci, uptr ClassId,
 
     const uptr First = Sci->MinRegionIndex;
     const uptr Last = Sci->MaxRegionIndex;
     DCHECK_NE(Last, 0U);
     DCHECK_LE(First, Last);
     uptr TotalReleasedBytes = 0;
     const uptr Base = First * RegionSize;
     const uptr NumberOfRegions = Last - First + 1U;
-    ReleaseRecorder Recorder(Base);
+    ReleaseRecorder Recorder(Base, RegionSize, &RegionsMapData[First],
+                             NumberOfRegions);
     auto SkipRegion = [this, First, ClassId](uptr RegionIndex) {
       return (PossibleRegions[First + RegionIndex] - 1U) != ClassId;
     };
     auto DecompactPtr = [](CompactPtrT CompactPtr) {
       return reinterpret_cast<uptr>(CompactPtr);
     };
     releaseFreeMemoryToOS(Sci->FreeList, RegionSize, NumberOfRegions, BlockSize,
                           &Recorder, DecompactPtr, SkipRegion);
private:
   ByteMap PossibleRegions = {};
   atomic_s32 ReleaseToOsIntervalMs = {};
   // Unless several threads request regions simultaneously from different size
   // classes, the stash rarely contains more than 1 entry.
   static constexpr uptr MaxStashedRegions = 4;
   HybridMutex RegionsStashMutex;
   uptr NumberOfStashedRegions = 0;
   uptr RegionsStash[MaxStashedRegions] = {};
+  [[no_unique_address]] MapPlatformData StashMapData[MaxStashedRegions] = {};
+  [[no_unique_address]] MapPlatformData RegionsMapData[NumRegions] = {};
 };
 
 } // namespace scudo
 
 #endif // SCUDO_PRIMARY32_H_

compiler-rt/lib/scudo/standalone/primary64.h

if (!Force) {
         return 0;
       if (Region->ReleaseInfo.LastReleaseAtNs +
               static_cast<u64>(IntervalMs) * 1000000 >
           getMonotonicTime()) {
         return 0; // Memory was returned recently.
       }
     }
 
-    ReleaseRecorder Recorder(Region->RegionBeg, &Region->Data);
+    ReleaseRecorder Recorder(Region->RegionBeg, RegionSize, &Region->Data, 1);
     const uptr CompactPtrBase = getCompactPtrBaseByClassId(ClassId);
     auto DecompactPtr = [CompactPtrBase](CompactPtrT CompactPtr) {
       return decompactPtrInternal(CompactPtrBase, CompactPtr);
     };
     auto SkipRegion = [](UNUSED uptr RegionIndex) { return false; };
     releaseFreeMemoryToOS(Region->FreeList, Region->AllocatedUser, 1U,
                           BlockSize, &Recorder, DecompactPtr, SkipRegion);
 

compiler-rt/lib/scudo/standalone/release.h

 //===-- release.h -----------------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef SCUDO_RELEASE_H_
 #define SCUDO_RELEASE_H_
 
-#include "common.h"
-#include "list.h"
-#include "mutex.h"
+#include "internal/scudo/common.h"
+#include "internal/scudo/list.h"
+#include "internal/scudo/mutex.h"
 
 namespace scudo {
 
 class ReleaseRecorder {
 public:
-  ReleaseRecorder(uptr Base, MapPlatformData *Data = nullptr)
-      : Base(Base), Data(Data) {}
+  ReleaseRecorder(uptr _Base, uptr _RegionSize, MapPlatformData *_Data,
+                  uptr _DataSize)
+      : Base(_Base), RegionSize(_RegionSize), Data(_Data), DataSize(_DataSize) {
+  }
 
   uptr getReleasedRangesCount() const { return ReleasedRangesCount; }
 
   uptr getReleasedBytes() const { return ReleasedBytes; }
 
   uptr getBase() const { return Base; }
 
   // Releases [From, To) range of pages back to OS.
   void releasePageRangeToOS(uptr From, uptr To) {
     const uptr Size = To - From;
-    releasePagesToOS(Base, From, Size, Data);
+    const uptr Idx = From / RegionSize;
+    CHECK_LT(Idx, DataSize);
+    releasePagesToOS(Base, From, Size, &Data[Idx]);
     ReleasedRangesCount++;
     ReleasedBytes += Size;
   }
 
 private:
   uptr ReleasedRangesCount = 0;
   uptr ReleasedBytes = 0;
   uptr Base = 0;
+  uptr RegionSize = 0;
   MapPlatformData *Data = nullptr;
+  uptr DataSize = 0;
 };
 
 // A packed array of Counters. Each counter occupies 2^N bits, enough to store
 // counter's MaxValue. Ctor will try to use a static buffer first, and if that
 // fails (the buffer is too small or already locked), will allocate the
 // required Buffer via map(). The caller is expected to check whether the
 // initialization was successful by checking isAllocated() result. For
 // performance sake, none of the accessors check the validity of the arguments,