[lldb] Replace PathMappingList::GetPathsAtIndex with thread-safe alternative

[bulbazord]

Iteration by index is not thread-safe with PathMapingList's design. Instead, we can expose a ForEach method that grabs the PathMappingList's lock before the callback.

There is a potential pitfall here where the callback tries to call another PathMappingList method (causing a deadlock), but that seems a little easier to debug than the PathMappingList getting updated in the middle of a loop somewhere.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Alex Langford (bulbazord)

Changes

Iteration by index is not thread-safe with PathMapingList's design. Instead, we can expose a ForEach method that grabs the PathMappingList's lock before the callback.

There is a potential pitfall here where the callback tries to call another PathMappingList method (causing a deadlock), but that seems a little easier to debug than the PathMappingList getting updated in the middle of a loop somewhere.

---

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

3 Files Affected:

• (modified) lldb/include/lldb/Target/PathMappingList.h (+4-2)
• (modified) lldb/source/Commands/CommandObjectTarget.cpp (+11-9)
• (modified) lldb/source/Target/PathMappingList.cpp (+5-8)

diff --git a/lldb/include/lldb/Target/PathMappingList.h b/lldb/include/lldb/Target/PathMappingList.h
index 2cf9569358f52..02fee4d8ed45f 100644
--- a/lldb/include/lldb/Target/PathMappingList.h
+++ b/lldb/include/lldb/Target/PathMappingList.h
@@ -61,8 +61,10 @@ class PathMappingList {
     return m_pairs.size();
   }
 
-  bool GetPathsAtIndex(uint32_t idx, ConstString &path,
-                       ConstString &new_path) const;
+  /// Invokes callback for each pair of paths in the list. The callback can
+  /// return false to immediately stop iteration.
+  void
+  ForEach(std::function<bool(llvm::StringRef, llvm::StringRef)> callback) const;
 
   void Insert(llvm::StringRef path, llvm::StringRef replacement,
               uint32_t insert_idx, bool notify);
diff --git a/lldb/source/Commands/CommandObjectTarget.cpp b/lldb/source/Commands/CommandObjectTarget.cpp
index fe421adfe8709..e8ebcb8008ff9 100644
--- a/lldb/source/Commands/CommandObjectTarget.cpp
+++ b/lldb/source/Commands/CommandObjectTarget.cpp
@@ -1139,15 +1139,17 @@ class CommandObjectTargetModulesSearchPathsInsert : public CommandObjectParsed {
 
     Target *target = m_exe_ctx.GetTargetPtr();
     const PathMappingList &list = target->GetImageSearchPathList();
-    const size_t num = list.GetSize();
-    ConstString old_path, new_path;
-    for (size_t i = 0; i < num; ++i) {
-      if (!list.GetPathsAtIndex(i, old_path, new_path))
-        break;
-      StreamString strm;
-      strm << old_path << " -> " << new_path;
-      request.TryCompleteCurrentArg(std::to_string(i), strm.GetString());
-    }
+
+    StreamString strm;
+    size_t index = 0;
+    list.ForEach([&](llvm::StringRef orig_path, llvm::StringRef remap_path) {
+      strm << orig_path << " -> " << remap_path;
+      request.TryCompleteCurrentArg(std::to_string(index), strm.GetString());
+
+      index++;
+      strm.Clear();
+      return true;
+    });
   }
 
 protected:
diff --git a/lldb/source/Target/PathMappingList.cpp b/lldb/source/Target/PathMappingList.cpp
index 0465b4280bbb1..d8dd9dde7a307 100644
--- a/lldb/source/Target/PathMappingList.cpp
+++ b/lldb/source/Target/PathMappingList.cpp
@@ -346,15 +346,12 @@ PathMappingList::FindIteratorForPath(ConstString path) {
   return pos;
 }
 
-bool PathMappingList::GetPathsAtIndex(uint32_t idx, ConstString &path,
-                                      ConstString &new_path) const {
+void PathMappingList::ForEach(
+    std::function<bool(llvm::StringRef, llvm::StringRef)> callback) const {
   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;
-    return true;
-  }
-  return false;
+  for (const auto &[original, replacement] : m_pairs)
+    if (!callback(original, replacement))
+      break;
 }
 
 uint32_t

[JDevlieghere]

Is this method used a lot? Could we migrate to using the LockingAdaptedIterable instead of match how other classes handle this?

[bulbazord]

Only used in one place. I'll look at LockingAdaptedIterable and see how bad it is.