[lldb] Improve locking in PathMappingLists (NFC)

[JDevlieghere]

In D148380, 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 in turns has a callback to Target::SetExecutableModule.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

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

---

Full diff: https://github.com/llvm/llvm-project/pull/114576.diff

2 Files Affected:

• (modified) lldb/include/lldb/Target/PathMappingList.h (+13-6)
• (modified) lldb/source/Target/PathMappingList.cpp (+94-69)

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();

[bulbazord]

It took me some time to read this and think about the implications, but I think this is generally okay. I left 1 comment about the order of lock acquisition for operator=.

The only way I can see this going wrong is if a callback can mutate the list in a way that triggers yet another callback? Not sure we should be too concerned about that right now though.

[JDevlieghere]

Seems like this might be causing GreenDragon to time out. Reverted to see if that resolves the issue. I also had to revert #114507 as it depends on this.

[kastiglione]

@JDevlieghere It seems these two lines lead to deadlock. FindIndexForPath attempts to lock m_pairs_mutex too, but can't.

[JDevlieghere]

Yup, I fixed the issue in the re-land. @kastiglione confirmed offline that that fixed the deadlock he observed.