From 9ebfdc84c46a795d35b4dd157573b97a57b232ce Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 1 Nov 2024 09:58:28 -0700 Subject: [PATCH 1/2] [lldb] Improve locking in PathMappingLists (NFC) In D148380 [1], Alex added locking to PathMappingLists. The current implementation runs the callback under the lock, which I don't believe is necessary. As far as I can tell, no users of the callback are relying on the list not having been modified until the callback is handled. This patch implements my suggestion to unlock the mutex before the callback. I also switched to a non-recursive mutex as I don't believe the recursive property is needed. To make the class fully thread safe, I did have to introduce another mutex to protect the callback members. The motivation for this change is #114507. Specifically, Target::SetExecutableModule calls Target::GetOrCreateModule, which potentially performs path remapping, which has a callback to Target::SetExecutableModule. [1] https://reviews.llvm.org/D148380 --- lldb/include/lldb/Target/PathMappingList.h | 19 ++- lldb/source/Target/PathMappingList.cpp | 163 ++++++++++++--------- 2 files changed, 107 insertions(+), 75 deletions(-) diff --git a/lldb/include/lldb/Target/PathMappingList.h b/lldb/include/lldb/Target/PathMappingList.h index 1c0ff564739c5a..a59539eb0e4bc6 100644 --- a/lldb/include/lldb/Target/PathMappingList.h +++ b/lldb/include/lldb/Target/PathMappingList.h @@ -25,7 +25,6 @@ class PathMappingList { typedef void (*ChangedCallback)(const PathMappingList &path_list, void *baton); - // Constructors and Destructors PathMappingList(); PathMappingList(ChangedCallback callback, void *callback_baton); @@ -53,12 +52,12 @@ class PathMappingList { llvm::json::Value ToJSON(); bool IsEmpty() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); return m_pairs.empty(); } size_t GetSize() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); return m_pairs.size(); } @@ -137,25 +136,33 @@ class PathMappingList { uint32_t FindIndexForPath(llvm::StringRef path) const; uint32_t GetModificationID() const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); return m_mod_id; } protected: - mutable std::recursive_mutex m_mutex; typedef std::pair pair; typedef std::vector collection; typedef collection::iterator iterator; typedef collection::const_iterator const_iterator; + void AppendImpl(llvm::StringRef path, llvm::StringRef replacement); + void Notify(bool notify) const; + iterator FindIteratorForPath(ConstString path); const_iterator FindIteratorForPath(ConstString path) const; collection m_pairs; + mutable std::mutex m_pairs_mutex; + ChangedCallback m_callback = nullptr; void *m_callback_baton = nullptr; - uint32_t m_mod_id = 0; // Incremented anytime anything is added or removed. + mutable std::mutex m_callback_mutex; + + /// Incremented anytime anything is added to or removed from m_pairs. Also + /// protected by m_pairs_mutex. + uint32_t m_mod_id = 0; }; } // namespace lldb_private diff --git a/lldb/source/Target/PathMappingList.cpp b/lldb/source/Target/PathMappingList.cpp index 9c283b0146fe07..7343522fafef20 100644 --- a/lldb/source/Target/PathMappingList.cpp +++ b/lldb/source/Target/PathMappingList.cpp @@ -48,7 +48,10 @@ PathMappingList::PathMappingList(const PathMappingList &rhs) const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) { if (this != &rhs) { - std::scoped_lock locks(m_mutex, rhs.m_mutex); + std::scoped_lock pairs_locks(m_pairs_mutex, + rhs.m_pairs_mutex); + std::scoped_lock callback_locks( + m_callback_mutex, rhs.m_callback_mutex); m_pairs = rhs.m_pairs; m_callback = nullptr; m_callback_baton = nullptr; @@ -59,85 +62,105 @@ const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) { PathMappingList::~PathMappingList() = default; -void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement, - bool notify) { - std::lock_guard lock(m_mutex); +void PathMappingList::AppendImpl(llvm::StringRef path, + llvm::StringRef replacement) { ++m_mod_id; m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement))); +} + +void PathMappingList::Notify(bool notify) const { + std::lock_guard lock(m_callback_mutex); if (notify && m_callback) m_callback(*this, m_callback_baton); } +void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement, + bool notify) { + { + std::lock_guard lock(m_pairs_mutex); + AppendImpl(path, replacement); + } + Notify(notify); +} + void PathMappingList::Append(const PathMappingList &rhs, bool notify) { - std::scoped_lock locks(m_mutex, rhs.m_mutex); - ++m_mod_id; - if (!rhs.m_pairs.empty()) { + { + std::scoped_lock locks(m_pairs_mutex, + rhs.m_pairs_mutex); + ++m_mod_id; + if (rhs.m_pairs.empty()) + return; const_iterator pos, end = rhs.m_pairs.end(); for (pos = rhs.m_pairs.begin(); pos != end; ++pos) m_pairs.push_back(*pos); - if (notify && m_callback) - m_callback(*this, m_callback_baton); } + Notify(notify); } bool PathMappingList::AppendUnique(llvm::StringRef path, llvm::StringRef replacement, bool notify) { auto normalized_path = NormalizePath(path); auto normalized_replacement = NormalizePath(replacement); - std::lock_guard lock(m_mutex); - for (const auto &pair : m_pairs) { - if (pair.first.GetStringRef() == normalized_path && - pair.second.GetStringRef() == normalized_replacement) - return false; + { + std::lock_guard lock(m_pairs_mutex); + for (const auto &pair : m_pairs) { + if (pair.first.GetStringRef() == normalized_path && + pair.second.GetStringRef() == normalized_replacement) + return false; + } + AppendImpl(path, replacement); } - Append(path, replacement, notify); + Notify(notify); return true; } void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement, uint32_t index, bool notify) { - std::lock_guard lock(m_mutex); - ++m_mod_id; - iterator insert_iter; - if (index >= m_pairs.size()) - insert_iter = m_pairs.end(); - else - insert_iter = m_pairs.begin() + index; - m_pairs.emplace(insert_iter, pair(NormalizePath(path), - NormalizePath(replacement))); - if (notify && m_callback) - m_callback(*this, m_callback_baton); + { + std::lock_guard lock(m_pairs_mutex); + ++m_mod_id; + iterator insert_iter; + if (index >= m_pairs.size()) + insert_iter = m_pairs.end(); + else + insert_iter = m_pairs.begin() + index; + m_pairs.emplace(insert_iter, + pair(NormalizePath(path), NormalizePath(replacement))); + } + Notify(notify); } bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement, uint32_t index, bool notify) { - std::lock_guard lock(m_mutex); - if (index >= m_pairs.size()) - return false; - ++m_mod_id; - m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement)); - if (notify && m_callback) - m_callback(*this, m_callback_baton); + { + std::lock_guard lock(m_pairs_mutex); + if (index >= m_pairs.size()) + return false; + ++m_mod_id; + m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement)); + } + Notify(notify); return true; } bool PathMappingList::Remove(size_t index, bool notify) { - std::lock_guard lock(m_mutex); - if (index >= m_pairs.size()) - return false; + { + std::lock_guard lock(m_pairs_mutex); + if (index >= m_pairs.size()) + return false; - ++m_mod_id; - iterator iter = m_pairs.begin() + index; - m_pairs.erase(iter); - if (notify && m_callback) - m_callback(*this, m_callback_baton); + ++m_mod_id; + iterator iter = m_pairs.begin() + index; + m_pairs.erase(iter); + } + Notify(notify); return true; } // For clients which do not need the pair index dumped, pass a pair_index >= 0 // to only dump the indicated pair. void PathMappingList::Dump(Stream *s, int pair_index) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); unsigned int numPairs = m_pairs.size(); if (pair_index < 0) { @@ -155,7 +178,7 @@ void PathMappingList::Dump(Stream *s, int pair_index) { llvm::json::Value PathMappingList::ToJSON() { llvm::json::Array entries; - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); for (const auto &pair : m_pairs) { llvm::json::Array entry{pair.first.GetStringRef().str(), pair.second.GetStringRef().str()}; @@ -165,12 +188,13 @@ llvm::json::Value PathMappingList::ToJSON() { } void PathMappingList::Clear(bool notify) { - std::lock_guard lock(m_mutex); - if (!m_pairs.empty()) - ++m_mod_id; - m_pairs.clear(); - if (notify && m_callback) - m_callback(*this, m_callback_baton); + { + std::lock_guard lock(m_pairs_mutex); + if (!m_pairs.empty()) + ++m_mod_id; + m_pairs.clear(); + } + Notify(notify); } bool PathMappingList::RemapPath(ConstString path, @@ -196,7 +220,7 @@ static void AppendPathComponents(FileSpec &path, llvm::StringRef components, std::optional PathMappingList::RemapPath(llvm::StringRef mapping_path, bool only_if_exists) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); if (m_pairs.empty() || mapping_path.empty()) return {}; LazyBool path_is_relative = eLazyBoolCalculate; @@ -235,7 +259,7 @@ std::optional PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const { std::string path = file.GetPath(); llvm::StringRef path_ref(path); - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); for (const auto &it : m_pairs) { llvm::StringRef removed_prefix = it.second.GetStringRef(); if (!path_ref.consume_front(it.second.GetStringRef())) @@ -264,34 +288,35 @@ PathMappingList::FindFile(const FileSpec &orig_spec) const { bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path, bool notify) { - std::lock_guard lock(m_mutex); - uint32_t idx = FindIndexForPath(path); - if (idx < m_pairs.size()) { + { + std::lock_guard lock(m_pairs_mutex); + uint32_t idx = FindIndexForPath(path); + if (idx >= m_pairs.size()) + return false; ++m_mod_id; m_pairs[idx].second = ConstString(new_path); - if (notify && m_callback) - m_callback(*this, m_callback_baton); - return true; } - return false; + Notify(notify); + return true; } bool PathMappingList::Remove(ConstString path, bool notify) { - std::lock_guard lock(m_mutex); - iterator pos = FindIteratorForPath(path); - if (pos != m_pairs.end()) { + { + std::lock_guard lock(m_pairs_mutex); + iterator pos = FindIteratorForPath(path); + if (pos == m_pairs.end()) + return false; + ++m_mod_id; m_pairs.erase(pos); - if (notify && m_callback) - m_callback(*this, m_callback_baton); - return true; } - return false; + Notify(notify); + return true; } PathMappingList::const_iterator PathMappingList::FindIteratorForPath(ConstString path) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); const_iterator pos; const_iterator begin = m_pairs.begin(); const_iterator end = m_pairs.end(); @@ -305,7 +330,7 @@ PathMappingList::FindIteratorForPath(ConstString path) const { PathMappingList::iterator PathMappingList::FindIteratorForPath(ConstString path) { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); iterator pos; iterator begin = m_pairs.begin(); iterator end = m_pairs.end(); @@ -319,7 +344,7 @@ PathMappingList::FindIteratorForPath(ConstString path) { bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path, ConstString &new_path) const { - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); if (idx < m_pairs.size()) { path = m_pairs[idx].first; new_path = m_pairs[idx].second; @@ -330,7 +355,7 @@ bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path, uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const { const ConstString path = ConstString(NormalizePath(orig_path)); - std::lock_guard lock(m_mutex); + std::lock_guard lock(m_pairs_mutex); const_iterator pos; const_iterator begin = m_pairs.begin(); const_iterator end = m_pairs.end(); From 081c9c15bb959897a26eb87423c786b38213cacb Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere Date: Fri, 1 Nov 2024 16:20:00 -0700 Subject: [PATCH 2/2] Invert order in which we acquire mutexes in operator= --- lldb/source/Target/PathMappingList.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lldb/source/Target/PathMappingList.cpp b/lldb/source/Target/PathMappingList.cpp index 7343522fafef20..81d10bdd53f920 100644 --- a/lldb/source/Target/PathMappingList.cpp +++ b/lldb/source/Target/PathMappingList.cpp @@ -48,10 +48,10 @@ PathMappingList::PathMappingList(const PathMappingList &rhs) const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) { if (this != &rhs) { - std::scoped_lock pairs_locks(m_pairs_mutex, - rhs.m_pairs_mutex); std::scoped_lock callback_locks( m_callback_mutex, rhs.m_callback_mutex); + std::scoped_lock pairs_locks(m_pairs_mutex, + rhs.m_pairs_mutex); m_pairs = rhs.m_pairs; m_callback = nullptr; m_callback_baton = nullptr;