[compiler-rt] [scudo] Add two missing locks to enable/disable. (PR #79670)
Evgenii Stepanov via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 29 13:25:30 PST 2024
https://github.com/eugenis updated https://github.com/llvm/llvm-project/pull/79670
>From cccdbb69ed588099aa1366c17514a1c4bfb97367 Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis at google.com>
Date: Fri, 26 Jan 2024 16:29:13 -0800
Subject: [PATCH 1/4] [scudo] Add two missing locks to enable/disable.
The StackDepot lock was actually observed deadlocking in the code that uses
malloc after fork but before exec. The other one was found by code
inspection.
---
compiler-rt/lib/scudo/standalone/combined.h | 2 ++
compiler-rt/lib/scudo/standalone/primary32.h | 2 ++
compiler-rt/lib/scudo/standalone/primary64.h | 2 ++
compiler-rt/lib/scudo/standalone/release.h | 16 ++++++++++++++++
compiler-rt/lib/scudo/standalone/stack_depot.h | 8 ++++++++
5 files changed, 30 insertions(+)
diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index 4624f83d142a0d..80774073522a72 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -691,10 +691,12 @@ class Allocator {
Quarantine.disable();
Primary.disable();
Secondary.disable();
+ Depot.disable();
}
void enable() NO_THREAD_SAFETY_ANALYSIS {
initThreadMaybe();
+ Depot.enable();
Secondary.enable();
Primary.enable();
Quarantine.enable();
diff --git a/compiler-rt/lib/scudo/standalone/primary32.h b/compiler-rt/lib/scudo/standalone/primary32.h
index 4d03b282d000de..89fffc50dcd009 100644
--- a/compiler-rt/lib/scudo/standalone/primary32.h
+++ b/compiler-rt/lib/scudo/standalone/primary32.h
@@ -261,6 +261,7 @@ template <typename Config> class SizeClassAllocator32 {
}
void disable() NO_THREAD_SAFETY_ANALYSIS {
+ RegionPageMap::disable();
// The BatchClassId must be locked last since other classes can use it.
for (sptr I = static_cast<sptr>(NumClasses) - 1; I >= 0; I--) {
if (static_cast<uptr>(I) == SizeClassMap::BatchClassId)
@@ -281,6 +282,7 @@ template <typename Config> class SizeClassAllocator32 {
continue;
getSizeClassInfo(I)->Mutex.unlock();
}
+ RegionPageMap::enable();
}
template <typename F> void iterateOverBlocks(F Callback) {
diff --git a/compiler-rt/lib/scudo/standalone/primary64.h b/compiler-rt/lib/scudo/standalone/primary64.h
index 9a642d23620e1e..4adb53b3748ffe 100644
--- a/compiler-rt/lib/scudo/standalone/primary64.h
+++ b/compiler-rt/lib/scudo/standalone/primary64.h
@@ -341,9 +341,11 @@ template <typename Config> class SizeClassAllocator64 {
}
getRegionInfo(SizeClassMap::BatchClassId)->MMLock.lock();
getRegionInfo(SizeClassMap::BatchClassId)->FLLock.lock();
+ RegionPageMap::disable();
}
void enable() NO_THREAD_SAFETY_ANALYSIS {
+ RegionPageMap::enable();
getRegionInfo(SizeClassMap::BatchClassId)->FLLock.unlock();
getRegionInfo(SizeClassMap::BatchClassId)->MMLock.unlock();
for (uptr I = 0; I < NumClasses; I++) {
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index b6f76a4d20585a..6e90ec97e0f3c3 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -168,6 +168,14 @@ class BufferPool {
return Buf.BufferIndex != StaticBufferCount;
}
+ void disable() NO_THREAD_SAFETY_ANALYSIS {
+ Mutex.lock();
+ }
+
+ void enable() NO_THREAD_SAFETY_ANALYSIS {
+ Mutex.unlock();
+ }
+
private:
Buffer getDynamicBuffer(const uptr NumElements) {
// When using a heap-based buffer, precommit the pages backing the
@@ -325,6 +333,14 @@ class RegionPageMap {
uptr getBufferNumElements() const { return BufferNumElements; }
+ static void disable() NO_THREAD_SAFETY_ANALYSIS {
+ Buffers.disable();
+ }
+
+ static void enable() NO_THREAD_SAFETY_ANALYSIS {
+ Buffers.enable();
+ }
+
private:
// We may consider making this configurable if there are cases which may
// benefit from this.
diff --git a/compiler-rt/lib/scudo/standalone/stack_depot.h b/compiler-rt/lib/scudo/standalone/stack_depot.h
index 12c35eb2a4f338..ab5e96680cab58 100644
--- a/compiler-rt/lib/scudo/standalone/stack_depot.h
+++ b/compiler-rt/lib/scudo/standalone/stack_depot.h
@@ -136,6 +136,14 @@ class StackDepot {
u64 operator[](uptr RingPos) const {
return atomic_load_relaxed(&Ring[RingPos & RingMask]);
}
+
+ void disable() NO_THREAD_SAFETY_ANALYSIS {
+ RingEndMu.lock();
+ }
+
+ void enable() NO_THREAD_SAFETY_ANALYSIS {
+ RingEndMu.unlock();
+ }
};
} // namespace scudo
>From 7a9ca1d9267e470b50d3122766064a0d204ecb6b Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis at google.com>
Date: Mon, 29 Jan 2024 10:08:12 -0800
Subject: [PATCH 2/4] fixup: clang-format
---
compiler-rt/lib/scudo/standalone/release.h | 16 ++++------------
compiler-rt/lib/scudo/standalone/stack_depot.h | 8 ++------
2 files changed, 6 insertions(+), 18 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index 6e90ec97e0f3c3..a444a2ce3dc9a8 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -168,13 +168,9 @@ class BufferPool {
return Buf.BufferIndex != StaticBufferCount;
}
- void disable() NO_THREAD_SAFETY_ANALYSIS {
- Mutex.lock();
- }
+ void disable() NO_THREAD_SAFETY_ANALYSIS { Mutex.lock(); }
- void enable() NO_THREAD_SAFETY_ANALYSIS {
- Mutex.unlock();
- }
+ void enable() NO_THREAD_SAFETY_ANALYSIS { Mutex.unlock(); }
private:
Buffer getDynamicBuffer(const uptr NumElements) {
@@ -333,13 +329,9 @@ class RegionPageMap {
uptr getBufferNumElements() const { return BufferNumElements; }
- static void disable() NO_THREAD_SAFETY_ANALYSIS {
- Buffers.disable();
- }
+ static void disable() NO_THREAD_SAFETY_ANALYSIS { Buffers.disable(); }
- static void enable() NO_THREAD_SAFETY_ANALYSIS {
- Buffers.enable();
- }
+ static void enable() NO_THREAD_SAFETY_ANALYSIS { Buffers.enable(); }
private:
// We may consider making this configurable if there are cases which may
diff --git a/compiler-rt/lib/scudo/standalone/stack_depot.h b/compiler-rt/lib/scudo/standalone/stack_depot.h
index ab5e96680cab58..bad30bdd85422d 100644
--- a/compiler-rt/lib/scudo/standalone/stack_depot.h
+++ b/compiler-rt/lib/scudo/standalone/stack_depot.h
@@ -137,13 +137,9 @@ class StackDepot {
return atomic_load_relaxed(&Ring[RingPos & RingMask]);
}
- void disable() NO_THREAD_SAFETY_ANALYSIS {
- RingEndMu.lock();
- }
+ void disable() NO_THREAD_SAFETY_ANALYSIS { RingEndMu.lock(); }
- void enable() NO_THREAD_SAFETY_ANALYSIS {
- RingEndMu.unlock();
- }
+ void enable() NO_THREAD_SAFETY_ANALYSIS { RingEndMu.unlock(); }
};
} // namespace scudo
>From ed28db52ea6233b0d6be878894df68e0076ddc87 Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis at google.com>
Date: Mon, 29 Jan 2024 13:19:17 -0800
Subject: [PATCH 3/4] Remove enable/disable logic for BufferPool.
Not needed because it is always taken under a Region's MMLock.
---
compiler-rt/lib/scudo/standalone/primary32.h | 2 --
compiler-rt/lib/scudo/standalone/primary64.h | 2 --
compiler-rt/lib/scudo/standalone/release.h | 8 --------
3 files changed, 12 deletions(-)
diff --git a/compiler-rt/lib/scudo/standalone/primary32.h b/compiler-rt/lib/scudo/standalone/primary32.h
index 89fffc50dcd009..4d03b282d000de 100644
--- a/compiler-rt/lib/scudo/standalone/primary32.h
+++ b/compiler-rt/lib/scudo/standalone/primary32.h
@@ -261,7 +261,6 @@ template <typename Config> class SizeClassAllocator32 {
}
void disable() NO_THREAD_SAFETY_ANALYSIS {
- RegionPageMap::disable();
// The BatchClassId must be locked last since other classes can use it.
for (sptr I = static_cast<sptr>(NumClasses) - 1; I >= 0; I--) {
if (static_cast<uptr>(I) == SizeClassMap::BatchClassId)
@@ -282,7 +281,6 @@ template <typename Config> class SizeClassAllocator32 {
continue;
getSizeClassInfo(I)->Mutex.unlock();
}
- RegionPageMap::enable();
}
template <typename F> void iterateOverBlocks(F Callback) {
diff --git a/compiler-rt/lib/scudo/standalone/primary64.h b/compiler-rt/lib/scudo/standalone/primary64.h
index 4adb53b3748ffe..9a642d23620e1e 100644
--- a/compiler-rt/lib/scudo/standalone/primary64.h
+++ b/compiler-rt/lib/scudo/standalone/primary64.h
@@ -341,11 +341,9 @@ template <typename Config> class SizeClassAllocator64 {
}
getRegionInfo(SizeClassMap::BatchClassId)->MMLock.lock();
getRegionInfo(SizeClassMap::BatchClassId)->FLLock.lock();
- RegionPageMap::disable();
}
void enable() NO_THREAD_SAFETY_ANALYSIS {
- RegionPageMap::enable();
getRegionInfo(SizeClassMap::BatchClassId)->FLLock.unlock();
getRegionInfo(SizeClassMap::BatchClassId)->MMLock.unlock();
for (uptr I = 0; I < NumClasses; I++) {
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index a444a2ce3dc9a8..b6f76a4d20585a 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -168,10 +168,6 @@ class BufferPool {
return Buf.BufferIndex != StaticBufferCount;
}
- void disable() NO_THREAD_SAFETY_ANALYSIS { Mutex.lock(); }
-
- void enable() NO_THREAD_SAFETY_ANALYSIS { Mutex.unlock(); }
-
private:
Buffer getDynamicBuffer(const uptr NumElements) {
// When using a heap-based buffer, precommit the pages backing the
@@ -329,10 +325,6 @@ class RegionPageMap {
uptr getBufferNumElements() const { return BufferNumElements; }
- static void disable() NO_THREAD_SAFETY_ANALYSIS { Buffers.disable(); }
-
- static void enable() NO_THREAD_SAFETY_ANALYSIS { Buffers.enable(); }
-
private:
// We may consider making this configurable if there are cases which may
// benefit from this.
>From 65825c722c3aabb9c1e162edd59277d7c757ea26 Mon Sep 17 00:00:00 2001
From: Evgenii Stepanov <eugenis at google.com>
Date: Mon, 29 Jan 2024 13:23:48 -0800
Subject: [PATCH 4/4] Add comment explaining disable in StackDepot.
---
compiler-rt/lib/scudo/standalone/stack_depot.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/compiler-rt/lib/scudo/standalone/stack_depot.h b/compiler-rt/lib/scudo/standalone/stack_depot.h
index bad30bdd85422d..e887d1b43a7cf0 100644
--- a/compiler-rt/lib/scudo/standalone/stack_depot.h
+++ b/compiler-rt/lib/scudo/standalone/stack_depot.h
@@ -137,6 +137,9 @@ class StackDepot {
return atomic_load_relaxed(&Ring[RingPos & RingMask]);
}
+ // This is done for the purpose of fork safety in multithreaded programs and
+ // does not fully disable StackDepot. In particular, find() still works and
+ // only insert() is blocked.
void disable() NO_THREAD_SAFETY_ANALYSIS { RingEndMu.lock(); }
void enable() NO_THREAD_SAFETY_ANALYSIS { RingEndMu.unlock(); }
More information about the llvm-commits
mailing list