[scudo] Get rid of initLinkerInitialized
Now that everything is forcibly linker initialized, it feels like a
good time to get rid of the `init`/`initLinkerInitialized` split.
This allows to get rid of various `memset` construct in `init` that
gcc complains about (this fixes a Fuchsia open issue).
I added various `DCHECK`s to ensure that we would get a zero-inited
object when entering `init`, which required ensuring that
`unmapTestOnly` leaves the object in a good state (tests are currently
the only location where an allocator can be "de-initialized").
Running the tests with `--gtest_repeat=` showed no issue.
Differential Revision: https://reviews.llvm.org/D103119
GitOrigin-RevId: a45877eea8c424cd91bc1f7749313c9cb3aab285
diff --git a/benchmarks/malloc_benchmark.cpp b/benchmarks/malloc_benchmark.cpp
index 661fff4..2adec88 100644
--- a/benchmarks/malloc_benchmark.cpp
+++ b/benchmarks/malloc_benchmark.cpp
@@ -29,7 +29,6 @@
std::unique_ptr<AllocatorT, decltype(Deleter)> Allocator(new AllocatorT,
Deleter);
CurrentAllocator = Allocator.get();
- Allocator->reset();
const size_t NBytes = State.range(0);
size_t PageSize = scudo::getPageSizeCached();
@@ -70,7 +69,6 @@
std::unique_ptr<AllocatorT, decltype(Deleter)> Allocator(new AllocatorT,
Deleter);
CurrentAllocator = Allocator.get();
- Allocator->reset();
const size_t NumIters = State.range(0);
size_t PageSize = scudo::getPageSizeCached();
diff --git a/bytemap.h b/bytemap.h
index e0d54f4..248e096 100644
--- a/bytemap.h
+++ b/bytemap.h
@@ -17,10 +17,9 @@
template <uptr Size> class FlatByteMap {
public:
- void initLinkerInitialized() {}
- void init() { memset(Map, 0, sizeof(Map)); }
+ void init() { DCHECK(Size == 0 || Map[0] == 0); }
- void unmapTestOnly() {}
+ void unmapTestOnly() { memset(Map, 0, Size); }
void set(uptr Index, u8 Value) {
DCHECK_LT(Index, Size);
@@ -36,7 +35,7 @@
void enable() {}
private:
- u8 Map[Size];
+ u8 Map[Size] = {};
};
} // namespace scudo
diff --git a/combined.h b/combined.h
index f17c371..8caffcc 100644
--- a/combined.h
+++ b/combined.h
@@ -132,7 +132,7 @@
typedef GlobalQuarantine<QuarantineCallback, void> QuarantineT;
typedef typename QuarantineT::CacheT QuarantineCacheT;
- void initLinkerInitialized() {
+ void init() {
performSanityChecks();
// Check if hardware CRC32 is supported in the binary and by the platform,
@@ -166,11 +166,10 @@
QuarantineMaxChunkSize =
static_cast<u32>(getFlags()->quarantine_max_chunk_size);
- Stats.initLinkerInitialized();
+ Stats.init();
const s32 ReleaseToOsIntervalMs = getFlags()->release_to_os_interval_ms;
- Primary.initLinkerInitialized(ReleaseToOsIntervalMs);
- Secondary.initLinkerInitialized(&Stats, ReleaseToOsIntervalMs);
-
+ Primary.init(ReleaseToOsIntervalMs);
+ Secondary.init(&Stats, ReleaseToOsIntervalMs);
Quarantine.init(
static_cast<uptr>(getFlags()->quarantine_size_kb << 10),
static_cast<uptr>(getFlags()->thread_local_quarantine_size_kb << 10));
@@ -210,10 +209,8 @@
TSDRegistry.initThreadMaybe(this, MinimalInit);
}
- void reset() { memset(this, 0, sizeof(*this)); }
-
void unmapTestOnly() {
- TSDRegistry.unmapTestOnly();
+ TSDRegistry.unmapTestOnly(this);
Primary.unmapTestOnly();
Secondary.unmapTestOnly();
#ifdef GWP_ASAN_HOOKS
@@ -226,9 +223,7 @@
TSDRegistryT *getTSDRegistry() { return &TSDRegistry; }
// The Cache must be provided zero-initialized.
- void initCache(CacheT *Cache) {
- Cache->initLinkerInitialized(&Stats, &Primary);
- }
+ void initCache(CacheT *Cache) { Cache->init(&Stats, &Primary); }
// Release the resources used by a TSD, which involves:
// - draining the local quarantine cache to the global quarantine;
diff --git a/local_cache.h b/local_cache.h
index 5003937..f46645f 100644
--- a/local_cache.h
+++ b/local_cache.h
@@ -49,18 +49,14 @@
CompactPtrT Batch[MaxNumCached];
};
- void initLinkerInitialized(GlobalStats *S, SizeClassAllocator *A) {
- Stats.initLinkerInitialized();
+ void init(GlobalStats *S, SizeClassAllocator *A) {
+ DCHECK(isEmpty());
+ Stats.init();
if (LIKELY(S))
S->link(&Stats);
Allocator = A;
}
- void init(GlobalStats *S, SizeClassAllocator *A) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(S, A);
- }
-
void destroy(GlobalStats *S) {
drain();
if (LIKELY(S))
diff --git a/mutex.h b/mutex.h
index a654d35..c8504c0 100644
--- a/mutex.h
+++ b/mutex.h
@@ -22,7 +22,6 @@
class HybridMutex {
public:
- void init() { M = {}; }
bool tryLock();
NOINLINE void lock() {
if (LIKELY(tryLock()))
diff --git a/primary32.h b/primary32.h
index 33d8175..059680b 100644
--- a/primary32.h
+++ b/primary32.h
@@ -60,11 +60,11 @@
static bool canAllocate(uptr Size) { return Size <= SizeClassMap::MaxSize; }
- void initLinkerInitialized(s32 ReleaseToOsInterval) {
+ void init(s32 ReleaseToOsInterval) {
if (SCUDO_FUCHSIA)
reportError("SizeClassAllocator32 is not supported on Fuchsia");
- PossibleRegions.initLinkerInitialized();
+ PossibleRegions.init();
u32 Seed;
const u64 Time = getMonotonicTime();
@@ -80,10 +80,6 @@
}
setOption(Option::ReleaseInterval, static_cast<sptr>(ReleaseToOsInterval));
}
- void init(s32 ReleaseToOsInterval) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(ReleaseToOsInterval);
- }
void unmapTestOnly() {
while (NumberOfStashedRegions > 0)
@@ -96,6 +92,7 @@
MinRegionIndex = Sci->MinRegionIndex;
if (Sci->MaxRegionIndex > MaxRegionIndex)
MaxRegionIndex = Sci->MaxRegionIndex;
+ *Sci = {};
}
for (uptr I = MinRegionIndex; I < MaxRegionIndex; I++)
if (PossibleRegions[I])
diff --git a/primary64.h b/primary64.h
index 94375fc..0a43f46 100644
--- a/primary64.h
+++ b/primary64.h
@@ -57,7 +57,8 @@
static bool canAllocate(uptr Size) { return Size <= SizeClassMap::MaxSize; }
- void initLinkerInitialized(s32 ReleaseToOsInterval) {
+ void init(s32 ReleaseToOsInterval) {
+ DCHECK_EQ(PrimaryBase, 0U);
// Reserve the space required for the Primary.
PrimaryBase = reinterpret_cast<uptr>(
map(nullptr, PrimarySize, nullptr, MAP_NOACCESS, &Data));
@@ -77,13 +78,14 @@
}
setOption(Option::ReleaseInterval, static_cast<sptr>(ReleaseToOsInterval));
}
- void init(s32 ReleaseToOsInterval) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(ReleaseToOsInterval);
- }
void unmapTestOnly() {
+ for (uptr I = 0; I < NumClasses; I++) {
+ RegionInfo *Region = getRegionInfo(I);
+ *Region = {};
+ }
unmap(reinterpret_cast<void *>(PrimaryBase), PrimarySize, UNMAP_ALL, &Data);
+ PrimaryBase = 0U;
}
TransferBatch *popBatch(CacheT *C, uptr ClassId) {
diff --git a/quarantine.h b/quarantine.h
index 8d4b38e..84eb90c 100644
--- a/quarantine.h
+++ b/quarantine.h
@@ -64,11 +64,7 @@
// Per-thread cache of memory blocks.
template <typename Callback> class QuarantineCache {
public:
- void initLinkerInitialized() {}
- void init() {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized();
- }
+ void init() { DCHECK_EQ(atomic_load_relaxed(&Size), 0U); }
// Total memory used, including internal accounting.
uptr getSize() const { return atomic_load_relaxed(&Size); }
@@ -175,7 +171,10 @@
public:
typedef QuarantineCache<Callback> CacheT;
- void initLinkerInitialized(uptr Size, uptr CacheSize) {
+ void init(uptr Size, uptr CacheSize) {
+ DCHECK_EQ(atomic_load_relaxed(&MaxSize), 0U);
+ DCHECK_EQ(atomic_load_relaxed(&MinSize), 0U);
+ DCHECK_EQ(atomic_load_relaxed(&MaxCacheSize), 0U);
// Thread local quarantine size can be zero only when global quarantine size
// is zero (it allows us to perform just one atomic read per put() call).
CHECK((Size == 0 && CacheSize == 0) || CacheSize != 0);
@@ -184,16 +183,7 @@
atomic_store_relaxed(&MinSize, Size / 10 * 9); // 90% of max size.
atomic_store_relaxed(&MaxCacheSize, CacheSize);
- Cache.initLinkerInitialized();
- }
- void init(uptr Size, uptr CacheSize) {
- CacheMutex.init();
Cache.init();
- RecycleMutex.init();
- MinSize = {};
- MaxSize = {};
- MaxCacheSize = {};
- initLinkerInitialized(Size, CacheSize);
}
uptr getMaxSize() const { return atomic_load_relaxed(&MaxSize); }
diff --git a/secondary.h b/secondary.h
index 3894b01..630e64d 100644
--- a/secondary.h
+++ b/secondary.h
@@ -71,7 +71,6 @@
class MapAllocatorNoCache {
public:
- void initLinkerInitialized(UNUSED s32 ReleaseToOsInterval) {}
void init(UNUSED s32 ReleaseToOsInterval) {}
bool retrieve(UNUSED Options Options, UNUSED uptr Size, UNUSED uptr Alignment,
UNUSED LargeBlock::Header **H, UNUSED bool *Zeroed) {
@@ -121,17 +120,14 @@
Config::SecondaryCacheEntriesArraySize,
"");
- void initLinkerInitialized(s32 ReleaseToOsInterval) {
+ void init(s32 ReleaseToOsInterval) {
+ DCHECK_EQ(EntriesCount, 0U);
setOption(Option::MaxCacheEntriesCount,
static_cast<sptr>(Config::SecondaryCacheDefaultMaxEntriesCount));
setOption(Option::MaxCacheEntrySize,
static_cast<sptr>(Config::SecondaryCacheDefaultMaxEntrySize));
setOption(Option::ReleaseInterval, static_cast<sptr>(ReleaseToOsInterval));
}
- void init(s32 ReleaseToOsInterval) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(ReleaseToOsInterval);
- }
void store(Options Options, LargeBlock::Header *H) {
if (!canCache(H->CommitSize))
@@ -404,16 +400,14 @@
template <typename Config> class MapAllocator {
public:
- void initLinkerInitialized(GlobalStats *S, s32 ReleaseToOsInterval = -1) {
- Cache.initLinkerInitialized(ReleaseToOsInterval);
- Stats.initLinkerInitialized();
+ void init(GlobalStats *S, s32 ReleaseToOsInterval = -1) {
+ DCHECK_EQ(AllocatedBytes, 0U);
+ DCHECK_EQ(FreedBytes, 0U);
+ Cache.init(ReleaseToOsInterval);
+ Stats.init();
if (LIKELY(S))
S->link(&Stats);
}
- void init(GlobalStats *S, s32 ReleaseToOsInterval = -1) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(S, ReleaseToOsInterval);
- }
void *allocate(Options Options, uptr Size, uptr AlignmentHint = 0,
uptr *BlockEnd = nullptr,
diff --git a/stats.h b/stats.h
index e15c056..be5bf2d 100644
--- a/stats.h
+++ b/stats.h
@@ -29,8 +29,10 @@
// LocalStats::add'ing, this is OK, we will still get a meaningful number.
class LocalStats {
public:
- void initLinkerInitialized() {}
- void init() { memset(this, 0, sizeof(*this)); }
+ void init() {
+ for (uptr I = 0; I < StatCount; I++)
+ DCHECK_EQ(get(static_cast<StatType>(I)), 0U);
+ }
void add(StatType I, uptr V) {
V += atomic_load_relaxed(&StatsArray[I]);
@@ -56,13 +58,7 @@
// Global stats, used for aggregation and querying.
class GlobalStats : public LocalStats {
public:
- void initLinkerInitialized() {}
- void init() {
- LocalStats::init();
- Mutex.init();
- StatsList = {};
- initLinkerInitialized();
- }
+ void init() { LocalStats::init(); }
void link(LocalStats *S) {
ScopedLock L(Mutex);
diff --git a/tests/combined_test.cpp b/tests/combined_test.cpp
index a5dcaed..983e9b4 100644
--- a/tests/combined_test.cpp
+++ b/tests/combined_test.cpp
@@ -68,7 +68,6 @@
template <typename Config> struct TestAllocator : scudo::Allocator<Config> {
TestAllocator() {
- this->reset();
this->initThreadMaybe();
if (scudo::archSupportsMemoryTagging() &&
!scudo::systemDetectsMemoryTagFaultsTestOnly())
diff --git a/tests/mutex_test.cpp b/tests/mutex_test.cpp
index ed56cb5..efee6fe 100644
--- a/tests/mutex_test.cpp
+++ b/tests/mutex_test.cpp
@@ -82,7 +82,6 @@
TEST(ScudoMutexTest, Mutex) {
scudo::HybridMutex M;
- M.init();
TestData Data(M);
pthread_t Threads[NumberOfThreads];
for (scudo::u32 I = 0; I < NumberOfThreads; I++)
@@ -93,7 +92,6 @@
TEST(ScudoMutexTest, MutexTry) {
scudo::HybridMutex M;
- M.init();
TestData Data(M);
pthread_t Threads[NumberOfThreads];
for (scudo::u32 I = 0; I < NumberOfThreads; I++)
diff --git a/tests/tsd_test.cpp b/tests/tsd_test.cpp
index 58ac9e7..35e579d 100644
--- a/tests/tsd_test.cpp
+++ b/tests/tsd_test.cpp
@@ -26,15 +26,14 @@
using CacheT = struct MockCache { volatile scudo::uptr Canary; };
using QuarantineCacheT = struct MockQuarantine {};
- void initLinkerInitialized() {
+ void init() {
// This should only be called once by the registry.
EXPECT_FALSE(Initialized);
Initialized = true;
}
- void reset() { memset(this, 0, sizeof(*this)); }
- void unmapTestOnly() { TSDRegistry.unmapTestOnly(); }
- void initCache(CacheT *Cache) { memset(Cache, 0, sizeof(*Cache)); }
+ void unmapTestOnly() { TSDRegistry.unmapTestOnly(this); }
+ void initCache(CacheT *Cache) { *Cache = {}; }
void commitBack(scudo::TSD<MockAllocator> *TSD) {}
TSDRegistryT *getTSDRegistry() { return &TSDRegistry; }
void callPostInitCallback() {}
@@ -42,7 +41,7 @@
bool isInitialized() { return Initialized; }
private:
- bool Initialized;
+ bool Initialized = false;
TSDRegistryT TSDRegistry;
};
@@ -69,11 +68,10 @@
};
std::unique_ptr<AllocatorT, decltype(Deleter)> Allocator(new AllocatorT,
Deleter);
- Allocator->reset();
EXPECT_FALSE(Allocator->isInitialized());
auto Registry = Allocator->getTSDRegistry();
- Registry->initLinkerInitialized(Allocator.get());
+ Registry->init(Allocator.get());
EXPECT_TRUE(Allocator->isInitialized());
}
@@ -84,7 +82,6 @@
};
std::unique_ptr<AllocatorT, decltype(Deleter)> Allocator(new AllocatorT,
Deleter);
- Allocator->reset();
EXPECT_FALSE(Allocator->isInitialized());
auto Registry = Allocator->getTSDRegistry();
@@ -153,7 +150,6 @@
};
std::unique_ptr<AllocatorT, decltype(Deleter)> Allocator(new AllocatorT,
Deleter);
- Allocator->reset();
std::thread Threads[32];
for (scudo::uptr I = 0; I < ARRAY_SIZE(Threads); I++)
Threads[I] = std::thread(stressCache<AllocatorT>, Allocator.get());
@@ -209,7 +205,6 @@
};
std::unique_ptr<AllocatorT, decltype(Deleter)> Allocator(new AllocatorT,
Deleter);
- Allocator->reset();
// We attempt to use as many TSDs as the shared cache offers by creating a
// decent amount of threads that will be run concurrently and attempt to get
// and lock TSDs. We put them all in a set and count the number of entries
diff --git a/tsd.h b/tsd.h
index a6e669b..e376df0 100644
--- a/tsd.h
+++ b/tsd.h
@@ -28,14 +28,11 @@
typename Allocator::QuarantineCacheT QuarantineCache;
u8 DestructorIterations = 0;
- void initLinkerInitialized(Allocator *Instance) {
+ void init(Allocator *Instance) {
+ DCHECK_EQ(DestructorIterations, 0U);
Instance->initCache(&Cache);
DestructorIterations = PTHREAD_DESTRUCTOR_ITERATIONS;
}
- void init(Allocator *Instance) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(Instance);
- }
void commitBack(Allocator *Instance) { Instance->commitBack(this); }
diff --git a/tsd_exclusive.h b/tsd_exclusive.h
index a907ed4..bba0c27 100644
--- a/tsd_exclusive.h
+++ b/tsd_exclusive.h
@@ -25,31 +25,35 @@
template <class Allocator> void teardownThread(void *Ptr);
template <class Allocator> struct TSDRegistryExT {
- void initLinkerInitialized(Allocator *Instance) {
- Instance->initLinkerInitialized();
- CHECK_EQ(pthread_key_create(&PThreadKey, teardownThread<Allocator>), 0);
- FallbackTSD.initLinkerInitialized(Instance);
- Initialized = true;
- }
void init(Allocator *Instance) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(Instance);
+ DCHECK(!Initialized);
+ Instance->init();
+ CHECK_EQ(pthread_key_create(&PThreadKey, teardownThread<Allocator>), 0);
+ FallbackTSD.init(Instance);
+ Initialized = true;
}
void initOnceMaybe(Allocator *Instance) {
ScopedLock L(Mutex);
if (LIKELY(Initialized))
return;
- initLinkerInitialized(Instance); // Sets Initialized.
+ init(Instance); // Sets Initialized.
}
- void unmapTestOnly() {
- Allocator *Instance =
- reinterpret_cast<Allocator *>(pthread_getspecific(PThreadKey));
- if (!Instance)
- return;
- ThreadTSD.commitBack(Instance);
+ void unmapTestOnly(Allocator *Instance) {
+ DCHECK(Instance);
+ if (reinterpret_cast<Allocator *>(pthread_getspecific(PThreadKey))) {
+ DCHECK_EQ(reinterpret_cast<Allocator *>(pthread_getspecific(PThreadKey)),
+ Instance);
+ ThreadTSD.commitBack(Instance);
+ ThreadTSD = {};
+ }
+ CHECK_EQ(pthread_key_delete(PThreadKey), 0);
+ PThreadKey = {};
+ FallbackTSD.commitBack(Instance);
+ FallbackTSD = {};
State = {};
+ Initialized = false;
}
ALWAYS_INLINE void initThreadMaybe(Allocator *Instance, bool MinimalInit) {
@@ -103,7 +107,7 @@
return;
CHECK_EQ(
pthread_setspecific(PThreadKey, reinterpret_cast<void *>(Instance)), 0);
- ThreadTSD.initLinkerInitialized(Instance);
+ ThreadTSD.init(Instance);
State.InitState = ThreadState::Initialized;
Instance->callPostInitCallback();
}
diff --git a/tsd_shared.h b/tsd_shared.h
index afe3623..1c2a880 100644
--- a/tsd_shared.h
+++ b/tsd_shared.h
@@ -24,28 +24,32 @@
template <class Allocator, u32 TSDsArraySize, u32 DefaultTSDCount>
struct TSDRegistrySharedT {
- void initLinkerInitialized(Allocator *Instance) {
- Instance->initLinkerInitialized();
+ void init(Allocator *Instance) {
+ DCHECK(!Initialized);
+ Instance->init();
for (u32 I = 0; I < TSDsArraySize; I++)
- TSDs[I].initLinkerInitialized(Instance);
+ TSDs[I].init(Instance);
const u32 NumberOfCPUs = getNumberOfCPUs();
setNumberOfTSDs((NumberOfCPUs == 0) ? DefaultTSDCount
: Min(NumberOfCPUs, DefaultTSDCount));
Initialized = true;
}
- void init(Allocator *Instance) {
- memset(this, 0, sizeof(*this));
- initLinkerInitialized(Instance);
- }
void initOnceMaybe(Allocator *Instance) {
ScopedLock L(Mutex);
if (LIKELY(Initialized))
return;
- initLinkerInitialized(Instance); // Sets Initialized.
+ init(Instance); // Sets Initialized.
}
- void unmapTestOnly() { setCurrentTSD(nullptr); }
+ void unmapTestOnly(Allocator *Instance) {
+ for (u32 I = 0; I < TSDsArraySize; I++) {
+ TSDs[I].commitBack(Instance);
+ TSDs[I] = {};
+ }
+ setCurrentTSD(nullptr);
+ Initialized = false;
+ }
ALWAYS_INLINE void initThreadMaybe(Allocator *Instance,
UNUSED bool MinimalInit) {
