[compiler-rt] dc7e9b1 - [scudo] Refactor BufferPool code slightly. (#222811)
via llvm-commits
llvm-commits at lists.llvm.org
Fri Sep 11 12:03:41 PDT 2026
Author: Christopher Ferris
Date: 2026-09-11T12:03:36-07:00
New Revision: dc7e9b16f229adbad90bafe1bd5685794ffb4710
URL: https://github.com/llvm/llvm-project/commit/dc7e9b16f229adbad90bafe1bd5685794ffb4710
DIFF: https://github.com/llvm/llvm-project/commit/dc7e9b16f229adbad90bafe1bd5685794ffb4710.diff
LOG: [scudo] Refactor BufferPool code slightly. (#222811)
Pass the Buffer object in buffer creation functions. Add an error
message if the dynamic buffer creation fails.
Add a threaded unit test to verify the BufferPool handles concurrent
buffer creation properly.
Also, fix a few typos in the code.
Added:
Modified:
compiler-rt/lib/scudo/standalone/release.h
compiler-rt/lib/scudo/standalone/tests/release_test.cpp
Removed:
################################################################################
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index 4171314011965..6a0e2a4f579e0 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -13,6 +13,7 @@
#include "list.h"
#include "mem_map.h"
#include "mutex.h"
+#include "string_utils.h"
#include "thread_annotations.h"
namespace scudo {
@@ -130,35 +131,36 @@ class BufferPool {
MemMapT MemMap = {};
};
- // Return a zero-initialized buffer which can contain at least the given
- // number of elements, or nullptr on failure.
- Buffer getBuffer(const uptr NumElements) {
+ // Buf must be an empty buffer that will be filled in to contain a zero
+ // initialized buffer which can contain the given number of elements.
+ // On failure, Buf.data is guaranteed to be nullptr, and returns false.
+ bool getBuffer(Buffer &Buf, const uptr NumElements) {
+ DCHECK(Buf.Data == nullptr);
if (UNLIKELY(NumElements > StaticBufferNumElements))
- return getDynamicBuffer(NumElements);
+ return getDynamicBuffer(Buf, NumElements);
- uptr index;
+ uptr Index;
{
// TODO: In general, we expect this operation should be fast so the
// waiting thread won't be put into sleep. The HybridMutex does implement
// the busy-waiting but we may want to review the performance and see if
// we need an explict spin lock here.
ScopedLock L(Mutex);
- index = getLeastSignificantSetBitIndex(Mask);
- if (index < StaticBufferCount)
- Mask ^= static_cast<uptr>(1) << index;
+ Index = getLeastSignificantSetBitIndex(Mask);
+ if (Index < StaticBufferCount)
+ Mask ^= static_cast<uptr>(1) << Index;
}
- if (index >= StaticBufferCount)
- return getDynamicBuffer(NumElements);
+ if (Index >= StaticBufferCount)
+ return getDynamicBuffer(Buf, NumElements);
- Buffer Buf;
- Buf.Data = &RawBuffer[index * StaticBufferNumElements];
- Buf.BufferIndex = index;
+ Buf.Data = &RawBuffer[Index * StaticBufferNumElements];
+ Buf.BufferIndex = Index;
memset(Buf.Data, 0, StaticBufferNumElements * sizeof(uptr));
- return Buf;
+ return true;
}
- void releaseBuffer(Buffer Buf) {
+ void releaseBuffer(Buffer &Buf) {
DCHECK_NE(Buf.Data, nullptr);
DCHECK_LE(Buf.BufferIndex, StaticBufferCount);
if (Buf.BufferIndex != StaticBufferCount) {
@@ -168,6 +170,7 @@ class BufferPool {
} else {
Buf.MemMap.unmap();
}
+ Buf.Data = nullptr;
}
bool isStaticBufferTestOnly(const Buffer &Buf) {
@@ -177,7 +180,7 @@ class BufferPool {
}
private:
- Buffer getDynamicBuffer(const uptr NumElements) {
+ bool getDynamicBuffer(Buffer &Buf, 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
@@ -186,12 +189,15 @@ class BufferPool {
const uptr MmapFlags = MAP_ALLOWNOMEM | (SCUDO_FUCHSIA ? MAP_PRECOMMIT : 0);
const uptr MappedSize =
roundUp(NumElements * sizeof(uptr), getPageSizeCached());
- Buffer Buf;
- if (Buf.MemMap.map(/*Addr=*/0, MappedSize, "scudo:counters", MmapFlags)) {
- Buf.Data = reinterpret_cast<uptr *>(Buf.MemMap.getBase());
- Buf.BufferIndex = StaticBufferCount;
+ if (!UNLIKELY(Buf.MemMap.map(/*Addr=*/0, MappedSize, "scudo:counters",
+ MmapFlags))) {
+ return false;
}
- return Buf;
+
+ DCHECK(Buf.Data == nullptr);
+ Buf.Data = reinterpret_cast<uptr *>(Buf.MemMap.getBase());
+ Buf.BufferIndex = StaticBufferCount;
+ return true;
}
HybridMutex Mutex;
@@ -222,19 +228,18 @@ class RegionPageMap {
if (!isAllocated())
return;
Buffers.releaseBuffer(Buffer);
- Buffer = {};
}
// Lock of `StaticBuffer` is acquired conditionally and there's no easy way to
// specify the thread-safety attribute properly in current code structure.
// Besides, it's the only place we may want to check thread safety. Therefore,
// it's fine to bypass the thread-safety analysis now.
- void reset(uptr NumberOfRegion, uptr CountersPerRegion, uptr MaxValue) {
- DCHECK_GT(NumberOfRegion, 0);
+ void reset(uptr NumberOfRegions, uptr CountersPerRegion, uptr MaxValue) {
+ DCHECK_GT(NumberOfRegions, 0);
DCHECK_GT(CountersPerRegion, 0);
DCHECK_GT(MaxValue, 0);
- Regions = NumberOfRegion;
+ Regions = NumberOfRegions;
NumCounters = CountersPerRegion;
constexpr uptr MaxCounterBits = sizeof(*Buffer.Data) * 8UL;
@@ -255,7 +260,10 @@ class RegionPageMap {
roundUp(NumCounters, static_cast<uptr>(1U) << PackingRatioLog) >>
PackingRatioLog;
BufferNumElements = SizePerRegion * Regions;
- Buffer = Buffers.getBuffer(BufferNumElements);
+ if (!Buffers.getBuffer(Buffer, BufferNumElements)) {
+ DCHECK(Buffer.Data == nullptr);
+ Printf("Scudo WARNING: unable to allocate buffer for RegionPageMap");
+ }
}
bool isAllocated() const { return Buffer.Data != nullptr; }
@@ -449,7 +457,6 @@ struct PageReleaseContext {
if (PageMap.isAllocated())
return true;
PageMap.reset(NumberOfRegions, PagesCount, FullPagesBlockCountMax);
- // TODO: Log some message when we fail on PageMap allocation.
return PageMap.isAllocated();
}
diff --git a/compiler-rt/lib/scudo/standalone/tests/release_test.cpp b/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
index 14b398a91fca4..4eff70148b88e 100644
--- a/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/release_test.cpp
@@ -15,8 +15,12 @@
#include <string.h>
#include <algorithm>
+#include <atomic>
+#include <memory>
#include <random>
#include <set>
+#include <thread>
+#include <vector>
TEST(ScudoReleaseTest, RegionPageMap) {
for (scudo::uptr I = 0; I < SCUDO_WORDSIZE; I++) {
@@ -639,16 +643,80 @@ TEST(ScudoReleaseTest, BufferPool) {
std::vector<BufferPool::Buffer> Buffers;
for (scudo::uptr I = 0; I < StaticBufferCount; ++I) {
- BufferPool::Buffer Buffer = Pool->getBuffer(StaticBufferNumElements);
+ BufferPool::Buffer Buffer;
+ EXPECT_TRUE(Pool->getBuffer(Buffer, StaticBufferNumElements));
EXPECT_TRUE(Pool->isStaticBufferTestOnly(Buffer));
Buffers.push_back(Buffer);
}
// The static buffer is supposed to be used up.
- BufferPool::Buffer Buffer = Pool->getBuffer(StaticBufferNumElements);
+ BufferPool::Buffer Buffer;
+ EXPECT_TRUE(Pool->getBuffer(Buffer, StaticBufferNumElements));
EXPECT_FALSE(Pool->isStaticBufferTestOnly(Buffer));
Pool->releaseBuffer(Buffer);
- for (auto &Buffer : Buffers)
+ EXPECT_EQ(Buffer.Data, nullptr);
+ for (auto &Buffer : Buffers) {
Pool->releaseBuffer(Buffer);
+ EXPECT_EQ(Buffer.Data, nullptr);
+ }
+}
+
+TEST(ScudoReleaseTest, BufferPoolMultiThreaded) {
+ constexpr scudo::uptr StaticBufferCount = SCUDO_WORDSIZE - 1;
+ constexpr scudo::uptr StaticBufferNumElements = 512U;
+
+ using BufferPool =
+ scudo::BufferPool<StaticBufferCount, StaticBufferNumElements>;
+ std::unique_ptr<BufferPool> Pool(new BufferPool());
+
+ std::atomic<bool> Ready{false};
+
+ // Create a bunch of threads that will allocate from the pool all at once.
+ constexpr scudo::uptr NumThreads = 12;
+ constexpr scudo::uptr NumBuffers = 10;
+ constexpr scudo::uptr NumLoops = 5;
+ std::thread Threads[NumThreads];
+ for (scudo::uptr I = 0; I < ARRAY_SIZE(Threads); ++I) {
+ Threads[I] = std::thread([&Ready, &Pool, I]() {
+ while (!Ready)
+ ;
+
+ for (scudo::uptr Loop = 0; Loop < NumLoops; ++Loop) {
+ std::vector<BufferPool::Buffer> Buffers;
+ for (scudo::uptr J = 0; J < NumBuffers; ++J) {
+ BufferPool::Buffer Buffer;
+ EXPECT_TRUE(Pool->getBuffer(Buffer, StaticBufferNumElements));
+ EXPECT_NE(Buffer.Data, nullptr);
+ if (TEST_HAS_FAILURE)
+ break;
+
+ Buffer.Data[0] = I + 1;
+ Buffer.Data[StaticBufferNumElements - 1] = I + 1;
+ Buffers.push_back(Buffer);
+ }
+ for (auto &Buffer : Buffers) {
+ EXPECT_EQ(Buffer.Data[0], I + 1);
+ EXPECT_EQ(Buffer.Data[StaticBufferNumElements - 1], I + 1);
+ Pool->releaseBuffer(Buffer);
+ EXPECT_EQ(Buffer.Data, nullptr);
+ }
+ if (TEST_HAS_FAILURE)
+ break;
+ }
+ });
+ }
+
+ Ready = true;
+
+ for (auto &T : Threads)
+ T.join();
+
+ // Now guarantee that the Pool is completely empty and we can allocate the
+ // entire static buffer.
+ BufferPool::Buffer Buffer;
+ EXPECT_TRUE(Pool->getBuffer(Buffer, StaticBufferNumElements));
+ EXPECT_TRUE(Pool->isStaticBufferTestOnly(Buffer));
+ Pool->releaseBuffer(Buffer);
+ EXPECT_EQ(Buffer.Data, nullptr);
}
More information about the llvm-commits
mailing list