[compiler-rt] [scudo] Only init RingBuffer when needed. (PR #85994)

Christopher Ferris via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 27 20:17:52 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/3] [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/3] 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/3] 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;
 



More information about the llvm-commits mailing list