[compiler-rt] [scudo] Make BufferPool typed and always express sizes in terms of element count (PR #66896)
Fabio D'Urso via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 26 02:25:57 PDT 2023
https://github.com/fabio-d updated https://github.com/llvm/llvm-project/pull/66896
>From a032878a9e7c71057567d9b667f358203eb9bbe8 Mon Sep 17 00:00:00 2001
From: Fabio D'Urso <fdurso at google.com>
Date: Tue, 19 Sep 2023 17:48:24 +0200
Subject: [PATCH 1/3] [scudo] Make BufferPool typed and always express sizes in
terms of element count
This fixes the issue that resulted in getBuffer interpreting its
argument as a number of elements and getDynamicBuffer interpreting it
as a number of bytes.
---
compiler-rt/lib/scudo/standalone/release.cpp | 3 +-
compiler-rt/lib/scudo/standalone/release.h | 82 ++++++++++---------
.../scudo/standalone/tests/release_test.cpp | 31 +++----
3 files changed, 63 insertions(+), 53 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/release.cpp b/compiler-rt/lib/scudo/standalone/release.cpp
index 938bb41faf69561..87dbc55e40df927 100644
--- a/compiler-rt/lib/scudo/standalone/release.cpp
+++ b/compiler-rt/lib/scudo/standalone/release.cpp
@@ -10,7 +10,8 @@
namespace scudo {
-BufferPool<RegionPageMap::StaticBufferCount, RegionPageMap::StaticBufferSize>
+BufferPool<uptr, RegionPageMap::StaticBufferCount,
+ RegionPageMap::StaticBufferNumElements>
RegionPageMap::Buffers;
} // namespace scudo
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index e138570e7993336..98f59535e282783 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/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 `T` 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 <typename T, 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(T),
+ 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.
+ T *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(T));
return &RawBuffer[Offset];
}
- void releaseBuffer(uptr *Buffer, const uptr BufferSize) {
- const uptr index = getStaticBufferIndex(Buffer, BufferSize);
+ void releaseBuffer(T *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(T), getPageSizeCached());
+ unmap(reinterpret_cast<void *>(Buffer), MappedSize);
}
}
- bool isStaticBufferTestOnly(uptr *Buffer, uptr BufferSize) {
- return getStaticBufferIndex(Buffer, BufferSize) < StaticBufferCount;
+ bool isStaticBufferTestOnly(T *Buffer, uptr NumElements) {
+ return getStaticBufferIndex(Buffer, NumElements) < StaticBufferCount;
}
private:
- uptr getStaticBufferIndex(uptr *Buffer, uptr BufferSize) {
- if (UNLIKELY(BufferSize > StaticBufferSize))
+ uptr getStaticBufferIndex(T *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(T),
+ RawBufferBase + sizeof(RawBuffer));
- const uptr index =
- (BufferBase - RawBufferBase) / (StaticBufferSize * sizeof(uptr));
+ const uptr StaticBufferSize = StaticBufferNumElements * sizeof(T);
+ DCHECK_EQ((BufferBase - RawBufferBase) % StaticBufferSize, 0U);
+ const uptr index = (BufferBase - RawBufferBase) / StaticBufferSize;
DCHECK_LT(index, StaticBufferCount);
return index;
}
- uptr *getDynamicBuffer(const uptr BufferSize) {
+ T *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);
- return reinterpret_cast<uptr *>(
- map(nullptr, roundUp(BufferSize, getPageSizeCached()), "scudo:counters",
- MmapFlags, &MapData));
+ const uptr MappedSize =
+ roundUp(NumElements * sizeof(T), getPageSizeCached());
+ return reinterpret_cast<T *>(
+ 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);
+ T RawBuffer[StaticBufferCount * StaticBufferNumElements] GUARDED_BY(Mutex);
[[no_unique_address]] MapPlatformData MapData = {};
};
@@ -207,7 +215,7 @@ class RegionPageMap {
PackingRatioLog(0),
BitOffsetMask(0),
SizePerRegion(0),
- BufferSize(0),
+ BufferNumElements(0),
Buffer(nullptr) {}
RegionPageMap(uptr NumberOfRegions, uptr CountersPerRegion, uptr MaxValue) {
reset(NumberOfRegions, CountersPerRegion, MaxValue);
@@ -215,7 +223,7 @@ class RegionPageMap {
~RegionPageMap() {
if (!isAllocated())
return;
- Buffers.releaseBuffer(Buffer, BufferSize);
+ Buffers.releaseBuffer(Buffer, BufferNumElements);
Buffer = nullptr;
}
@@ -248,8 +256,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 +332,7 @@ class RegionPageMap {
return get(Region, I) == CounterMask;
}
- uptr getBufferSize() const { return BufferSize; }
+ uptr getBufferNumElements() const { return BufferNumElements; }
private:
uptr Regions;
@@ -335,14 +343,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<uptr, StaticBufferCount, StaticBufferNumElements> Buffers;
};
template <class ReleaseRecorderT> class FreePagesRangeTracker {
diff --git a/compiler-rt/lib/scudo/standalone/tests/release_test.cpp b/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
index 0ab49460807f297..11f1bb1e37875ef 100644
--- a/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
+++ b/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<scudo::uptr, 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);
}
>From 13490d0600f9ed4f23d70a4ff19854d432781ad6 Mon Sep 17 00:00:00 2001
From: Fabio D'Urso <fdurso at google.com>
Date: Tue, 26 Sep 2023 10:45:06 +0200
Subject: [PATCH 2/3] Hardcode `uptr` instead of taking the type as a template
parameter
---
compiler-rt/lib/scudo/standalone/release.cpp | 2 +-
compiler-rt/lib/scudo/standalone/release.h | 32 +++++++++----------
.../scudo/standalone/tests/release_test.cpp | 4 +--
3 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/release.cpp b/compiler-rt/lib/scudo/standalone/release.cpp
index 87dbc55e40df927..875a2b0c1c572e6 100644
--- a/compiler-rt/lib/scudo/standalone/release.cpp
+++ b/compiler-rt/lib/scudo/standalone/release.cpp
@@ -10,7 +10,7 @@
namespace scudo {
-BufferPool<uptr, RegionPageMap::StaticBufferCount,
+BufferPool<RegionPageMap::StaticBufferCount,
RegionPageMap::StaticBufferNumElements>
RegionPageMap::Buffers;
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index 98f59535e282783..efe3d03fc04601f 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -95,23 +95,23 @@ class FragmentationRecorder {
uptr ReleasedPagesCount = 0;
};
-// A buffer pool which holds a fixed number of static buffers of `T` elements
+// 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 <typename T, uptr StaticBufferCount, uptr StaticBufferNumElements>
+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(StaticBufferNumElements * sizeof(T),
+ static_assert(isAligned(StaticBufferNumElements * sizeof(uptr),
SCUDO_CACHE_LINE_SIZE),
"");
// Return a zero-initialized buffer which can contain at least the given
// number of elements, or nullptr on failure.
- T *getBuffer(const uptr NumElements) {
+ uptr *getBuffer(const uptr NumElements) {
if (UNLIKELY(NumElements > StaticBufferNumElements))
return getDynamicBuffer(NumElements);
@@ -131,11 +131,11 @@ class BufferPool {
return getDynamicBuffer(NumElements);
const uptr Offset = index * StaticBufferNumElements;
- memset(&RawBuffer[Offset], 0, StaticBufferNumElements * sizeof(T));
+ memset(&RawBuffer[Offset], 0, StaticBufferNumElements * sizeof(uptr));
return &RawBuffer[Offset];
}
- void releaseBuffer(T *Buffer, const uptr NumElements) {
+ void releaseBuffer(uptr *Buffer, const uptr NumElements) {
const uptr index = getStaticBufferIndex(Buffer, NumElements);
if (index < StaticBufferCount) {
ScopedLock L(Mutex);
@@ -143,17 +143,17 @@ class BufferPool {
Mask |= static_cast<uptr>(1) << index;
} else {
const uptr MappedSize =
- roundUp(NumElements * sizeof(T), getPageSizeCached());
+ roundUp(NumElements * sizeof(uptr), getPageSizeCached());
unmap(reinterpret_cast<void *>(Buffer), MappedSize);
}
}
- bool isStaticBufferTestOnly(T *Buffer, uptr NumElements) {
+ bool isStaticBufferTestOnly(uptr *Buffer, uptr NumElements) {
return getStaticBufferIndex(Buffer, NumElements) < StaticBufferCount;
}
private:
- uptr getStaticBufferIndex(T *Buffer, uptr NumElements) {
+ uptr getStaticBufferIndex(uptr *Buffer, uptr NumElements) {
if (UNLIKELY(NumElements > StaticBufferNumElements))
return StaticBufferCount;
@@ -166,17 +166,17 @@ class BufferPool {
}
DCHECK_LE(NumElements, StaticBufferNumElements);
- DCHECK_LE(BufferBase + NumElements * sizeof(T),
+ DCHECK_LE(BufferBase + NumElements * sizeof(uptr),
RawBufferBase + sizeof(RawBuffer));
- const uptr StaticBufferSize = StaticBufferNumElements * sizeof(T);
+ const uptr StaticBufferSize = StaticBufferNumElements * sizeof(uptr);
DCHECK_EQ((BufferBase - RawBufferBase) % StaticBufferSize, 0U);
const uptr index = (BufferBase - RawBufferBase) / StaticBufferSize;
DCHECK_LT(index, StaticBufferCount);
return index;
}
- T *getDynamicBuffer(const uptr NumElements) {
+ 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
@@ -184,15 +184,15 @@ class BufferPool {
// performance benefit on other platforms.
const uptr MmapFlags = MAP_ALLOWNOMEM | (SCUDO_FUCHSIA ? MAP_PRECOMMIT : 0);
const uptr MappedSize =
- roundUp(NumElements * sizeof(T), getPageSizeCached());
- return reinterpret_cast<T *>(
+ roundUp(NumElements * sizeof(uptr), getPageSizeCached());
+ return reinterpret_cast<uptr *>(
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);
- T RawBuffer[StaticBufferCount * StaticBufferNumElements] GUARDED_BY(Mutex);
+ uptr RawBuffer[StaticBufferCount * StaticBufferNumElements] GUARDED_BY(Mutex);
[[no_unique_address]] MapPlatformData MapData = {};
};
@@ -350,7 +350,7 @@ class RegionPageMap {
// benefit from this.
static const uptr StaticBufferCount = 2U;
static const uptr StaticBufferNumElements = 512U;
- static BufferPool<uptr, StaticBufferCount, StaticBufferNumElements> Buffers;
+ static BufferPool<StaticBufferCount, StaticBufferNumElements> Buffers;
};
template <class ReleaseRecorderT> class FreePagesRangeTracker {
diff --git a/compiler-rt/lib/scudo/standalone/tests/release_test.cpp b/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
index 11f1bb1e37875ef..948eb5a8dc2fd68 100644
--- a/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
@@ -633,8 +633,8 @@ TEST(ScudoReleaseTest, BufferPool) {
// Allocate the buffer pool on the heap because it is quite large (slightly
// more than StaticBufferCount * StaticBufferNumElements * sizeof(uptr)) and
// it may not fit in the stack on some platforms.
- using BufferPool = scudo::BufferPool<scudo::uptr, StaticBufferCount,
- StaticBufferNumElements>;
+ using BufferPool =
+ scudo::BufferPool<StaticBufferCount, StaticBufferNumElements>;
std::unique_ptr<BufferPool> Pool(new BufferPool());
std::vector<std::pair<scudo::uptr *, scudo::uptr>> Buffers;
>From bbfedeb33801dc78b121f79c629137fdfc49867c Mon Sep 17 00:00:00 2001
From: Fabio D'Urso <fdurso at google.com>
Date: Tue, 26 Sep 2023 11:23:11 +0200
Subject: [PATCH 3/3] Reformat RegionPageMap's ctor to make the clang-format
bot happy
---
compiler-rt/lib/scudo/standalone/release.h | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index efe3d03fc04601f..89ca884b4de45be 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -208,15 +208,9 @@ class BufferPool {
class RegionPageMap {
public:
RegionPageMap()
- : Regions(0),
- NumCounters(0),
- CounterSizeBitsLog(0),
- CounterMask(0),
- PackingRatioLog(0),
- BitOffsetMask(0),
- SizePerRegion(0),
- BufferNumElements(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);
}
More information about the llvm-commits
mailing list