https://github.com/Teemperor created https://github.com/llvm/llvm-project/pull/219424
A SourceManager::File is shared by every Target and Process because Debugger and Process hand out cached instances (see SourceFileCache). m_offsets is the one member that is computed after the File was created. CalculateLineOffsets() indexes the file on the first access. Two threads that read the same source file at the same time therefore race in the current implementation. This patch guards m_offsets with a `Guarded` which avoids any potential races. The shared mutex allows concurrent accesses once the line offsets were calculated. assisted-by: claude >From d5fc2112dc99bc54f2e27388eb079a0e5535f61d Mon Sep 17 00:00:00 2001 From: Raphael Isemann <[email protected]> Date: Thu, 27 Aug 2026 15:28:32 +0100 Subject: [PATCH 1/2] [lldb] Add Guarded<T, Mutex> to Locked.h LLDB's code base has many variables that have an associated mutex that needs to be locked to safely access that variable from multiple threads. However, this locking scheme is currently not enforced by the compiler and code sometimes accesses these variables without aquiring the respective mutex first. This patch introduces a `Guarded` class that strictly enforces that some memory is only accessed after the respective mutex has been aquired. This class hands out `Locked` objects for every access which guarentee that the mutex is held as long as the variable is in scope. --- lldb/include/lldb/Utility/Locked.h | 28 ++++++++++++++++++++ lldb/unittests/Utility/LockedTest.cpp | 37 +++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/lldb/include/lldb/Utility/Locked.h b/lldb/include/lldb/Utility/Locked.h index acd34e454a918..f5d7b1aa8e4d9 100644 --- a/lldb/include/lldb/Utility/Locked.h +++ b/lldb/include/lldb/Utility/Locked.h @@ -168,6 +168,34 @@ template <typename T, typename Mutex = llvm::sys::RWMutex> using SharedLockedUP = SharedLocked<std::unique_ptr<const T>, Mutex>; /// @} +/// Bundles a value of type `T` with the `Mutex` that guards it. +/// +/// This class prevents accidential use of a value without aquiring the +/// lock and should be preferred over a member + mutex pair. +/// +/// `Mutex` must satisfy `Lockable` when calling `Lock()` and `SharedLockable` +/// when calling `LockShared()`. +template <typename T, typename Mutex = llvm::sys::RWMutex> class Guarded { +public: + Guarded() = default; + explicit Guarded(T value) : m_value(std::move(value)) {} + + Guarded(const Guarded &) = delete; + Guarded &operator=(const Guarded &) = delete; + + /// Exclusive (read/write) access to the value. + Locked<T *, Mutex> Lock() { return Locked<T *, Mutex>(m_mutex, &m_value); } + + /// Shared (read-only) access to the value. + SharedLocked<const T *, Mutex> LockShared() const { + return SharedLocked<const T *, Mutex>(m_mutex, &m_value); + } + +private: + mutable Mutex m_mutex; + T m_value{}; +}; + } // namespace lldb_private #endif // LLDB_UTILITY_LOCKED_H diff --git a/lldb/unittests/Utility/LockedTest.cpp b/lldb/unittests/Utility/LockedTest.cpp index cae24293dee2e..f898f21b56291 100644 --- a/lldb/unittests/Utility/LockedTest.cpp +++ b/lldb/unittests/Utility/LockedTest.cpp @@ -225,3 +225,40 @@ TEST(LockedTest, ExclusiveAccessOnRWMutex) { writer->value = 11; EXPECT_EQ(widget.value, 11); } + +// Guarded is neither copyable nor movable. +static_assert(!std::is_copy_constructible_v<Guarded<Widget>>); +static_assert(!std::is_move_constructible_v<Guarded<Widget>>); + +TEST(LockedTest, GuardedDefaultConstructed) { + Guarded<Widget> guarded; + EXPECT_EQ(guarded.Lock()->value, 0); +} + +TEST(LockedTest, GuardedValueConstructed) { + Guarded<Widget> guarded(Widget{42}); + EXPECT_EQ(guarded.Lock()->value, 42); +} + +TEST(LockedTest, GuardedExclusiveAccessMutatesValue) { + Guarded<Widget> guarded; + guarded.Lock()->value = 7; + EXPECT_EQ(guarded.Lock()->value, 7); +} + +TEST(LockedTest, GuardedSharedAccessIsReadOnly) { + Guarded<Widget> guarded(Widget{5}); + SharedLocked<const Widget *, llvm::sys::RWMutex> reader = + guarded.LockShared(); + EXPECT_EQ(reader->value, 5); + static_assert(std::is_same_v<decltype(reader.get()), const Widget *>, + "shared access borrows a const-qualified pointer"); +} + +// std::shared_mutex satisfies SharedLockable too, so Guarded works with it +// as a drop-in replacement for llvm::sys::RWMutex. +TEST(LockedTest, GuardedWorksWithStdSharedMutex) { + Guarded<Widget, std::shared_mutex> guarded; + guarded.Lock()->value = 3; + EXPECT_EQ(guarded.LockShared()->value, 3); +} >From 37f45b3b43009b15336164a6aef9a72d1da21693 Mon Sep 17 00:00:00 2001 From: Raphael Isemann <[email protected]> Date: Thu, 27 Aug 2026 15:28:55 +0100 Subject: [PATCH 2/2] [lldb] Guard SourceManager::File's line offsets with a mutex A SourceManager::File is shared by every Target and Process because Debugger and Process hand out cached instances (see SourceFileCache). m_offsets is the one member that is computed after the File was created. CalculateLineOffsets() indexes the file on the first access. Two threads that read the same source file at the same time therefore race in the current implementation. This patch guards m_offsets with a `Guarded` which avoids any potential races. The shared mutex allows concurrent accesses once the line offsets were calculated. assisted-by: claude --- lldb/include/lldb/Core/SourceManager.h | 9 ++++++- lldb/source/Core/SourceManager.cpp | 37 +++++++++++++++++--------- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/lldb/include/lldb/Core/SourceManager.h b/lldb/include/lldb/Core/SourceManager.h index 034f171f0a40d..e5b6f313ed2f7 100644 --- a/lldb/include/lldb/Core/SourceManager.h +++ b/lldb/include/lldb/Core/SourceManager.h @@ -11,6 +11,7 @@ #include "lldb/Utility/Checksum.h" #include "lldb/Utility/FileSpec.h" +#include "lldb/Utility/Locked.h" #include "lldb/Utility/SupportFile.h" #include "lldb/lldb-defines.h" #include "lldb/lldb-forward.h" @@ -23,6 +24,7 @@ #include <map> #include <memory> #include <optional> +#include <shared_mutex> #include <string> #include <vector> @@ -104,7 +106,12 @@ class SourceManager { uint32_t m_source_map_mod_id = 0; lldb::DataBufferSP m_data_sp; typedef std::vector<uint32_t> LineOffsets; - LineOffsets m_offsets; + + /// The line offsets for this file. + /// This member that is computed after this File was created, so write + /// access can happen from several threads.. + Guarded<LineOffsets, std::shared_mutex> m_offsets; + lldb::DebuggerWP m_debugger_wp; lldb::TargetWP m_target_wp; diff --git a/lldb/source/Core/SourceManager.cpp b/lldb/source/Core/SourceManager.cpp index 5ffedcc86ca1f..bec772fe584f6 100644 --- a/lldb/source/Core/SourceManager.cpp +++ b/lldb/source/Core/SourceManager.cpp @@ -611,15 +611,17 @@ uint32_t SourceManager::File::GetLineOffset(uint32_t line) { return 0; if (CalculateLineOffsets(line)) { - if (line < m_offsets.size()) - return m_offsets[line - 1]; // yes we want "line - 1" in the index + SharedLocked<const LineOffsets *, std::shared_mutex> offsets = + m_offsets.LockShared(); + if (line < offsets->size()) + return (*offsets)[line - 1]; // yes we want "line - 1" in the index } return UINT32_MAX; } uint32_t SourceManager::File::GetNumLines() { CalculateLineOffsets(); - return m_offsets.size(); + return m_offsets.LockShared()->size(); } const char *SourceManager::File::PeekLineData(uint32_t line) { @@ -669,7 +671,7 @@ bool SourceManager::File::LineIsValid(uint32_t line) { return false; if (CalculateLineOffsets(line)) - return line < m_offsets.size(); + return line < m_offsets.LockShared()->size(); return false; } @@ -782,11 +784,22 @@ bool SourceManager::File::CalculateLineOffsets(uint32_t line) { line = UINT32_MAX; // TODO: take this line out when we support partial indexing if (line == UINT32_MAX) { - // Already done? - if (!m_offsets.empty() && m_offsets[0] == UINT32_MAX) + // Already done? Check with just a reader lock first so concurrent reads + // of an already-indexed file don't serialize on each other. + { + SharedLocked<const LineOffsets *, std::shared_mutex> offsets = + m_offsets.LockShared(); + if (!offsets->empty() && (*offsets)[0] == UINT32_MAX) + return true; + } + + Locked<LineOffsets *, std::shared_mutex> offsets = m_offsets.Lock(); + // Another thread may have finished indexing while we were waiting for + // the writer lock. + if (!offsets->empty() && (*offsets)[0] == UINT32_MAX) return true; - if (m_offsets.empty()) { + if (offsets->empty()) { if (!m_data_sp) return false; @@ -798,7 +811,7 @@ bool SourceManager::File::CalculateLineOffsets(uint32_t line) { // Push a 1 at index zero to indicate the file has been completely // indexed. - m_offsets.push_back(UINT32_MAX); + offsets->push_back(UINT32_MAX); const char *s; for (s = start; s < end; ++s) { char curr_ch = *s; @@ -810,12 +823,12 @@ bool SourceManager::File::CalculateLineOffsets(uint32_t line) { ++s; } } - m_offsets.push_back(s + 1 - start); + offsets->push_back(s + 1 - start); } } - if (!m_offsets.empty()) { - if (m_offsets.back() < size_t(end - start)) - m_offsets.push_back(end - start); + if (!offsets->empty()) { + if (offsets->back() < size_t(end - start)) + offsets->push_back(end - start); } return true; } _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
