[lldb] Replace GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with iterators (NFC)

[JDevlieghere]

Summary

Replace uses of GetModuleAtIndexUnlocked and GetModulePointerAtIndexUnlocked with the ModuleIterable and ModuleIterableNoLocking where applicable.

[mib]

LGTM apart from few nits.

lldb/include/lldb/Core/ModuleList.h

485–486

nit: Can we rename that to ModuleIterableNonLocking and ModulesNonLocking ?

Sounds more correct to me ^^

lldb/source/Commands/CommandObjectTarget.cpp

1404

Same here, we could remove the check and just use module_list.GetSize() in the printf.

2279

Same.

3911–3914

Early return instead to get rid of num_modules ?

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

409

This doesn't seem to be used anymore.

[JDevlieghere]

Address @mib's code review feedback.

lldb/include/lldb/Core/ModuleList.h

485–486

If you feel strongly about it I can do it as a pre-commit change, but personally I don't think it's worth the churn.

lldb/source/Commands/CommandObjectTarget.cpp

1404

There's a few places in this file where that's true, but here we still need it for both the early return and the print statement. I'm being pedantic, but generally ::size is only guaranteed to be O(n) as opposed to ::empty which is O(1). I think having the temporary variable here is fine.

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

409

It is below, but I've inlined it.

[mib]

lldb/include/lldb/Core/ModuleList.h

485–486

Not really, it can remain as is.

[mib]

lldb/source/Commands/CommandObjectTarget.cpp

1404

Sounds good then!

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

409

Oh! The code was collapsed --' My bad!

lldb/include/lldb/Core/ModuleList.h

public:
   ///
   /// \return
   ///     A pointer to a Module which can by nullptr if \a idx is out
   ///     of range.
   ///
   /// \see ModuleList::GetSize()
   Module *GetModulePointerAtIndex(size_t idx) const;
 
-  /// Get the module pointer for the module at index \a idx without acquiring
-  /// the ModuleList mutex.  This MUST already have been acquired with
-  /// ModuleList::GetMutex and locked for this call to be safe.
-  ///
-  /// \param[in] idx
-  ///     An index into this module collection.
-  ///
-  /// \return
-  ///     A pointer to a Module which can by nullptr if \a idx is out
-  ///     of range.
-  ///
-  /// \see ModuleList::GetSize()
-  Module *GetModulePointerAtIndexUnlocked(size_t idx) const;
-
   /// Find compile units by partial or full path.
   ///
   /// Finds all compile units that match \a path in all of the modules and
   /// returns the results in \a sc_list.
   ///
   /// \param[in] path
   ///     The name of the compile unit we are looking for.
   ///
protected:
   mutable std::recursive_mutex m_modules_mutex;
 
   Notifier *m_notifier;
 
 public:
   typedef LockingAdaptedIterable<collection, lldb::ModuleSP, vector_adapter,
                                  std::recursive_mutex>
       ModuleIterable;
-  ModuleIterable Modules() { return ModuleIterable(m_modules, GetMutex()); }
+  ModuleIterable Modules() const {
+    return ModuleIterable(m_modules, GetMutex());
+  }
 
   typedef AdaptedIterable<collection, lldb::ModuleSP, vector_adapter>
       ModuleIterableNoLocking;
-  ModuleIterableNoLocking ModulesNoLocking() {
+  ModuleIterableNoLocking ModulesNoLocking() const {
     return ModuleIterableNoLocking(m_modules);
   }
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_CORE_MODULELIST_H

lldb/include/lldb/Utility/Iterable.h

AdaptedConstIterator<C, E, A> end() {
     return AdaptedConstIterator<C, E, A>(m_container.end());
   }
 };
 
 template <typename C, typename E, E (*A)(typename C::const_iterator &),
           typename MutexType>
 class LockingAdaptedIterable : public AdaptedIterable<C, E, A> {
 public:
-  LockingAdaptedIterable(C &container, MutexType &mutex)
+  LockingAdaptedIterable(const C &container, MutexType &mutex)
       : AdaptedIterable<C, E, A>(container), m_mutex(&mutex) {
     m_mutex->lock();
   }
 
   LockingAdaptedIterable(LockingAdaptedIterable &&rhs)
       : AdaptedIterable<C, E, A>(rhs), m_mutex(rhs.m_mutex) {
     rhs.m_mutex = nullptr;
   }

lldb/source/Breakpoint/Breakpoint.cpp

 void Breakpoint::ModulesChanged(ModuleList &module_list, bool load,
                                 bool delete_locations) {
   Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_BREAKPOINTS));
   LLDB_LOGF(log,
             "Breakpoint::ModulesChanged: num_modules: %zu load: %i "
             "delete_locations: %i\n",
             module_list.GetSize(), load, delete_locations);
 
-  std::lock_guard<std::recursive_mutex> guard(module_list.GetMutex());
   if (load) {
     // The logic for handling new modules is:
     // 1) If the filter rejects this module, then skip it. 2) Run through the
     // current location list and if there are any locations
     //    for that module, we mark the module as "seen" and we don't try to
     //    re-resolve
     //    breakpoint locations for that module.
     //    However, we do add breakpoint sites to these locations if needed.
     // 3) If we don't see this module in our breakpoint location list, call
     // ResolveInModules.
 
     ModuleList new_modules; // We'll stuff the "unseen" modules in this list,
                             // and then resolve
     // them after the locations pass.  Have to do it this way because resolving
     // breakpoints will add new locations potentially.
 
-    for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
+    for (ModuleSP module_sp : module_list.Modules()) {
       bool seen = false;
       if (!m_filter_sp->ModulePasses(module_sp))
         continue;
 
       BreakpointLocationCollection locations_with_no_section;
       for (BreakpointLocationSP break_loc_sp :
            m_locations.BreakpointLocations()) {
 
if (load) {
 
     BreakpointEventData *removed_locations_event;
     if (!IsInternal())
       removed_locations_event = new BreakpointEventData(
           eBreakpointEventTypeLocationsRemoved, shared_from_this());
     else
       removed_locations_event = nullptr;
 
-    size_t num_modules = module_list.GetSize();
-    for (size_t i = 0; i < num_modules; i++) {
-      ModuleSP module_sp(module_list.GetModuleAtIndexUnlocked(i));
+    for (ModuleSP module_sp : module_list.Modules()) {
       if (m_filter_sp->ModulePasses(module_sp)) {
         size_t loc_idx = 0;
         size_t num_locations = m_locations.GetSize();
         BreakpointLocationCollection locations_to_remove;
         for (loc_idx = 0; loc_idx < num_locations; loc_idx++) {
           BreakpointLocationSP break_loc_sp(m_locations.GetByIndex(loc_idx));
           SectionSP section_sp(break_loc_sp->GetAddress().GetSection());
           if (section_sp && section_sp->GetModule() == module_sp) {

lldb/source/Commands/CommandObjectTarget.cpp

if (file_spec_ptr) {
     return;
   }
   // Keep the width spacing correct if things go wrong...
   if (width > 0)
     strm.Printf("%-*s", width, "");
 }
 
 static size_t DumpModuleObjfileHeaders(Stream &strm, ModuleList &module_list) {
-  size_t num_dumped = 0;
   std::lock_guard<std::recursive_mutex> guard(module_list.GetMutex());
   const size_t num_modules = module_list.GetSize();
-  if (num_modules > 0) {
-    strm.Printf("Dumping headers for %" PRIu64 " module(s).\n",
-                static_cast<uint64_t>(num_modules));
-    strm.IndentMore();
-    for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
-      Module *module = module_list.GetModulePointerAtIndexUnlocked(image_idx);
-      if (module) {
-        if (num_dumped++ > 0) {
-          strm.EOL();
-          strm.EOL();
-        }
-        ObjectFile *objfile = module->GetObjectFile();
-        if (objfile)
-          objfile->Dump(&strm);
-        else {
-          strm.Format("No object file for module: {0:F}\n",
-                      module->GetFileSpec());
-        }
-      }
-    }
-    strm.IndentLess();
-  }
+  if (num_modules == 0)
+    return 0;
+
+  size_t num_dumped = 0;
+  strm.Format("Dumping headers for {0} module(s).\n", num_modules);
+  for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
+    if (module_sp) {
+      ObjectFile *objfile = module_sp->GetObjectFile();
+                    module_sp->GetFileSpec());
   return num_dumped;
 }
 
 static void DumpModuleSymtab(CommandInterpreter &interpreter, Stream &strm,
                              Module *module, SortOrder sort_order,
                              Mangled::NamePreference name_preference) {
   if (!module)
     return;
Mangled::NamePreference name_preference =
                                     : Mangled::ePreferDemangled);
 
     uint32_t addr_byte_size = target->GetArchitecture().GetAddressByteSize();
     result.GetOutputStream().SetAddressByteSize(addr_byte_size);
     result.GetErrorStream().SetAddressByteSize(addr_byte_size);
 
     if (command.GetArgumentCount() == 0) {
       // Dump all sections for all modules images
-      std::lock_guard<std::recursive_mutex> guard(
-          target->GetImages().GetMutex());
-      const size_t num_modules = target->GetImages().GetSize();
+      const ModuleList &module_list = target->GetImages();
+      std::lock_guard<std::recursive_mutex> guard(module_list.GetMutex());
+      const size_t num_modules = module_list.GetSize();
       if (num_modules > 0) {
-        result.GetOutputStream().Printf("Dumping symbol table for %" PRIu64
-                                        " modules.\n",
-                                        (uint64_t)num_modules);
-        for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
+        result.GetOutputStream().Format(
+            "Dumping symbol table for {0} modules.\n", num_modules);
+        for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
           if (num_dumped > 0) {
             result.GetOutputStream().EOL();
             result.GetOutputStream().EOL();
           }
           if (m_interpreter.WasInterrupted())
             break;
           num_dumped++;
-          DumpModuleSymtab(
-              m_interpreter, result.GetOutputStream(),
-              target->GetImages().GetModulePointerAtIndexUnlocked(image_idx),
-              m_options.m_sort_order, name_preference);
+          DumpModuleSymtab(m_interpreter, result.GetOutputStream(),
+                           module_sp.get(), m_options.m_sort_order,
+                           name_preference);
         }
       } else {
         result.AppendError("the target has no associated executable images");
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
     } else {
       // Dump specified images (by basename or fullpath)
       const char *arg_cstr;
       for (int arg_idx = 0;
            (arg_cstr = command.GetArgumentAtIndex(arg_idx)) != nullptr;
            ++arg_idx) {
         ModuleList module_list;
         const size_t num_matches =
             FindModulesByName(target, arg_cstr, module_list, true);
         if (num_matches > 0) {
-          for (size_t i = 0; i < num_matches; ++i) {
-            Module *module = module_list.GetModulePointerAtIndex(i);
-            if (module) {
+          for (ModuleSP module_sp : module_list.Modules()) {
+            if (module_sp) {
               if (num_dumped > 0) {
                 result.GetOutputStream().EOL();
                 result.GetOutputStream().EOL();
               }
               if (m_interpreter.WasInterrupted())
                 break;
               num_dumped++;
-              DumpModuleSymtab(m_interpreter, result.GetOutputStream(), module,
-                               m_options.m_sort_order, name_preference);
+              DumpModuleSymtab(m_interpreter, result.GetOutputStream(),
+                               module_sp.get(), m_options.m_sort_order,
+                               name_preference);
             }
           }
         } else
           result.AppendWarningWithFormat(
               "Unable to find an image that matches '%s'.\n", arg_cstr);
       }
     }
 
bool DoExecute(Args &command, CommandReturnObject &result) override {
 
     uint32_t addr_byte_size = target->GetArchitecture().GetAddressByteSize();
     result.GetOutputStream().SetAddressByteSize(addr_byte_size);
     result.GetErrorStream().SetAddressByteSize(addr_byte_size);
 
     if (command.GetArgumentCount() == 0) {
       // Dump all sections for all modules images
       const size_t num_modules = target->GetImages().GetSize();
-      if (num_modules > 0) {
-        result.GetOutputStream().Printf("Dumping sections for %" PRIu64
-                                        " modules.\n",
-                                        (uint64_t)num_modules);
-        for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
-          if (m_interpreter.WasInterrupted())
-            break;
-          num_dumped++;
-          DumpModuleSections(
-              m_interpreter, result.GetOutputStream(),
-              target->GetImages().GetModulePointerAtIndex(image_idx));
-        }
-      } else {
-        result.AppendError("the target has no associated executable images");
-        result.SetStatus(eReturnStatusFailed);
-        return false;
-      }
-    } else {
+      if (num_modules == 0) {
+        result.AppendError("the target has no associated executable images");
+        result.SetStatus(eReturnStatusFailed);
+        return false;
+      }
+
+      result.GetOutputStream().Format("Dumping sections for {0} modules.\n",
+                                      num_modules);
       // Dump specified images (by basename or fullpath)
       const char *arg_cstr;
       for (int arg_idx = 0;
            (arg_cstr = command.GetArgumentAtIndex(arg_idx)) != nullptr;
            ++arg_idx) {
         ModuleList module_list;
         const size_t num_matches =
             FindModulesByName(target, arg_cstr, module_list, true);
CommandObjectTargetModulesDumpClangAST(CommandInterpreter &interpreter)
             nullptr, eCommandRequiresTarget) {}
 
   ~CommandObjectTargetModulesDumpClangAST() override = default;
 
 protected:
   bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target *target = &GetSelectedTarget();
 
-    const size_t num_modules = target->GetImages().GetSize();
+    const ModuleList &module_list = target->GetImages();
+    const size_t num_modules = module_list.GetSize();
     if (num_modules == 0) {
       result.AppendError("the target has no associated executable images");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     if (command.GetArgumentCount() == 0) {
       // Dump all ASTs for all modules images
-      result.GetOutputStream().Printf("Dumping clang ast for %" PRIu64
-                                      " modules.\n",
-                                      (uint64_t)num_modules);
-      for (size_t image_idx = 0; image_idx < num_modules; ++image_idx) {
+      result.GetOutputStream().Format("Dumping clang ast for {0} modules.\n",
+                                      num_modules);
+      for (ModuleSP module_sp : module_list.ModulesNoLocking()) {
         if (m_interpreter.WasInterrupted())
           break;
-        Module *m = target->GetImages().GetModulePointerAtIndex(image_idx);
-        if (SymbolFile *sf = m->GetSymbolFile())
+        if (SymbolFile *sf = module_sp->GetSymbolFile())
           sf->DumpClangAST(result.GetOutputStream());
       }
       result.SetStatus(eReturnStatusSuccessFinishResult);
       return true;
     }
 
     // Dump specified ASTs (by basename or fullpath)
     for (const Args::ArgEntry &arg : command.entries()) {
bool DoExecute(Args &command, CommandReturnObject &result) override {
     result.GetOutputStream().SetAddressByteSize(addr_byte_size);
     result.GetErrorStream().SetAddressByteSize(addr_byte_size);
 
     if (command.GetArgumentCount() == 0) {
       // Dump all sections for all modules images
       const ModuleList &target_modules = target->GetImages();
       std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
       const size_t num_modules = target_modules.GetSize();
-      if (num_modules > 0) {
-        result.GetOutputStream().Printf("Dumping debug symbols for %" PRIu64
-                                        " modules.\n",
-                                        (uint64_t)num_modules);
-        for (uint32_t image_idx = 0; image_idx < num_modules; ++image_idx) {
-          if (m_interpreter.WasInterrupted())
-            break;
-          if (DumpModuleSymbolFile(
-                  result.GetOutputStream(),
-                  target_modules.GetModulePointerAtIndexUnlocked(image_idx)))
-            num_dumped++;
-        }
-      } else {
+      if (num_modules == 0) {
         result.AppendError("the target has no associated executable images");
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
+      result.GetOutputStream().Format(
+          "Dumping debug symbols for {0} modules.\n", num_modules);
+      for (ModuleSP module_sp : target_modules.ModulesNoLocking()) {
+        if (m_interpreter.WasInterrupted())
+          break;
+        if (DumpModuleSymbolFile(result.GetOutputStream(), module_sp.get()))
+          num_dumped++;
+      }
     } else {
       // Dump specified images (by basename or fullpath)
       const char *arg_cstr;
       for (int arg_idx = 0;
            (arg_cstr = command.GetArgumentAtIndex(arg_idx)) != nullptr;
            ++arg_idx) {
         ModuleList module_list;
         const size_t num_matches =
if (command.GetArgumentCount() == 0) {
       const char *arg_cstr;
       for (int arg_idx = 0;
            (arg_cstr = command.GetArgumentAtIndex(arg_idx)) != nullptr;
            ++arg_idx) {
         FileSpec file_spec(arg_cstr);
 
         const ModuleList &target_modules = target->GetImages();
         std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-        const size_t num_modules = target_modules.GetSize();
-        if (num_modules > 0) {
+        if (target_modules.GetSize() > 0) {
           uint32_t num_dumped = 0;
-          for (uint32_t i = 0; i < num_modules; ++i) {
+          for (ModuleSP module_sp : target_modules.ModulesNoLocking()) {
             if (m_interpreter.WasInterrupted())
               break;
             if (DumpCompileUnitLineTable(
-                    m_interpreter, result.GetOutputStream(),
-                    target_modules.GetModulePointerAtIndexUnlocked(i),
+                    m_interpreter, result.GetOutputStream(), module_sp.get(),
                     file_spec,
                     m_options.m_verbose ? eDescriptionLevelFull
                                         : eDescriptionLevelBrief))
               num_dumped++;
           }
           if (num_dumped == 0)
             result.AppendWarningWithFormat(
                 "No source filenames matched '%s'.\n", arg_cstr);
if (command.GetArgumentCount() == 0) {
           return result.Succeeded();
         }
       }
 
       // Dump all sections for all other modules
 
       const ModuleList &target_modules = target->GetImages();
       std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-      const size_t num_modules = target_modules.GetSize();
-      if (num_modules > 0) {
-        for (i = 0; i < num_modules && !syntax_error; ++i) {
-          Module *module_pointer =
-              target_modules.GetModulePointerAtIndexUnlocked(i);
-
-          if (module_pointer != current_module.get() &&
-              LookupInModule(m_interpreter,
-                             target_modules.GetModulePointerAtIndexUnlocked(i),
-                             result, syntax_error)) {
-            result.GetOutputStream().EOL();
-            num_successful_lookups++;
-          }
-        }
-      } else {
+      if (target_modules.GetSize() == 0) {
         result.AppendError("the target has no associated executable images");
         result.SetStatus(eReturnStatusFailed);
         return false;
       }
+
+      for (ModuleSP module_sp : target_modules.ModulesNoLocking()) {
+        if (module_sp != current_module &&
+            LookupInModule(m_interpreter, module_sp.get(), result,
+                           syntax_error)) {
+          result.GetOutputStream().EOL();
+          num_successful_lookups++;
+        }
+      }
     } else {
       // Dump specified images (by basename or fullpath)
       const char *arg_cstr;
       for (i = 0; (arg_cstr = command.GetArgumentAtIndex(i)) != nullptr &&
                   !syntax_error;
            ++i) {
         ModuleList module_list;
         const size_t num_matches =
             FindModulesByName(target, arg_cstr, module_list, false);
         if (num_matches > 0) {
           for (size_t j = 0; j < num_matches; ++j) {
             Module *module = module_list.GetModulePointerAtIndex(j);
             if (module) {
               if (LookupInModule(m_interpreter, module, result, syntax_error)) {

lldb/source/Core/ModuleList.cpp

void ModuleList::ClearImpl(bool use_notifier) {
   std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
   if (use_notifier && m_notifier)
     m_notifier->NotifyWillClearList(*this);
   m_modules.clear();
 }
 
 Module *ModuleList::GetModulePointerAtIndex(size_t idx) const {
   std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
-  return GetModulePointerAtIndexUnlocked(idx);
-}
-
-Module *ModuleList::GetModulePointerAtIndexUnlocked(size_t idx) const {
   if (idx < m_modules.size())
     return m_modules[idx].get();
   return nullptr;
 }
 
 ModuleSP ModuleList::GetModuleAtIndex(size_t idx) const {
   std::lock_guard<std::recursive_mutex> guard(m_modules_mutex);
   return GetModuleAtIndexUnlocked(idx);

lldb/source/Core/SearchFilter.cpp

if (!m_target_sp)
     return;
   empty_sc.target_sp = m_target_sp;
 
   if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     searcher.SearchCallback(*this, empty_sc, nullptr);
     return;
   }
 
-  std::lock_guard<std::recursive_mutex> guard(modules.GetMutex());
-  const size_t numModules = modules.GetSize();
-
-  for (size_t i = 0; i < numModules; i++) {
-    ModuleSP module_sp(modules.GetModuleAtIndexUnlocked(i));
+  for (ModuleSP module_sp : modules.Modules()) {
     if (!ModulePasses(module_sp))
       continue;
     if (DoModuleIteration(module_sp, searcher) == Searcher::eCallbackReturnStop)
       return;
   }
 }
 
 Searcher::CallbackReturn
if (context.module_sp) {
     if (searcher.GetDepth() != lldb::eSearchDepthModule)
       return DoCUIteration(context.module_sp, context, searcher);
 
     SymbolContext matchingContext(context.module_sp.get());
     searcher.SearchCallback(*this, matchingContext, nullptr);
     return Searcher::eCallbackReturnContinue;
   }
 
-  const ModuleList &target_images = m_target_sp->GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
-
-  size_t n_modules = target_images.GetSize();
-  for (size_t i = 0; i < n_modules; i++) {
+  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
     // If this is the last level supplied, then call the callback directly,
     // otherwise descend.
-    ModuleSP module_sp(target_images.GetModuleAtIndexUnlocked(i));
     if (!ModulePasses(module_sp))
       continue;
 
     if (searcher.GetDepth() == lldb::eSearchDepthModule) {
       SymbolContext matchingContext(m_target_sp, module_sp);
 
       Searcher::CallbackReturn shouldContinue =
           searcher.SearchCallback(*this, matchingContext, nullptr);
void SearchFilterByModule::Search(Searcher &searcher) {
 
   // If the module file spec is a full path, then we can just find the one
   // filespec that passes.  Otherwise, we need to go through all modules and
   // find the ones that match the file name.
 
   const ModuleList &target_modules = m_target_sp->GetImages();
   std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
 
-  const size_t num_modules = target_modules.GetSize();
-  for (size_t i = 0; i < num_modules; i++) {
-    Module *module = target_modules.GetModulePointerAtIndexUnlocked(i);
-    if (FileSpec::Match(m_module_spec, module->GetFileSpec())) {
-      SymbolContext matchingContext(m_target_sp, module->shared_from_this());
+  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
+    if (FileSpec::Match(m_module_spec, module_sp->GetFileSpec())) {
+      SymbolContext matchingContext(m_target_sp, module_sp);
       Searcher::CallbackReturn shouldContinue;
 
       shouldContinue = DoModuleIteration(matchingContext, searcher);
       if (shouldContinue == Searcher::eCallbackReturnStop)
         return;
     }
   }
 }
if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     SymbolContext empty_sc;
     empty_sc.target_sp = m_target_sp;
     searcher.SearchCallback(*this, empty_sc, nullptr);
   }
 
   // If the module file spec is a full path, then we can just find the one
   // filespec that passes.  Otherwise, we need to go through all modules and
   // find the ones that match the file name.
-
-  const ModuleList &target_modules = m_target_sp->GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-
-  const size_t num_modules = target_modules.GetSize();
-  for (size_t i = 0; i < num_modules; i++) {
-    Module *module = target_modules.GetModulePointerAtIndexUnlocked(i);
-    if (m_module_spec_list.FindFileIndex(0, module->GetFileSpec(), false) ==
+  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
+    if (m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) ==
         UINT32_MAX)
       continue;
-    SymbolContext matchingContext(m_target_sp, module->shared_from_this());
+    SymbolContext matchingContext(m_target_sp, module_sp);
     Searcher::CallbackReturn shouldContinue;
 
     shouldContinue = DoModuleIteration(matchingContext, searcher);
     if (shouldContinue == Searcher::eCallbackReturnStop)
       return;
   }
 }
 
if (searcher.GetDepth() == lldb::eSearchDepthTarget) {
     searcher.SearchCallback(*this, empty_sc, nullptr);
   }
 
   // If the module file spec is a full path, then we can just find the one
   // filespec that passes.  Otherwise, we need to go through all modules and
   // find the ones that match the file name.
 
   ModuleList matching_modules;
-  const ModuleList &target_images = m_target_sp->GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
 
-  const size_t num_modules = target_images.GetSize();
   bool no_modules_in_filter = m_module_spec_list.GetSize() == 0;
-  for (size_t i = 0; i < num_modules; i++) {
-    lldb::ModuleSP module_sp = target_images.GetModuleAtIndexUnlocked(i);
+  for (ModuleSP module_sp : m_target_sp->GetImages().Modules()) {
     if (!no_modules_in_filter &&
         m_module_spec_list.FindFileIndex(0, module_sp->GetFileSpec(), false) ==
             UINT32_MAX)
       continue;
 
     SymbolContext matchingContext(m_target_sp, module_sp);
     Searcher::CallbackReturn shouldContinue;
 

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp

 void DynamicLoaderDarwin::UnloadAllImages() {
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
   ModuleList unloaded_modules_list;
 
   Target &target = m_process->GetTarget();
   const ModuleList &target_modules = target.GetImages();
   std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
 
-  size_t num_modules = target_modules.GetSize();
   ModuleSP dyld_sp(GetDYLDModule());
-
-  for (size_t i = 0; i < num_modules; i++) {
-    ModuleSP module_sp = target_modules.GetModuleAtIndexUnlocked(i);
-
+  for (ModuleSP module_sp : target_modules.Modules()) {
     // Don't remove dyld - else we'll lose our breakpoint notifying us about
     // libraries being re-loaded...
-    if (module_sp.get() != nullptr && module_sp.get() != dyld_sp.get()) {
+    if (module_sp && module_sp != dyld_sp) {
       UnloadSections(module_sp);
       unloaded_modules_list.Append(module_sp);
     }
   }
 
   if (unloaded_modules_list.GetSize() != 0) {
     if (log) {
       log->PutCString("Unloaded:");

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOS.cpp

 //
 //  int __attribute__((visibility("hidden")))           _dyld_global_lock_held =
 //  0;
 //
 //  in libdyld.dylib.
 Status DynamicLoaderMacOS::CanLoadImage() {
   Status error;
   addr_t symbol_address = LLDB_INVALID_ADDRESS;
+  ConstString g_libdyld_name("libdyld.dylib");
   Target &target = m_process->GetTarget();
   const ModuleList &target_modules = target.GetImages();
   std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-  const size_t num_modules = target_modules.GetSize();
-  ConstString g_libdyld_name("libdyld.dylib");
 
   // Find any modules named "libdyld.dylib" and look for the symbol there first
-  for (size_t i = 0; i < num_modules; i++) {
-    Module *module_pointer = target_modules.GetModulePointerAtIndexUnlocked(i);
-    if (module_pointer) {
-      if (module_pointer->GetFileSpec().GetFilename() == g_libdyld_name) {
-        symbol_address = GetDyldLockVariableAddressFromModule(module_pointer);
+  for (ModuleSP module_sp : target.GetImages().ModulesNoLocking()) {
+    if (module_sp) {
+      if (module_sp->GetFileSpec().GetFilename() == g_libdyld_name) {
+        symbol_address = GetDyldLockVariableAddressFromModule(module_sp.get());
         if (symbol_address != LLDB_INVALID_ADDRESS)
           break;
       }
     }
   }
 
   // Search through all modules looking for the symbol in them
   if (symbol_address == LLDB_INVALID_ADDRESS) {
-    for (size_t i = 0; i < num_modules; i++) {
-      Module *module_pointer =
-          target_modules.GetModulePointerAtIndexUnlocked(i);
-      if (module_pointer) {
+    for (ModuleSP module_sp : target.GetImages().Modules()) {
+      if (module_sp) {
         addr_t symbol_address =
-            GetDyldLockVariableAddressFromModule(module_pointer);
+            GetDyldLockVariableAddressFromModule(module_sp.get());
         if (symbol_address != LLDB_INVALID_ADDRESS)
           break;
       }
     }
   }
 
   // Default assumption is that it is OK to load images. Only say that we
   // cannot load images if we find the symbol in libdyld and it indicates that
   // we cannot.
 
   if (symbol_address != LLDB_INVALID_ADDRESS) {
     {
       int lock_held =
           m_process->ReadUnsignedIntegerFromMemory(symbol_address, 4, 0, error);
       if (lock_held != 0) {
         error.SetErrorString("dyld lock held - unsafe to load images.");
       }
     }
   } else {
     // If we were unable to find _dyld_global_lock_held in any modules, or it
     // is not loaded into memory yet, we may be at process startup (sitting  at
     // _dyld_start) - so we should not allow dlopen calls. But if we found more
     // than one module then we are clearly past _dyld_start so in that case
     // we'll default to "it's safe".
-    if (num_modules <= 1)
-        error.SetErrorString("could not find the dyld library or "
-                                       "the dyld lock symbol");
+    if (target.GetImages().GetSize() <= 1)
   }
   return error;
 }
 
 bool DynamicLoaderMacOS::GetSharedCacheInformation(
     lldb::addr_t &base_address, UUID &uuid, LazyBool &using_shared_cache,
     LazyBool &private_shared_cache) {
   base_address = LLDB_INVALID_ADDRESS;

lldb/source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderMacOSXDYLD.cpp

if (ReadAllImageInfosStructure()) {
 
     // Now we have one more bit of business.  If there is a library left in the
     // images for our target that doesn't have a load address, then it must be
     // something that we were expecting to load (for instance we read a load
     // command for it) but it didn't in fact load - probably because
     // DYLD_*_PATH pointed to an equivalent version.  We don't want it to stay
     // in the target's module list or it will confuse us, so unload it here.
     Target &target = m_process->GetTarget();
-    const ModuleList &target_modules = target.GetImages();
     ModuleList not_loaded_modules;
-    std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-
-    size_t num_modules = target_modules.GetSize();
-    for (size_t i = 0; i < num_modules; i++) {
-      ModuleSP module_sp = target_modules.GetModuleAtIndexUnlocked(i);
+    for (ModuleSP module_sp : target.GetImages().Modules()) {
       if (!module_sp->IsLoadedInTarget(&target)) {
         if (log) {
           StreamString s;
           module_sp->GetDescription(s.AsRawOstream());
           LLDB_LOGF(log, "Unloading pre-run module: %s.", s.GetData());
         }
         not_loaded_modules.Append(module_sp);
       }

lldb/source/Plugins/DynamicLoader/Static/DynamicLoaderStatic.cpp

 void DynamicLoaderStatic::LoadAllImagesAtFileAddresses() {
   const ModuleList &module_list = m_process->GetTarget().GetImages();
 
   ModuleList loaded_module_list;
 
   // Disable JIT for static dynamic loader targets
   m_process->SetCanJIT(false);
 
-  std::lock_guard<std::recursive_mutex> guard(module_list.GetMutex());
-
-  const size_t num_modules = module_list.GetSize();
-  for (uint32_t idx = 0; idx < num_modules; ++idx) {
-    ModuleSP module_sp(module_list.GetModuleAtIndexUnlocked(idx));
+  for (ModuleSP module_sp : module_list.Modules()) {
     if (module_sp) {
       bool changed = false;
       ObjectFile *image_object_file = module_sp->GetObjectFile();
       if (image_object_file) {
         SectionList *section_list = image_object_file->GetSectionList();
         if (section_list) {
           // All sections listed in the dyld image info structure will all
           // either be fixed up already, or they will all be off by a single

lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp

if (SymbolFile *symbol_file = module_sp->GetSymbolFile()) {
 
         LLDB_LOG(log, "  CAS::FEVD Found namespace {0} in module {1}", name,
                  module_sp->GetFileSpec().GetFilename());
       }
     }
     return;
   }
 
-  const ModuleList &target_images = m_target->GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
-
-  for (size_t i = 0, e = target_images.GetSize(); i < e; ++i) {
-    lldb::ModuleSP image = target_images.GetModuleAtIndexUnlocked(i);
-
+  for (lldb::ModuleSP image : m_target->GetImages().Modules()) {
     if (!image)
       continue;
 
     CompilerDeclContext found_namespace_decl;
 
     SymbolFile *symbol_file = image->GetSymbolFile();
 
     if (!symbol_file)
for (ClangASTImporter::NamespaceMap::iterator i = parent_map->begin(),
 
       namespace_map->push_back(std::pair<lldb::ModuleSP, CompilerDeclContext>(
           module_sp, found_namespace_decl));
 
       LLDB_LOG(log, "  CMN Found namespace {0} in module {1}", name,
                module_sp->GetFileSpec().GetFilename());
     }
   } else {
-    const ModuleList &target_images = m_target->GetImages();
-    std::lock_guard<std::recursive_mutex> guard(target_images.GetMutex());
-
     CompilerDeclContext null_namespace_decl;
-
-    for (size_t i = 0, e = target_images.GetSize(); i < e; ++i) {
-      lldb::ModuleSP image = target_images.GetModuleAtIndexUnlocked(i);
-
+    for (lldb::ModuleSP image : m_target->GetImages().Modules()) {
       if (!image)
         continue;
 
       CompilerDeclContext found_namespace_decl;
 
       SymbolFile *symbol_file = image->GetSymbolFile();
 
       if (!symbol_file)

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntime.cpp

AppleObjCRuntime::GetObjCVersion(Process *process, ModuleSP &objc_module_sp) {
   if (!process)
     return ObjCRuntimeVersions::eObjC_VersionUnknown;
 
   Target &target = process->GetTarget();
   if (target.GetArchitecture().GetTriple().getVendor() !=
       llvm::Triple::VendorType::Apple)
     return ObjCRuntimeVersions::eObjC_VersionUnknown;
 
-  const ModuleList &target_modules = target.GetImages();
-  std::lock_guard<std::recursive_mutex> gaurd(target_modules.GetMutex());
-
-  size_t num_images = target_modules.GetSize();
-  for (size_t i = 0; i < num_images; i++) {
-    ModuleSP module_sp = target_modules.GetModuleAtIndexUnlocked(i);
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
     // One tricky bit here is that we might get called as part of the initial
     // module loading, but before all the pre-run libraries get winnowed from
     // the module list.  So there might actually be an old and incorrect ObjC
     // library sitting around in the list, and we don't want to look at that.
     // That's why we call IsLoadedInTarget.
 
     if (AppleIsModuleObjCLibrary(module_sp) &&
         module_sp->IsLoadedInTarget(&target)) {

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp

if (!m_get_shared_cache_class_info_code) {
     // use that in our jitted expression.  Else fall back to the old
     // class_getName.
     static ConstString g_class_getName_symbol_name("class_getName");
     static ConstString g_class_getNameRaw_symbol_name("objc_debug_class_getNameRaw");
     ConstString class_name_getter_function_name = g_class_getName_symbol_name;
 
     ObjCLanguageRuntime *objc_runtime = ObjCLanguageRuntime::Get(*process);
     if (objc_runtime) {
-      const ModuleList &images = process->GetTarget().GetImages();
-      std::lock_guard<std::recursive_mutex> guard(images.GetMutex());
-      for (size_t i = 0; i < images.GetSize(); ++i) {
-        lldb::ModuleSP mod_sp = images.GetModuleAtIndexUnlocked(i);
+      for (lldb::ModuleSP mod_sp : process->GetTarget().GetImages().Modules()) {
         if (objc_runtime->IsModuleObjCLibrary(mod_sp)) {
           const Symbol *symbol =
               mod_sp->FindFirstSymbolWithNameAndType(g_class_getNameRaw_symbol_name, 
                                                 lldb::eSymbolTypeCode);
           if (symbol && 
               (symbol->ValueIsAddress() || symbol->GetAddressRef().IsValid())) {
             class_name_getter_function_name = g_class_getNameRaw_symbol_name;
           }

lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCTrampolineHandler.cpp

 bool AppleObjCTrampolineHandler::AppleObjCVTables::InitializeVTableSymbols() {
   if (m_trampoline_header != LLDB_INVALID_ADDRESS)
     return true;
 
   ProcessSP process_sp = GetProcessSP();
   if (process_sp) {
     Target &target = process_sp->GetTarget();
 
-    const ModuleList &target_modules = target.GetImages();
-    std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-    size_t num_modules = target_modules.GetSize();
     if (!m_objc_module_sp) {
-      for (size_t i = 0; i < num_modules; i++) {
+      for (ModuleSP module_sp : target.GetImages().Modules()) {
         if (ObjCLanguageRuntime::Get(*process_sp)
-                ->IsModuleObjCLibrary(
-                    target_modules.GetModuleAtIndexUnlocked(i))) {
-          m_objc_module_sp = target_modules.GetModuleAtIndexUnlocked(i);
+                ->IsModuleObjCLibrary(module_sp)) {
+          m_objc_module_sp = module_sp;
           break;
         }
       }
     }
 
     if (m_objc_module_sp) {
       ConstString trampoline_name("gdb_objc_trampolines");
       const Symbol *trampoline_symbol =

lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp

 LLDB_PLUGIN_DEFINE(MemoryHistoryASan)
 
 MemoryHistorySP MemoryHistoryASan::CreateInstance(const ProcessSP &process_sp) {
   if (!process_sp.get())
     return nullptr;
 
   Target &target = process_sp->GetTarget();
 
-  const ModuleList &target_modules = target.GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-  const size_t num_modules = target_modules.GetSize();
-  for (size_t i = 0; i < num_modules; ++i) {
-    Module *module_pointer = target_modules.GetModulePointerAtIndexUnlocked(i);
-
-    const Symbol *symbol = module_pointer->FindFirstSymbolWithNameAndType(
+  for (ModuleSP module_sp : target.GetImages().Modules()) {
+    const Symbol *symbol = module_sp->FindFirstSymbolWithNameAndType(
         ConstString("__asan_get_alloc_stack"), lldb::eSymbolTypeAny);
 
     if (symbol != nullptr)
       return MemoryHistorySP(new MemoryHistoryASan(process_sp));
   }
 
   return MemoryHistorySP();
 }

lldb/source/Target/Process.cpp

if (m_os_up) {
       // Somebody might have gotten threads before now, but we need to force the
       // update after we've loaded the OperatingSystem plugin or it won't get a
       // chance to process the threads.
       m_thread_list.Clear();
       UpdateThreadListIfNeeded();
     }
   }
   // Figure out which one is the executable, and set that in our target:
-  const ModuleList &target_modules = GetTarget().GetImages();
-  std::lock_guard<std::recursive_mutex> guard(target_modules.GetMutex());
-  size_t num_modules = target_modules.GetSize();
   ModuleSP new_executable_module_sp;
-
-  for (size_t i = 0; i < num_modules; i++) {
-    ModuleSP module_sp(target_modules.GetModuleAtIndexUnlocked(i));
+  for (ModuleSP module_sp : GetTarget().GetImages().Modules()) {
     if (module_sp && module_sp->IsExecutable()) {
       if (GetTarget().GetExecutableModulePointer() != module_sp.get())
         new_executable_module_sp = module_sp;
       break;
     }
   }
   if (new_executable_module_sp) {
     GetTarget().SetExecutableModule(new_executable_module_sp,