[lldb] Make ModuleList iterable (NFC)

[JDevlieghere]

Summary

Add begin and end iterators to the ModuleList so we can use range-based for loops without locking from beginning to end. This is what makes it different from the Modules() method which returns a LockingAdaptedIterable but also means the burden of correctness is on the caller.

I'm not sure if this distinction matters, but it preserves the semantics of the code that is currently using GetModuleAtIndex directly from inside a classically indexed for loop.

[dblaikie]

lldb/include/lldb/Core/ModuleList.h

70

FWIW, std::iterator is deprecated since C++17 - probably best not to add new uses of it. (I think the idea is that the typedefs that std::iterator provides are no longer needed because of auto/decltype/etc, but I might be wrong there (would have to check the iterator concepts specifications))

If you need some utility help with implementing an iterator, llvm's iterator_facade_base might help by allowing a fairly minimal implementation to autogenerate various symmetric members, etc.

[JDevlieghere]

Use iterator_facade_base as suggested by @dblaikie.

[dblaikie]

lldb/include/lldb/Core/ModuleList.h

71

On the fence, but this could be a random access iterator, since it's only based on an integer index - would be fairly easy to implement, but I can appreciate not wanting to add more iterator surface area than you need for range-based-for loops.

76

Returning const value types is particularly weird/problematic in some ways - it prevents any moving from the return value. So should probably drop the "const" here.

93–96

Seems you don't need to implement != - iterator_facade_base will provide that for you based on op==, I think?

98

This being a reference (rather than, say, a pointer) would prevent the iterator from being assignable, I think? Which isn't ideal for many iterator algorithms, etc.

[JDevlieghere]

Address Dave's feedback.

[dblaikie]

Looks good to me with a few minor cleanups.

lldb/include/lldb/Core/ModuleList.h

73–74

Interesting choice to make the single-arg ctor implicit but the multi-arg explicit (usually people end up the other way around - because until C++ 17 or something, explicit was only relevant for single-arg constructors). In any case the ctors are only used once each & probably might as well have them both explicit.

Or maybe make only one ctor that takes the index explicitly and use it from begin/end - not sure there's much value added by having two ctors here, and honestly I'd have expected the no-arg version to start at zero, and the arg-taking version to start wherever you want, and I see they're the opposite, so that'd probably reinforce the idea that just having the one that takes an explicit index might be less confusing.

78–87

I think you can drop these two now - they'll be provided by the iterator facade.

[JDevlieghere]

lldb/include/lldb/Core/ModuleList.h

73–74

I copied all of this from somewhere else in LLDB without giving it much thought because honestly I'm not sure this iterator is that much better than then existing LockingAdaptedIterable. Sorry for making you review this half-baked code...

[JDevlieghere]

Abandoning this in favor of using the ModuleIterable everywhere: https://reviews.llvm.org/D94271

lldb/include/lldb/Core/ModuleList.h

 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/Support/RWMutex.h"
 
 #include <functional>
+#include <limits>
 #include <list>
 #include <mutex>
 #include <vector>
 
 #include <stddef.h>
 #include <stdint.h>
 
 namespace lldb_private {
public:
   FileSpec GetClangModulesCachePath() const;
   bool SetClangModulesCachePath(const FileSpec &path);
   bool GetEnableExternalLookup() const;
   bool SetEnableExternalLookup(bool new_value);
 
   PathMappingList GetSymlinkMappings() const;
 };
 
+class ModuleList;
+
+class ModuleIterator
+    : public std::iterator<std::bidirectional_iterator_tag, lldb::ModuleSP> {
+public:
+  explicit ModuleIterator(const ModuleList &module_list, size_t idx);
+  ModuleIterator(const ModuleList &module_list);
+
+  const lldb::ModuleSP operator*() const;
+
+  ModuleIterator &operator++() {
+    ++m_index;
+    return *this;
+  }
+
+  ModuleIterator &operator--() {
+    --m_index;
+    return *this;
+  }
+
+  friend bool operator==(const ModuleIterator &lhs, const ModuleIterator &rhs) {
+    return &lhs.m_module_list == &rhs.m_module_list &&
+           lhs.m_index == rhs.m_index;
+  }
+
+  friend bool operator!=(const ModuleIterator &lhs, const ModuleIterator &rhs) {
+    return !(lhs == rhs);
+  }
+
+private:
+  const ModuleList &m_module_list;
+  size_t m_index;
+};
+
 /// \class ModuleList ModuleList.h "lldb/Core/ModuleList.h"
 /// A collection class for Module objects.
 ///
 /// Modules in the module collection class are stored as reference counted
 /// shared pointers to Module objects.
 class ModuleList {
 public:
   class Notifier {
public:
 
   static size_t RemoveOrphanSharedModules(bool mandatory);
 
   static bool RemoveSharedModuleIfOrphaned(const Module *module_ptr);
 
   void ForEach(std::function<bool(const lldb::ModuleSP &module_sp)> const
                    &callback) const;
 
+  typedef ModuleIterator iterator;
+  typedef ModuleIterator const_iterator;
+
+  ModuleIterator begin() const { return ModuleIterator(*this, 0); }
+  ModuleIterator end() const { return ModuleIterator(*this); }
+
 protected:
   // Class typedefs.
   typedef std::vector<lldb::ModuleSP>
       collection; ///< The module collection type.
 
   void AppendImpl(const lldb::ModuleSP &module_sp, bool use_notifier = true);
 
   bool RemoveImpl(const lldb::ModuleSP &module_sp, bool use_notifier = true);

lldb/source/Core/ModuleList.cpp

void ModuleListProperties::UpdateSymlinkMappings() {
   }
 }
 
 PathMappingList ModuleListProperties::GetSymlinkMappings() const {
   llvm::sys::ScopedReader lock(m_symlink_paths_mutex);
   return m_symlink_paths;
 }
 
+ModuleIterator::ModuleIterator(const ModuleList &module_list, size_t idx)
+    : m_module_list(module_list), m_index(idx) {}
+
+ModuleIterator::ModuleIterator(const ModuleList &module_list)
+    : m_module_list(module_list), m_index(module_list.GetSize()) {}
+
+const lldb::ModuleSP ModuleIterator::operator*() const {
+  return m_module_list.GetModuleAtIndex(m_index);
+}
+
 ModuleList::ModuleList()
     : m_modules(), m_modules_mutex(), m_notifier(nullptr) {}
 
 ModuleList::ModuleList(const ModuleList &rhs)
     : m_modules(), m_modules_mutex(), m_notifier(nullptr) {
   std::lock_guard<std::recursive_mutex> lhs_guard(m_modules_mutex);
   std::lock_guard<std::recursive_mutex> rhs_guard(rhs.m_modules_mutex);
   m_modules = rhs.m_modules;

lldb/source/Target/Target.cpp

     BroadcastEvent(eBroadcastBitModulesUnloaded,
                    new TargetEventData(this->shared_from_this(), module_list));
   }
 }
 
 bool Target::ModuleIsExcludedForUnconstrainedSearches(
     const FileSpec &module_file_spec) {
   if (GetBreakpointsConsultPlatformAvoidList()) {
-    ModuleList matchingModules;
+    ModuleList matching_modules;
     ModuleSpec module_spec(module_file_spec);
-    GetImages().FindModules(module_spec, matchingModules);
-    size_t num_modules = matchingModules.GetSize();
+    GetImages().FindModules(module_spec, matching_modules);
 
     // If there is more than one module for this file spec, only
     // return true if ALL the modules are on the black list.
-    if (num_modules > 0) {
-      for (size_t i = 0; i < num_modules; i++) {
-        if (!ModuleIsExcludedForUnconstrainedSearches(
-                matchingModules.GetModuleAtIndex(i)))
+    if (matching_modules.GetSize() > 0) {
+      for (ModuleSP m : matching_modules) {
+        if (!ModuleIsExcludedForUnconstrainedSearches(m))
           return false;
       }
       return true;
     }
   }
   return false;
 }
 

   const bool has_primary_executable = exe_module && exe_module->GetObjectFile();
   if (has_primary_executable) {
     Address entry_addr = exe_module->GetObjectFile()->GetEntryPointAddress();
     if (entry_addr.IsValid())
       return entry_addr;
   }
 
   const ModuleList &modules = GetImages();
-  const size_t num_images = modules.GetSize();
-  for (size_t idx = 0; idx < num_images; ++idx) {
-    ModuleSP module_sp(modules.GetModuleAtIndex(idx));
+  for (ModuleSP module_sp : modules) {
     if (!module_sp || !module_sp->GetObjectFile())
       continue;
 
     Address entry_addr = module_sp->GetObjectFile()->GetEntryPointAddress();
     if (entry_addr.IsValid())
       return entry_addr;
   }
 

             stop_id, section_sp, new_section_load_addr, warn_multiple))
       return true; // Return true if the section load address was changed...
   }
   return false; // Return false to indicate nothing changed
 }
 
 size_t Target::UnloadModuleSections(const ModuleList &module_list) {
   size_t section_unload_count = 0;
-  size_t num_modules = module_list.GetSize();
-  for (size_t i = 0; i < num_modules; ++i) {
-    section_unload_count +=
-        UnloadModuleSections(module_list.GetModuleAtIndex(i));
-  }
+  for (ModuleSP module_sp : module_list)
+    section_unload_count += UnloadModuleSections(module_sp);
   return section_unload_count;
 }
 
 size_t Target::UnloadModuleSections(const lldb::ModuleSP &module_sp) {
   uint32_t stop_id = 0;
   ProcessSP process_sp(GetProcessSP());
   if (process_sp)
     stop_id = process_sp->GetStopID();

lldb/unittests/Core/CMakeLists.txt

 add_lldb_unittest(LLDBCoreTests
   CommunicationTest.cpp
   MangledTest.cpp
   ModuleSpecTest.cpp
+  ModuleListTest.cpp
   RichManglingContextTest.cpp
   SourceManagerTest.cpp
   StreamCallbackTest.cpp
   UniqueCStringMapTest.cpp
 
   LINK_LIBS
     lldbCore
     lldbHost

lldb/unittests/Core/ModuleListTest.cpp

+//===-- ModuleListTest.cpp ------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestingSupport/SubsystemRAII.h"
+#include "TestingSupport/TestUtilities.h"
+
+#include "lldb/Core/Module.h"
+#include "lldb/Core/ModuleList.h"
+#include "lldb/Utility/DataBuffer.h"
+
+#include "Plugins/ObjectFile/ELF/ObjectFileELF.h"
+#include "Plugins/ObjectFile/Mach-O/ObjectFileMachO.h"
+#include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
+
+#include "gtest/gtest.h"
+
+using namespace lldb;
+using namespace lldb_private;
+
+namespace {
+class ModuleListTest : public testing::Test {
+public:
+  static void SetUpTestCase() { FileSystem::Initialize(); }
+  static void TearDownTestCase() { FileSystem::Terminate(); }
+};
+} // namespace
+
+TEST_F(ModuleListTest, Iterators) {
+  auto AS = ArchSpec();
+  ModuleSP module_foo = std::make_shared<Module>(FileSpec("foo"), AS);
+  ModuleSP module_bar = std::make_shared<Module>(FileSpec("bar"), AS);
+  ModuleSP module_baz = std::make_shared<Module>(FileSpec("baz"), AS);
+
+  ModuleList module_list;
+  module_list.Append(module_foo);
+  module_list.Append(module_bar);
+  module_list.Append(module_baz);
+
+  ASSERT_EQ(module_list.GetModuleAtIndex(0), module_foo);
+  ASSERT_EQ(module_list.GetModuleAtIndex(1), module_bar);
+  ASSERT_EQ(module_list.GetModuleAtIndex(2), module_baz);
+
+  auto begin = module_list.begin();
+  auto end = module_list.end();
+  auto it = begin;
+
+  ASSERT_NE(it, end);
+  ASSERT_EQ(*it, module_foo);
+
+  ++it;
+  ASSERT_NE(it, end);
+  ASSERT_EQ(*it, module_bar);
+
+  ++it;
+  ASSERT_NE(it, end);
+  ASSERT_EQ(*it, module_baz);
+
+  ++it;
+  ASSERT_EQ(it, end);
+
+  --it;
+  ASSERT_NE(it, end);
+  ASSERT_EQ(*it, module_baz);
+
+  --it;
+  ASSERT_NE(it, end);
+  ASSERT_EQ(*it, module_bar);
+
+  --it;
+  ASSERT_EQ(*it, module_foo);
+  ASSERT_EQ(it, begin);
+}