Author: Jonas Devlieghere Date: 2024-11-03T12:34:52-08:00 New Revision: 74b56c7eb807e2ba54bd7a2bcfda5d0bceff1c0c
URL: https://github.com/llvm/llvm-project/commit/74b56c7eb807e2ba54bd7a2bcfda5d0bceff1c0c DIFF: https://github.com/llvm/llvm-project/commit/74b56c7eb807e2ba54bd7a2bcfda5d0bceff1c0c.diff LOG: Revert "[lldb] Improve locking in PathMappingLists (NFC) (#114576)" This reverts commit c8209943faeead43c6932c5ddcc69c9e9d1e4262. Added: Modified: lldb/include/lldb/Target/PathMappingList.h lldb/source/Target/PathMappingList.cpp Removed: ################################################################################ diff --git a/lldb/include/lldb/Target/PathMappingList.h b/lldb/include/lldb/Target/PathMappingList.h index a59539eb0e4bc64..1c0ff564739c5ae 100644 --- a/lldb/include/lldb/Target/PathMappingList.h +++ b/lldb/include/lldb/Target/PathMappingList.h @@ -25,6 +25,7 @@ class PathMappingList { typedef void (*ChangedCallback)(const PathMappingList &path_list, void *baton); + // Constructors and Destructors PathMappingList(); PathMappingList(ChangedCallback callback, void *callback_baton); @@ -52,12 +53,12 @@ class PathMappingList { llvm::json::Value ToJSON(); bool IsEmpty() const { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); return m_pairs.empty(); } size_t GetSize() const { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); return m_pairs.size(); } @@ -136,33 +137,25 @@ class PathMappingList { uint32_t FindIndexForPath(llvm::StringRef path) const; uint32_t GetModificationID() const { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); return m_mod_id; } protected: + mutable std::recursive_mutex m_mutex; typedef std::pair<ConstString, ConstString> pair; typedef std::vector<pair> 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; - 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; + uint32_t m_mod_id = 0; // Incremented anytime anything is added or removed. }; } // namespace lldb_private diff --git a/lldb/source/Target/PathMappingList.cpp b/lldb/source/Target/PathMappingList.cpp index 81d10bdd53f9207..9c283b0146fe07d 100644 --- a/lldb/source/Target/PathMappingList.cpp +++ b/lldb/source/Target/PathMappingList.cpp @@ -48,10 +48,7 @@ PathMappingList::PathMappingList(const PathMappingList &rhs) const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) { if (this != &rhs) { - std::scoped_lock<std::mutex, std::mutex> callback_locks( - m_callback_mutex, rhs.m_callback_mutex); - std::scoped_lock<std::mutex, std::mutex> pairs_locks(m_pairs_mutex, - rhs.m_pairs_mutex); + std::scoped_lock<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex); m_pairs = rhs.m_pairs; m_callback = nullptr; m_callback_baton = nullptr; @@ -62,105 +59,85 @@ const PathMappingList &PathMappingList::operator=(const PathMappingList &rhs) { PathMappingList::~PathMappingList() = default; -void PathMappingList::AppendImpl(llvm::StringRef path, - llvm::StringRef replacement) { +void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement, + bool notify) { + std::lock_guard<std::recursive_mutex> lock(m_mutex); ++m_mod_id; m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement))); -} - -void PathMappingList::Notify(bool notify) const { - std::lock_guard<std::mutex> 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<std::mutex> lock(m_pairs_mutex); - AppendImpl(path, replacement); - } - Notify(notify); -} - void PathMappingList::Append(const PathMappingList &rhs, bool notify) { - { - std::scoped_lock<std::mutex, std::mutex> locks(m_pairs_mutex, - rhs.m_pairs_mutex); - ++m_mod_id; - if (rhs.m_pairs.empty()) - return; + std::scoped_lock<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex); + ++m_mod_id; + if (!rhs.m_pairs.empty()) { 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<std::mutex> 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); + std::lock_guard<std::recursive_mutex> lock(m_mutex); + for (const auto &pair : m_pairs) { + if (pair.first.GetStringRef() == normalized_path && + pair.second.GetStringRef() == normalized_replacement) + return false; } - Notify(notify); + Append(path, replacement, notify); return true; } void PathMappingList::Insert(llvm::StringRef path, llvm::StringRef replacement, uint32_t index, bool notify) { - { - std::lock_guard<std::mutex> 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); + std::lock_guard<std::recursive_mutex> 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); } bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement, uint32_t index, bool notify) { - { - std::lock_guard<std::mutex> lock(m_pairs_mutex); - if (index >= m_pairs.size()) - return false; - ++m_mod_id; - m_pairs[index] = pair(NormalizePath(path), NormalizePath(replacement)); - } - Notify(notify); + std::lock_guard<std::recursive_mutex> 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); return true; } bool PathMappingList::Remove(size_t index, bool notify) { - { - std::lock_guard<std::mutex> lock(m_pairs_mutex); - if (index >= m_pairs.size()) - return false; + std::lock_guard<std::recursive_mutex> lock(m_mutex); + if (index >= m_pairs.size()) + return false; - ++m_mod_id; - iterator iter = m_pairs.begin() + index; - m_pairs.erase(iter); - } - Notify(notify); + ++m_mod_id; + iterator iter = m_pairs.begin() + index; + m_pairs.erase(iter); + if (notify && m_callback) + m_callback(*this, m_callback_baton); 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<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); unsigned int numPairs = m_pairs.size(); if (pair_index < 0) { @@ -178,7 +155,7 @@ void PathMappingList::Dump(Stream *s, int pair_index) { llvm::json::Value PathMappingList::ToJSON() { llvm::json::Array entries; - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); for (const auto &pair : m_pairs) { llvm::json::Array entry{pair.first.GetStringRef().str(), pair.second.GetStringRef().str()}; @@ -188,13 +165,12 @@ llvm::json::Value PathMappingList::ToJSON() { } void PathMappingList::Clear(bool notify) { - { - std::lock_guard<std::mutex> lock(m_pairs_mutex); - if (!m_pairs.empty()) - ++m_mod_id; - m_pairs.clear(); - } - Notify(notify); + std::lock_guard<std::recursive_mutex> lock(m_mutex); + if (!m_pairs.empty()) + ++m_mod_id; + m_pairs.clear(); + if (notify && m_callback) + m_callback(*this, m_callback_baton); } bool PathMappingList::RemapPath(ConstString path, @@ -220,7 +196,7 @@ static void AppendPathComponents(FileSpec &path, llvm::StringRef components, std::optional<FileSpec> PathMappingList::RemapPath(llvm::StringRef mapping_path, bool only_if_exists) const { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); if (m_pairs.empty() || mapping_path.empty()) return {}; LazyBool path_is_relative = eLazyBoolCalculate; @@ -259,7 +235,7 @@ std::optional<llvm::StringRef> PathMappingList::ReverseRemapPath(const FileSpec &file, FileSpec &fixed) const { std::string path = file.GetPath(); llvm::StringRef path_ref(path); - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); for (const auto &it : m_pairs) { llvm::StringRef removed_prefix = it.second.GetStringRef(); if (!path_ref.consume_front(it.second.GetStringRef())) @@ -288,35 +264,34 @@ PathMappingList::FindFile(const FileSpec &orig_spec) const { bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef new_path, bool notify) { - { - std::lock_guard<std::mutex> lock(m_pairs_mutex); - uint32_t idx = FindIndexForPath(path); - if (idx >= m_pairs.size()) - return false; + std::lock_guard<std::recursive_mutex> lock(m_mutex); + uint32_t idx = FindIndexForPath(path); + if (idx < m_pairs.size()) { ++m_mod_id; m_pairs[idx].second = ConstString(new_path); + if (notify && m_callback) + m_callback(*this, m_callback_baton); + return true; } - Notify(notify); - return true; + return false; } bool PathMappingList::Remove(ConstString path, bool notify) { - { - std::lock_guard<std::mutex> lock(m_pairs_mutex); - iterator pos = FindIteratorForPath(path); - if (pos == m_pairs.end()) - return false; - + std::lock_guard<std::recursive_mutex> lock(m_mutex); + iterator pos = FindIteratorForPath(path); + if (pos != m_pairs.end()) { ++m_mod_id; m_pairs.erase(pos); + if (notify && m_callback) + m_callback(*this, m_callback_baton); + return true; } - Notify(notify); - return true; + return false; } PathMappingList::const_iterator PathMappingList::FindIteratorForPath(ConstString path) const { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); const_iterator pos; const_iterator begin = m_pairs.begin(); const_iterator end = m_pairs.end(); @@ -330,7 +305,7 @@ PathMappingList::FindIteratorForPath(ConstString path) const { PathMappingList::iterator PathMappingList::FindIteratorForPath(ConstString path) { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); iterator pos; iterator begin = m_pairs.begin(); iterator end = m_pairs.end(); @@ -344,7 +319,7 @@ PathMappingList::FindIteratorForPath(ConstString path) { bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path, ConstString &new_path) const { - std::lock_guard<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); if (idx < m_pairs.size()) { path = m_pairs[idx].first; new_path = m_pairs[idx].second; @@ -355,7 +330,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<std::mutex> lock(m_pairs_mutex); + std::lock_guard<std::recursive_mutex> lock(m_mutex); const_iterator pos; const_iterator begin = m_pairs.begin(); const_iterator end = m_pairs.end(); _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
