[lldb] Improve locking in PathMappingLists (NFC) (llvm#114576)

In [D148380](https://reviews.llvm.org/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 llvm#114507. Specifically,
Target::SetExecutableModule calls Target::GetOrCreateModule, which
potentially performs path remapping, which in turns has a callback to
Target::SetExecutableModule.

(cherry picked from commit 8f8e2b7)

Author: JDevlieghere

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();
   }
 
@@ -134,28 +133,35 @@ class PathMappingList {
   ///     The newly remapped filespec that is guaranteed to exist.
   std::optional<FileSpec> FindFile(const FileSpec &orig_spec) const;
 
-  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 AppendNoLock(llvm::StringRef path, llvm::StringRef replacement);
+  uint32_t FindIndexForPathNoLock(llvm::StringRef path) const;
+  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

lldb/source/Target/PathMappingList.cpp

@@ -48,7 +48,8 @@ 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, std::mutex> locks(
+        m_callback_mutex, m_pairs_mutex, rhs.m_pairs_mutex);
     m_pairs = rhs.m_pairs;
     m_callback = nullptr;
     m_callback_baton = nullptr;
@@ -59,85 +60,111 @@ 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::AppendNoLock(llvm::StringRef path,
+                                   llvm::StringRef replacement) {
   ++m_mod_id;
   m_pairs.emplace_back(pair(NormalizePath(path), NormalizePath(replacement)));
-  if (notify && m_callback)
-    m_callback(*this, m_callback_baton);
+}
+
+void PathMappingList::Notify(bool notify) const {
+  ChangedCallback callback = nullptr;
+  void *baton = nullptr;
+  {
+    std::lock_guard<std::mutex> lock(m_callback_mutex);
+    callback = m_callback;
+    baton = m_callback_baton;
+  }
+  if (notify && callback)
+    callback(*this, baton);
+}
+
+void PathMappingList::Append(llvm::StringRef path, llvm::StringRef replacement,
+                             bool notify) {
+  {
+    std::lock_guard<std::mutex> lock(m_pairs_mutex);
+    AppendNoLock(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;
+    }
+    AppendNoLock(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 +182,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 +192,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 +224,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 +263,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 +292,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 = FindIndexForPathNoLock(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 +334,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 +348,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;
@@ -328,9 +357,9 @@ bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path,
   return false;
 }
 
-uint32_t PathMappingList::FindIndexForPath(llvm::StringRef orig_path) const {
+uint32_t
+PathMappingList::FindIndexForPathNoLock(llvm::StringRef orig_path) const {
   const ConstString path = ConstString(NormalizePath(orig_path));
-  std::lock_guard<std::recursive_mutex> lock(m_mutex);
   const_iterator pos;
   const_iterator begin = m_pairs.begin();
   const_iterator end = m_pairs.end();