-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[scudo] Only init RingBuffer when needed. #85994
Conversation
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.
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Christopher Ferris (cferris1000) ChangesOnly 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. Full diff: https://github.com/llvm/llvm-project/pull/85994.diff 2 Files Affected:
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());
|
LGTM |
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.