[Lldb-commits] [lldb] [lldb] Improve locking in PathMappingLists (NFC) (PR #114576)
Jonas Devlieghere via lldb-commits
lldb-commits at lists.llvm.org
Fri Nov 1 16:20:14 PDT 2024
https://github.com/JDevlieghere updated https://github.com/llvm/llvm-project/pull/114576
>From 9ebfdc84c46a795d35b4dd157573b97a57b232ce Mon Sep 17 00:00:00 2001
From: Jonas Devlieghere <jonas at devlieghere.com>
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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> lock(m_pairs_mutex);
return m_pairs.empty();
}
size_t GetSize() const {
- std::lock_guard<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> lock(m_pairs_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;
- 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<std::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex);
+ std::scoped_lock<std::mutex, std::mutex> pairs_locks(m_pairs_mutex,
+ rhs.m_pairs_mutex);
+ std::scoped_lock<std::mutex, std::mutex> 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<std::recursive_mutex> 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<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::recursive_mutex, std::recursive_mutex> locks(m_mutex, rhs.m_mutex);
- ++m_mod_id;
- if (!rhs.m_pairs.empty()) {
+ {
+ std::scoped_lock<std::mutex, std::mutex> 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<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;
+ {
+ 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);
}
- 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<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);
+ {
+ 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);
}
bool PathMappingList::Replace(llvm::StringRef path, llvm::StringRef replacement,
uint32_t index, bool 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);
+ {
+ 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);
return true;
}
bool PathMappingList::Remove(size_t index, bool notify) {
- std::lock_guard<std::recursive_mutex> lock(m_mutex);
- if (index >= m_pairs.size())
- return false;
+ {
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<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);
+ {
+ std::lock_guard<std::mutex> 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<FileSpec> PathMappingList::RemapPath(llvm::StringRef mapping_path,
bool only_if_exists) const {
- std::lock_guard<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> lock(m_pairs_mutex);
if (m_pairs.empty() || mapping_path.empty())
return {};
LazyBool path_is_relative = eLazyBoolCalculate;
@@ -235,7 +259,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::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
- uint32_t idx = FindIndexForPath(path);
- if (idx < m_pairs.size()) {
+ {
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
- iterator pos = FindIteratorForPath(path);
- if (pos != m_pairs.end()) {
+ {
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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<std::recursive_mutex> lock(m_mutex);
+ std::lock_guard<std::mutex> 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 <jonas at devlieghere.com>
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<std::mutex, std::mutex> pairs_locks(m_pairs_mutex,
- rhs.m_pairs_mutex);
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);
m_pairs = rhs.m_pairs;
m_callback = nullptr;
m_callback_baton = nullptr;
More information about the lldb-commits
mailing list