[compiler-rt] [scudo] Only init RingBuffer when needed. (PR #85994)
Christopher Ferris via llvm-commits
llvm-commits at lists.llvm.org
Thu Mar 28 14:13:20 PDT 2024
https://github.com/cferris1000 updated https://github.com/llvm/llvm-project/pull/85994
>From c18d8f1405eff173bd4ea914e75940353fd4aa9b Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris at google.com>
Date: Wed, 20 Mar 2024 12:31:03 -0700
Subject: [PATCH 1/4] [scudo] Only init RingBuffer when needed.
Only attempt to initialize the ring buffer when tracking is enabled.
Updated unit tests, and added a few new unit tests to verify the
RingBuffer is not initialized by default.
Verified that the two maps associated with the RingBuffer are not
created in processes by default.
---
compiler-rt/lib/scudo/standalone/combined.h | 44 ++++++++++-----
.../scudo/standalone/tests/combined_test.cpp | 54 +++++++++++++++++++
2 files changed, 84 insertions(+), 14 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index f4dd90aac66555..64ba623eacdd8a 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -178,8 +178,6 @@ class Allocator {
Quarantine.init(
static_cast<uptr>(getFlags()->quarantine_size_kb << 10),
static_cast<uptr>(getFlags()->thread_local_quarantine_size_kb << 10));
-
- mapAndInitializeRingBuffer();
}
void enableRingBuffer() {
@@ -915,9 +913,11 @@ class Allocator {
DCHECK(!Primary.Options.load().get(OptionBit::TrackAllocationStacks));
return;
}
- if (Track)
+
+ if (Track) {
+ initRingBufferMaybe();
Primary.Options.set(OptionBit::TrackAllocationStacks);
- else
+ } else
Primary.Options.clear(OptionBit::TrackAllocationStacks);
}
@@ -1546,11 +1546,15 @@ class Allocator {
RBEntryStart)[N];
}
- void mapAndInitializeRingBuffer() {
- if (getFlags()->allocation_ring_buffer_size <= 0)
+ void initRingBufferMaybe() {
+ if (getRingBuffer() != nullptr)
+ return;
+
+ int ring_buffer_size = getFlags()->allocation_ring_buffer_size;
+ if (ring_buffer_size <= 0)
return;
- u32 AllocationRingBufferSize =
- static_cast<u32>(getFlags()->allocation_ring_buffer_size);
+
+ u32 AllocationRingBufferSize = static_cast<u32>(ring_buffer_size);
// We store alloc and free stacks for each entry.
constexpr u32 kStacksPerRingBufferEntry = 2;
@@ -1594,14 +1598,19 @@ class Allocator {
RB->StackDepotSize = StackDepotSize;
RB->RawStackDepotMap = DepotMap;
- atomic_store(&RingBufferAddress, reinterpret_cast<uptr>(RB),
- memory_order_release);
+ // If multiple threads try to initialize at the same time, let one thread
+ // win and throw away the work done in the other threads. Since this
+ // path is only meant for debugging, a race that results in work being
+ // discarded should not matter.
+ uptr EmptyPtr = 0;
+ if (!atomic_compare_exchange_strong(&RingBufferAddress, &EmptyPtr,
+ reinterpret_cast<uptr>(RB),
+ memory_order_acquire)) {
+ unmapRingBuffer(RB);
+ }
}
- void unmapRingBuffer() {
- AllocationRingBuffer *RB = getRingBuffer();
- if (RB == nullptr)
- return;
+ void unmapRingBuffer(AllocationRingBuffer *RB) {
// N.B. because RawStackDepotMap is part of RawRingBufferMap, the order
// is very important.
RB->RawStackDepotMap.unmap(RB->RawStackDepotMap.getBase(),
@@ -1612,6 +1621,13 @@ class Allocator {
MemMapT RawRingBufferMap = RB->RawRingBufferMap;
RawRingBufferMap.unmap(RawRingBufferMap.getBase(),
RawRingBufferMap.getCapacity());
+ }
+
+ void unmapRingBuffer() {
+ AllocationRingBuffer *RB = getRingBuffer();
+ if (RB == nullptr)
+ return;
+ unmapRingBuffer(RB);
atomic_store(&RingBufferAddress, 0, memory_order_release);
}
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 6a311adc55e4bd..9a16886acd688d 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -867,8 +867,33 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, ReallocateInPlaceStress) {
}
}
+SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferDefaultDisabled) {
+ // The RingBuffer is not initialized until tracking is enabled for the
+ // first time.
+ auto *Allocator = this->Allocator.get();
+ ASSERT_EQ(0u, Allocator->getRingBufferSize());
+ ASSERT_EQ(nullptr, Allocator->getRingBufferAddress());
+}
+
+SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferInitOnce) {
+ auto *Allocator = this->Allocator.get();
+ Allocator->setTrackAllocationStacks(true);
+
+ auto Size = Allocator->getRingBufferSize();
+ ASSERT_GT(Size, 0u);
+ auto *Addr = Allocator->getRingBufferAddress();
+ EXPECT_NE(nullptr, Addr);
+
+ // Enable tracking again to verify that the initialization only happens once.
+ Allocator->setTrackAllocationStacks(true);
+ ASSERT_EQ(Size, Allocator->getRingBufferSize());
+ EXPECT_EQ(Addr, Allocator->getRingBufferAddress());
+}
+
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferSize) {
auto *Allocator = this->Allocator.get();
+ Allocator->setTrackAllocationStacks(true);
+
auto Size = Allocator->getRingBufferSize();
ASSERT_GT(Size, 0u);
EXPECT_EQ(Allocator->getRingBufferAddress()[Size - 1], '\0');
@@ -876,13 +901,40 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferSize) {
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferAddress) {
auto *Allocator = this->Allocator.get();
+ Allocator->setTrackAllocationStacks(true);
+
auto *Addr = Allocator->getRingBufferAddress();
EXPECT_NE(Addr, nullptr);
EXPECT_EQ(Addr, Allocator->getRingBufferAddress());
}
+SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotDefaultDisabled) {
+ // The StackDepot is not initialized until tracking is enabled for the
+ // first time.
+ auto *Allocator = this->Allocator.get();
+ ASSERT_EQ(0u, Allocator->getStackDepotSize());
+ ASSERT_EQ(nullptr, Allocator->getStackDepotAddress());
+}
+
+SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotInitOnce) {
+ auto *Allocator = this->Allocator.get();
+ Allocator->setTrackAllocationStacks(true);
+
+ auto Size = Allocator->getStackDepotSize();
+ ASSERT_GT(Size, 0u);
+ auto *Addr = Allocator->getStackDepotAddress();
+ EXPECT_NE(nullptr, Addr);
+
+ // Enable tracking again to verify that the initialization only happens once.
+ Allocator->setTrackAllocationStacks(true);
+ ASSERT_EQ(Size, Allocator->getStackDepotSize());
+ EXPECT_EQ(Addr, Allocator->getStackDepotAddress());
+}
+
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotSize) {
auto *Allocator = this->Allocator.get();
+ Allocator->setTrackAllocationStacks(true);
+
auto Size = Allocator->getStackDepotSize();
ASSERT_GT(Size, 0u);
EXPECT_EQ(Allocator->getStackDepotAddress()[Size - 1], '\0');
@@ -890,6 +942,8 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotSize) {
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotAddress) {
auto *Allocator = this->Allocator.get();
+ Allocator->setTrackAllocationStacks(true);
+
auto *Addr = Allocator->getStackDepotAddress();
EXPECT_NE(Addr, nullptr);
EXPECT_EQ(Addr, Allocator->getStackDepotAddress());
>From 415ea52a2a37382d17a8175e184a8f4cb47d4164 Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris at google.com>
Date: Tue, 26 Mar 2024 20:05:43 -0700
Subject: [PATCH 2/4] Update for comments.
---
compiler-rt/lib/scudo/standalone/combined.h | 27 +++------
.../scudo/standalone/tests/combined_test.cpp | 56 +++++++++----------
2 files changed, 37 insertions(+), 46 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 64ba623eacdd8a..bf488dacd9c8f0 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -18,6 +18,7 @@
#include "local_cache.h"
#include "mem_map.h"
#include "memtag.h"
+#include "mutex.h"
#include "options.h"
#include "quarantine.h"
#include "report.h"
@@ -1547,6 +1548,8 @@ class Allocator {
}
void initRingBufferMaybe() {
+ static HybridMutex RingBufferLock;
+ ScopedLock L(RingBufferLock);
if (getRingBuffer() != nullptr)
return;
@@ -1598,19 +1601,14 @@ class Allocator {
RB->StackDepotSize = StackDepotSize;
RB->RawStackDepotMap = DepotMap;
- // If multiple threads try to initialize at the same time, let one thread
- // win and throw away the work done in the other threads. Since this
- // path is only meant for debugging, a race that results in work being
- // discarded should not matter.
- uptr EmptyPtr = 0;
- if (!atomic_compare_exchange_strong(&RingBufferAddress, &EmptyPtr,
- reinterpret_cast<uptr>(RB),
- memory_order_acquire)) {
- unmapRingBuffer(RB);
- }
+ atomic_store(&RingBufferAddress, reinterpret_cast<uptr>(RB),
+ memory_order_release);
}
- void unmapRingBuffer(AllocationRingBuffer *RB) {
+ void unmapRingBuffer() {
+ AllocationRingBuffer *RB = getRingBuffer();
+ if (RB == nullptr)
+ return;
// N.B. because RawStackDepotMap is part of RawRingBufferMap, the order
// is very important.
RB->RawStackDepotMap.unmap(RB->RawStackDepotMap.getBase(),
@@ -1621,13 +1619,6 @@ class Allocator {
MemMapT RawRingBufferMap = RB->RawRingBufferMap;
RawRingBufferMap.unmap(RawRingBufferMap.getBase(),
RawRingBufferMap.getCapacity());
- }
-
- void unmapRingBuffer() {
- AllocationRingBuffer *RB = getRingBuffer();
- if (RB == nullptr)
- return;
- unmapRingBuffer(RB);
atomic_store(&RingBufferAddress, 0, memory_order_release);
}
diff --git a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
index 9a16886acd688d..1a36155bcd423a 100644
--- a/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/combined_test.cpp
@@ -871,82 +871,82 @@ SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferDefaultDisabled) {
// The RingBuffer is not initialized until tracking is enabled for the
// first time.
auto *Allocator = this->Allocator.get();
- ASSERT_EQ(0u, Allocator->getRingBufferSize());
- ASSERT_EQ(nullptr, Allocator->getRingBufferAddress());
+ EXPECT_EQ(0u, Allocator->getRingBufferSize());
+ EXPECT_EQ(nullptr, Allocator->getRingBufferAddress());
}
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferInitOnce) {
auto *Allocator = this->Allocator.get();
Allocator->setTrackAllocationStacks(true);
- auto Size = Allocator->getRingBufferSize();
- ASSERT_GT(Size, 0u);
- auto *Addr = Allocator->getRingBufferAddress();
- EXPECT_NE(nullptr, Addr);
+ auto RingBufferSize = Allocator->getRingBufferSize();
+ ASSERT_GT(RingBufferSize, 0u);
+ auto *RingBufferAddress = Allocator->getRingBufferAddress();
+ EXPECT_NE(nullptr, RingBufferAddress);
// Enable tracking again to verify that the initialization only happens once.
Allocator->setTrackAllocationStacks(true);
- ASSERT_EQ(Size, Allocator->getRingBufferSize());
- EXPECT_EQ(Addr, Allocator->getRingBufferAddress());
+ ASSERT_EQ(RingBufferSize, Allocator->getRingBufferSize());
+ EXPECT_EQ(RingBufferAddress, Allocator->getRingBufferAddress());
}
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferSize) {
auto *Allocator = this->Allocator.get();
Allocator->setTrackAllocationStacks(true);
- auto Size = Allocator->getRingBufferSize();
- ASSERT_GT(Size, 0u);
- EXPECT_EQ(Allocator->getRingBufferAddress()[Size - 1], '\0');
+ auto RingBufferSize = Allocator->getRingBufferSize();
+ ASSERT_GT(RingBufferSize, 0u);
+ EXPECT_EQ(Allocator->getRingBufferAddress()[RingBufferSize - 1], '\0');
}
SCUDO_TYPED_TEST(ScudoCombinedTest, RingBufferAddress) {
auto *Allocator = this->Allocator.get();
Allocator->setTrackAllocationStacks(true);
- auto *Addr = Allocator->getRingBufferAddress();
- EXPECT_NE(Addr, nullptr);
- EXPECT_EQ(Addr, Allocator->getRingBufferAddress());
+ auto *RingBufferAddress = Allocator->getRingBufferAddress();
+ EXPECT_NE(RingBufferAddress, nullptr);
+ EXPECT_EQ(RingBufferAddress, Allocator->getRingBufferAddress());
}
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotDefaultDisabled) {
// The StackDepot is not initialized until tracking is enabled for the
// first time.
auto *Allocator = this->Allocator.get();
- ASSERT_EQ(0u, Allocator->getStackDepotSize());
- ASSERT_EQ(nullptr, Allocator->getStackDepotAddress());
+ EXPECT_EQ(0u, Allocator->getStackDepotSize());
+ EXPECT_EQ(nullptr, Allocator->getStackDepotAddress());
}
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotInitOnce) {
auto *Allocator = this->Allocator.get();
Allocator->setTrackAllocationStacks(true);
- auto Size = Allocator->getStackDepotSize();
- ASSERT_GT(Size, 0u);
- auto *Addr = Allocator->getStackDepotAddress();
- EXPECT_NE(nullptr, Addr);
+ auto StackDepotSize = Allocator->getStackDepotSize();
+ EXPECT_GT(StackDepotSize, 0u);
+ auto *StackDepotAddress = Allocator->getStackDepotAddress();
+ EXPECT_NE(nullptr, StackDepotAddress);
// Enable tracking again to verify that the initialization only happens once.
Allocator->setTrackAllocationStacks(true);
- ASSERT_EQ(Size, Allocator->getStackDepotSize());
- EXPECT_EQ(Addr, Allocator->getStackDepotAddress());
+ EXPECT_EQ(StackDepotSize, Allocator->getStackDepotSize());
+ EXPECT_EQ(StackDepotAddress, Allocator->getStackDepotAddress());
}
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotSize) {
auto *Allocator = this->Allocator.get();
Allocator->setTrackAllocationStacks(true);
- auto Size = Allocator->getStackDepotSize();
- ASSERT_GT(Size, 0u);
- EXPECT_EQ(Allocator->getStackDepotAddress()[Size - 1], '\0');
+ auto StackDepotSize = Allocator->getStackDepotSize();
+ EXPECT_GT(StackDepotSize, 0u);
+ EXPECT_EQ(Allocator->getStackDepotAddress()[StackDepotSize - 1], '\0');
}
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepotAddress) {
auto *Allocator = this->Allocator.get();
Allocator->setTrackAllocationStacks(true);
- auto *Addr = Allocator->getStackDepotAddress();
- EXPECT_NE(Addr, nullptr);
- EXPECT_EQ(Addr, Allocator->getStackDepotAddress());
+ auto *StackDepotAddress = Allocator->getStackDepotAddress();
+ EXPECT_NE(StackDepotAddress, nullptr);
+ EXPECT_EQ(StackDepotAddress, Allocator->getStackDepotAddress());
}
SCUDO_TYPED_TEST(ScudoCombinedTest, StackDepot) {
>From cfb10bf26c4dd9223ca8feb6d9f908e3525fcdb5 Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris at google.com>
Date: Wed, 27 Mar 2024 20:16:53 -0700
Subject: [PATCH 3/4] Make ring buffer init lock global and lock/unlock in
enable/disable.
---
compiler-rt/lib/scudo/standalone/combined.h | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index bf488dacd9c8f0..023b10a07b4972 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -181,13 +181,15 @@ class Allocator {
static_cast<uptr>(getFlags()->thread_local_quarantine_size_kb << 10));
}
- void enableRingBuffer() {
+ void enableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
+ RingBufferInitLock.unlock();
AllocationRingBuffer *RB = getRingBuffer();
if (RB)
RB->Depot->enable();
}
- void disableRingBuffer() {
+ void disableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
+ RingBufferInitLock.lock();
AllocationRingBuffer *RB = getRingBuffer();
if (RB)
RB->Depot->disable();
@@ -1093,6 +1095,9 @@ class Allocator {
0,
"invalid alignment");
+ // Lock to initialize the RingBuffer
+ HybridMutex RingBufferInitLock;
+
// Pointer to memory mapped area starting with AllocationRingBuffer struct,
// and immediately followed by Size elements of type Entry.
atomic_uptr RingBufferAddress = {};
@@ -1548,8 +1553,7 @@ class Allocator {
}
void initRingBufferMaybe() {
- static HybridMutex RingBufferLock;
- ScopedLock L(RingBufferLock);
+ ScopedLock L(RingBufferInitLock);
if (getRingBuffer() != nullptr)
return;
>From ad8a9736c95ef0ed3ce94c9b12618c42a86b4039 Mon Sep 17 00:00:00 2001
From: Christopher Ferris <cferris at google.com>
Date: Thu, 28 Mar 2024 14:12:32 -0700
Subject: [PATCH 4/4] Make enable/disable symmetrical.
---
compiler-rt/lib/scudo/standalone/combined.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 023b10a07b4972..e7bc90cd0960ed 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -182,10 +182,10 @@ class Allocator {
}
void enableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
- RingBufferInitLock.unlock();
AllocationRingBuffer *RB = getRingBuffer();
if (RB)
RB->Depot->enable();
+ RingBufferInitLock.unlock();
}
void disableRingBuffer() NO_THREAD_SAFETY_ANALYSIS {
More information about the llvm-commits
mailing list