[scudo] Make BufferPool typed and always express sizes in terms of element count

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

@@ -10,7 +10,8 @@
 
 namespace scudo {
 
-BufferPool<RegionPageMap::StaticBufferCount, RegionPageMap::StaticBufferSize>
+BufferPool<RegionPageMap::StaticBufferCount,
+           RegionPageMap::StaticBufferNumElements>
     RegionPageMap::Buffers;
 
 } // namespace scudo

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

@@ -95,20 +95,25 @@ class FragmentationRecorder {
   uptr ReleasedPagesCount = 0;
 };
 
-// A buffer pool which holds a fixed number of static buffers for fast buffer
-// allocation. If the request size is greater than `StaticBufferSize`, it'll
+// A buffer pool which holds a fixed number of static buffers of `uptr` elements
+// for fast buffer allocation. If the request size is greater than
+// `StaticBufferNumElements` or if all the static buffers are in use, it'll
 // delegate the allocation to map().
-template <uptr StaticBufferCount, uptr StaticBufferSize> class BufferPool {
+template <uptr StaticBufferCount, uptr StaticBufferNumElements>
+class BufferPool {
 public:
   // Preserve 1 bit in the `Mask` so that we don't need to do zero-check while
   // extracting the least significant bit from the `Mask`.
   static_assert(StaticBufferCount < SCUDO_WORDSIZE, "");
-  static_assert(isAligned(StaticBufferSize, SCUDO_CACHE_LINE_SIZE), "");
+  static_assert(isAligned(StaticBufferNumElements * sizeof(uptr),
+                          SCUDO_CACHE_LINE_SIZE),
+                "");
 
-  // Return a buffer which is at least `BufferSize`.
-  uptr *getBuffer(const uptr BufferSize) {
-    if (UNLIKELY(BufferSize > StaticBufferSize))
-      return getDynamicBuffer(BufferSize);
+  // Return a zero-initialized buffer which can contain at least the given
+  // number of elements, or nullptr on failure.
+  uptr *getBuffer(const uptr NumElements) {
+    if (UNLIKELY(NumElements > StaticBufferNumElements))
+      return getDynamicBuffer(NumElements);
 
     uptr index;
     {
@@ -123,32 +128,33 @@ template <uptr StaticBufferCount, uptr StaticBufferSize> class BufferPool {
     }
 
     if (index >= StaticBufferCount)
-      return getDynamicBuffer(BufferSize);
+      return getDynamicBuffer(NumElements);
 
-    const uptr Offset = index * StaticBufferSize;
-    memset(&RawBuffer[Offset], 0, StaticBufferSize);
+    const uptr Offset = index * StaticBufferNumElements;
+    memset(&RawBuffer[Offset], 0, StaticBufferNumElements * sizeof(uptr));
     return &RawBuffer[Offset];
   }
 
-  void releaseBuffer(uptr *Buffer, const uptr BufferSize) {
-    const uptr index = getStaticBufferIndex(Buffer, BufferSize);
+  void releaseBuffer(uptr *Buffer, const uptr NumElements) {
+    const uptr index = getStaticBufferIndex(Buffer, NumElements);
     if (index < StaticBufferCount) {
       ScopedLock L(Mutex);
       DCHECK_EQ((Mask & (static_cast<uptr>(1) << index)), 0U);
       Mask |= static_cast<uptr>(1) << index;
     } else {
-      unmap(reinterpret_cast<void *>(Buffer),
-            roundUp(BufferSize, getPageSizeCached()));
+      const uptr MappedSize =
+          roundUp(NumElements * sizeof(uptr), getPageSizeCached());
+      unmap(reinterpret_cast<void *>(Buffer), MappedSize);
     }
   }
 
-  bool isStaticBufferTestOnly(uptr *Buffer, uptr BufferSize) {
-    return getStaticBufferIndex(Buffer, BufferSize) < StaticBufferCount;
+  bool isStaticBufferTestOnly(uptr *Buffer, uptr NumElements) {
+    return getStaticBufferIndex(Buffer, NumElements) < StaticBufferCount;
   }
 
 private:
-  uptr getStaticBufferIndex(uptr *Buffer, uptr BufferSize) {
-    if (UNLIKELY(BufferSize > StaticBufferSize))
+  uptr getStaticBufferIndex(uptr *Buffer, uptr NumElements) {
+    if (UNLIKELY(NumElements > StaticBufferNumElements))
       return StaticBufferCount;
 
     const uptr BufferBase = reinterpret_cast<uptr>(Buffer);
@@ -159,32 +165,34 @@ template <uptr StaticBufferCount, uptr StaticBufferSize> class BufferPool {
       return StaticBufferCount;
     }
 
-    DCHECK_LE(BufferSize, StaticBufferSize);
-    DCHECK_LE(BufferBase + BufferSize, RawBufferBase + sizeof(RawBuffer));
-    DCHECK_EQ((BufferBase - RawBufferBase) % StaticBufferSize, 0U);
+    DCHECK_LE(NumElements, StaticBufferNumElements);
+    DCHECK_LE(BufferBase + NumElements * sizeof(uptr),
+              RawBufferBase + sizeof(RawBuffer));
 
-    const uptr index =
-        (BufferBase - RawBufferBase) / (StaticBufferSize * sizeof(uptr));
+    const uptr StaticBufferSize = StaticBufferNumElements * sizeof(uptr);
+    DCHECK_EQ((BufferBase - RawBufferBase) % StaticBufferSize, 0U);
+    const uptr index = (BufferBase - RawBufferBase) / StaticBufferSize;
     DCHECK_LT(index, StaticBufferCount);
     return index;
   }
 
-  uptr *getDynamicBuffer(const uptr BufferSize) {
+  uptr *getDynamicBuffer(const uptr NumElements) {
     // When using a heap-based buffer, precommit the pages backing the
     // Vmar by passing |MAP_PRECOMMIT| flag. This allows an optimization
     // where page fault exceptions are skipped as the allocated memory
     // is accessed. So far, this is only enabled on Fuchsia. It hasn't proven a
     // performance benefit on other platforms.
     const uptr MmapFlags = MAP_ALLOWNOMEM | (SCUDO_FUCHSIA ? MAP_PRECOMMIT : 0);
+    const uptr MappedSize =
+        roundUp(NumElements * sizeof(uptr), getPageSizeCached());
     return reinterpret_cast<uptr *>(
-        map(nullptr, roundUp(BufferSize, getPageSizeCached()), "scudo:counters",
-            MmapFlags, &MapData));
+        map(nullptr, MappedSize, "scudo:counters", MmapFlags, &MapData));
   }
 
   HybridMutex Mutex;
   // '1' means that buffer index is not used. '0' means the buffer is in use.
   uptr Mask GUARDED_BY(Mutex) = ~static_cast<uptr>(0);
-  uptr RawBuffer[StaticBufferCount * StaticBufferSize] GUARDED_BY(Mutex);
+  uptr RawBuffer[StaticBufferCount * StaticBufferNumElements] GUARDED_BY(Mutex);
   [[no_unique_address]] MapPlatformData MapData = {};
 };
 
@@ -200,22 +208,16 @@ template <uptr StaticBufferCount, uptr StaticBufferSize> class BufferPool {
 class RegionPageMap {
 public:
   RegionPageMap()
-      : Regions(0),
-        NumCounters(0),
-        CounterSizeBitsLog(0),
-        CounterMask(0),
-        PackingRatioLog(0),
-        BitOffsetMask(0),
-        SizePerRegion(0),
-        BufferSize(0),
-        Buffer(nullptr) {}
+      : Regions(0), NumCounters(0), CounterSizeBitsLog(0), CounterMask(0),
+        PackingRatioLog(0), BitOffsetMask(0), SizePerRegion(0),
+        BufferNumElements(0), Buffer(nullptr) {}
   RegionPageMap(uptr NumberOfRegions, uptr CountersPerRegion, uptr MaxValue) {
     reset(NumberOfRegions, CountersPerRegion, MaxValue);
   }
   ~RegionPageMap() {
     if (!isAllocated())
       return;
-    Buffers.releaseBuffer(Buffer, BufferSize);
+    Buffers.releaseBuffer(Buffer, BufferNumElements);
     Buffer = nullptr;
   }
 
@@ -248,8 +250,8 @@ class RegionPageMap {
     SizePerRegion =
         roundUp(NumCounters, static_cast<uptr>(1U) << PackingRatioLog) >>
         PackingRatioLog;
-    BufferSize = SizePerRegion * sizeof(*Buffer) * Regions;
-    Buffer = Buffers.getBuffer(BufferSize);
+    BufferNumElements = SizePerRegion * Regions;
+    Buffer = Buffers.getBuffer(BufferNumElements);
   }
 
   bool isAllocated() const { return !!Buffer; }
@@ -324,7 +326,7 @@ class RegionPageMap {
     return get(Region, I) == CounterMask;
   }
 
-  uptr getBufferSize() const { return BufferSize; }
+  uptr getBufferNumElements() const { return BufferNumElements; }
 
 private:
   uptr Regions;
@@ -335,14 +337,14 @@ class RegionPageMap {
   uptr BitOffsetMask;
 
   uptr SizePerRegion;
-  uptr BufferSize;
+  uptr BufferNumElements;
   uptr *Buffer;
 
   // We may consider making this configurable if there are cases which may
   // benefit from this.
   static const uptr StaticBufferCount = 2U;
-  static const uptr StaticBufferSize = 512U;
-  static BufferPool<StaticBufferCount, StaticBufferSize> Buffers;
+  static const uptr StaticBufferNumElements = 512U;
+  static BufferPool<StaticBufferCount, StaticBufferNumElements> Buffers;
 };
 
 template <class ReleaseRecorderT> class FreePagesRangeTracker {

compiler-rt/lib/scudo/standalone/tests/release_test.cpp

@@ -23,17 +23,16 @@ TEST(ScudoReleaseTest, RegionPageMap) {
     // Various valid counter's max values packed into one word.
     scudo::RegionPageMap PageMap2N(1U, 1U, 1UL << I);
     ASSERT_TRUE(PageMap2N.isAllocated());
-    EXPECT_EQ(sizeof(scudo::uptr), PageMap2N.getBufferSize());
+    EXPECT_EQ(1, PageMap2N.getBufferNumElements());
     // Check the "all bit set" values too.
     scudo::RegionPageMap PageMap2N1_1(1U, 1U, ~0UL >> I);
     ASSERT_TRUE(PageMap2N1_1.isAllocated());
-    EXPECT_EQ(sizeof(scudo::uptr), PageMap2N1_1.getBufferSize());
+    EXPECT_EQ(1, PageMap2N1_1.getBufferNumElements());
     // Verify the packing ratio, the counter is Expected to be packed into the
     // closest power of 2 bits.
     scudo::RegionPageMap PageMap(1U, SCUDO_WORDSIZE, 1UL << I);
     ASSERT_TRUE(PageMap.isAllocated());
-    EXPECT_EQ(sizeof(scudo::uptr) * scudo::roundUpPowerOfTwo(I + 1),
-              PageMap.getBufferSize());
+    EXPECT_EQ(scudo::roundUpPowerOfTwo(I + 1), PageMap.getBufferNumElements());
   }
 
   // Go through 1, 2, 4, 8, .. {32,64} bits per counter.
@@ -533,7 +532,8 @@ template <class SizeClassMap> void testReleasePartialRegion() {
                                         ReleaseBase);
       Partial.ensurePageMapAllocated();
 
-      EXPECT_GE(Full.PageMap.getBufferSize(), Partial.PageMap.getBufferSize());
+      EXPECT_GE(Full.PageMap.getBufferNumElements(),
+                Partial.PageMap.getBufferNumElements());
     }
 
     while (!FreeList.empty()) {
@@ -628,26 +628,27 @@ TEST(ScudoReleaseTest, RangeReleaseRegionWithSingleBlock) {
 
 TEST(ScudoReleaseTest, BufferPool) {
   constexpr scudo::uptr StaticBufferCount = SCUDO_WORDSIZE - 1;
-  constexpr scudo::uptr StaticBufferSize = 512U;
+  constexpr scudo::uptr StaticBufferNumElements = 512U;
 
   // Allocate the buffer pool on the heap because it is quite large (slightly
-  // more than StaticBufferCount * StaticBufferSize * sizeof(uptr)) and it may
-  // not fit in the stack on some platforms.
-  using BufferPool = scudo::BufferPool<StaticBufferCount, StaticBufferSize>;
+  // more than StaticBufferCount * StaticBufferNumElements * sizeof(uptr)) and
+  // it may not fit in the stack on some platforms.
+  using BufferPool =
+      scudo::BufferPool<StaticBufferCount, StaticBufferNumElements>;
   std::unique_ptr<BufferPool> Pool(new BufferPool());
 
   std::vector<std::pair<scudo::uptr *, scudo::uptr>> Buffers;
   for (scudo::uptr I = 0; I < StaticBufferCount; ++I) {
-    scudo::uptr *P = Pool->getBuffer(StaticBufferSize);
-    EXPECT_TRUE(Pool->isStaticBufferTestOnly(P, StaticBufferSize));
-    Buffers.emplace_back(P, StaticBufferSize);
+    scudo::uptr *P = Pool->getBuffer(StaticBufferNumElements);
+    EXPECT_TRUE(Pool->isStaticBufferTestOnly(P, StaticBufferNumElements));
+    Buffers.emplace_back(P, StaticBufferNumElements);
   }
 
   // The static buffer is supposed to be used up.
-  scudo::uptr *P = Pool->getBuffer(StaticBufferSize);
-  EXPECT_FALSE(Pool->isStaticBufferTestOnly(P, StaticBufferSize));
+  scudo::uptr *P = Pool->getBuffer(StaticBufferNumElements);
+  EXPECT_FALSE(Pool->isStaticBufferTestOnly(P, StaticBufferNumElements));
 
-  Pool->releaseBuffer(P, StaticBufferSize);
+  Pool->releaseBuffer(P, StaticBufferNumElements);
   for (auto &Buffer : Buffers)
     Pool->releaseBuffer(Buffer.first, Buffer.second);
 }