[lldb] GetSharedModule: Collect old modules in SmallVector

The various GetSharedModule methods have an optional out parameter for
the old module when a file has changed or been replaced, which the
Target uses to keep its module list current/correct.  We've been using
a single ModuleSP to track "the" old module, and this change switches
to using a SmallVector of ModuleSP, which has a couple benefits:
 - There are multiple codepaths which may discover an old module, and
   this centralizes the code for how to handle multiples in one place,
   in the Target code.  With the single ModuleSP, each place that may
   discover an old module is responsible for how it handles multiples,
   and the current code is inconsistent (some code paths drop the first
   old module, others drop the second).
 - The API will be more natural for identifying old modules in routines
   that work on sets, like ModuleList::ReplaceEquivalent (which I plan
   on updating to report old module(s) in a subsequent change to fix a
   bug).

I'm not convinced we can ever actually run into the case that multiple
old modules are found in the same GetOrCreateModule call, but I think
this change makes sense regardless, in light of the above.

When an old module is reported, Target::GetOrCreateModule calls
m_images.ReplaceModule, which doesn't allow multiple "old" modules; the
new code calls ReplaceModule for the first "old" module, and for any
subsequent old modules it logs the event and calls m_images.Remove.

Reviewed By: jingham

Differential Revision: https://reviews.llvm.org/D89156

(cherry picked from commit 61bfc70)

lldb/include/lldb/Core/ModuleList.h

@@ -443,12 +443,11 @@ class ModuleList {
 
   static bool ModuleIsInCache(const Module *module_ptr);
 
-  static Status GetSharedModule(const ModuleSpec &module_spec,
-                                lldb::ModuleSP &module_sp,
-                                const FileSpecList *module_search_paths_ptr,
-                                lldb::ModuleSP *old_module_sp_ptr,
-                                bool *did_create_ptr,
-                                bool always_create = false);
+  static Status
+  GetSharedModule(const ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
+                  const FileSpecList *module_search_paths_ptr,
+                  llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
+                  bool *did_create_ptr, bool always_create = false);
 
   static bool RemoveSharedModule(lldb::ModuleSP &module_sp);
 

lldb/include/lldb/Target/Platform.h

@@ -301,11 +301,10 @@ class Platform : public PluginInterface {
   LocateExecutableScriptingResources(Target *target, Module &module,
                                      Stream *feedback_stream);
 
-  virtual Status GetSharedModule(const ModuleSpec &module_spec,
-                                 Process *process, lldb::ModuleSP &module_sp,
-                                 const FileSpecList *module_search_paths_ptr,
-                                 lldb::ModuleSP *old_module_sp_ptr,
-                                 bool *did_create_ptr);
+  virtual Status GetSharedModule(
+      const ModuleSpec &module_spec, Process *process,
+      lldb::ModuleSP &module_sp, const FileSpecList *module_search_paths_ptr,
+      llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules, bool *did_create_ptr);
 
   virtual bool GetModuleSpec(const FileSpec &module_file_spec,
                              const ArchSpec &arch, ModuleSpec &module_spec);

lldb/source/Core/ModuleList.cpp

@@ -731,11 +731,11 @@ size_t ModuleList::RemoveOrphanSharedModules(bool mandatory) {
   return GetSharedModuleList().RemoveOrphans(mandatory);
 }
 
-Status ModuleList::GetSharedModule(const ModuleSpec &module_spec,
-                                   ModuleSP &module_sp,
-                                   const FileSpecList *module_search_paths_ptr,
-                                   ModuleSP *old_module_sp_ptr,
-                                   bool *did_create_ptr, bool always_create) {
+Status
+ModuleList::GetSharedModule(const ModuleSpec &module_spec, ModuleSP &module_sp,
+                            const FileSpecList *module_search_paths_ptr,
+                            llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
+                            bool *did_create_ptr, bool always_create) {
   ModuleList &shared_module_list = GetSharedModuleList();
   std::lock_guard<std::recursive_mutex> guard(
       shared_module_list.m_modules_mutex);
@@ -747,8 +747,6 @@ Status ModuleList::GetSharedModule(const ModuleSpec &module_spec,
 
   if (did_create_ptr)
     *did_create_ptr = false;
-  if (old_module_sp_ptr)
-    old_module_sp_ptr->reset();
 
   const UUID *uuid_ptr = module_spec.GetUUIDPtr();
   const FileSpec &module_file_spec = module_spec.GetFileSpec();
@@ -769,8 +767,8 @@ Status ModuleList::GetSharedModule(const ModuleSpec &module_spec,
 
         // Make sure the file for the module hasn't been modified
         if (module_sp->FileHasChanged()) {
-          if (old_module_sp_ptr && !*old_module_sp_ptr)
-            *old_module_sp_ptr = module_sp;
+          if (old_modules)
+            old_modules->push_back(module_sp);
 
           Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_MODULES));
           if (log != nullptr)
@@ -924,8 +922,8 @@ Status ModuleList::GetSharedModule(const ModuleSpec &module_spec,
             located_binary_modulespec.GetFileSpec());
         if (file_spec_mod_time != llvm::sys::TimePoint<>()) {
           if (file_spec_mod_time != module_sp->GetModificationTime()) {
-            if (old_module_sp_ptr)
-              *old_module_sp_ptr = module_sp;
+            if (old_modules)
+              old_modules->push_back(module_sp);
             shared_module_list.Remove(module_sp);
             module_sp.reset();
           }

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp

@@ -221,7 +221,7 @@ BringInRemoteFile(Platform *platform,
 lldb_private::Status PlatformDarwin::GetSharedModuleWithLocalCache(
     const lldb_private::ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
     const lldb_private::FileSpecList *module_search_paths_ptr,
-    lldb::ModuleSP *old_module_sp_ptr, bool *did_create_ptr) {
+    llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules, bool *did_create_ptr) {
 
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PLATFORM));
   LLDB_LOGF(log,
@@ -238,7 +238,7 @@ lldb_private::Status PlatformDarwin::GetSharedModuleWithLocalCache(
   Status err;
 
   err = ModuleList::GetSharedModule(module_spec, module_sp,
-                                    module_search_paths_ptr, old_module_sp_ptr,
+                                    module_search_paths_ptr, old_modules,
                                     did_create_ptr);
   if (module_sp)
     return err;
@@ -341,8 +341,8 @@ lldb_private::Status PlatformDarwin::GetSharedModuleWithLocalCache(
 
 Status PlatformDarwin::GetSharedModule(
     const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
-    const FileSpecList *module_search_paths_ptr, ModuleSP *old_module_sp_ptr,
-    bool *did_create_ptr) {
+    const FileSpecList *module_search_paths_ptr,
+    llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
   Status error;
   module_sp.reset();
 
@@ -351,16 +351,16 @@ Status PlatformDarwin::GetSharedModule(
     // module first.
     if (m_remote_platform_sp) {
       error = m_remote_platform_sp->GetSharedModule(
-          module_spec, process, module_sp, module_search_paths_ptr,
-          old_module_sp_ptr, did_create_ptr);
+          module_spec, process, module_sp, module_search_paths_ptr, old_modules,
+          did_create_ptr);
     }
   }
 
   if (!module_sp) {
     // Fall back to the local platform and find the file locally
     error = Platform::GetSharedModule(module_spec, process, module_sp,
-                                      module_search_paths_ptr,
-                                      old_module_sp_ptr, did_create_ptr);
+                                      module_search_paths_ptr, old_modules,
+                                      did_create_ptr);
 
     const FileSpec &platform_file = module_spec.GetFileSpec();
     if (!module_sp && module_search_paths_ptr && platform_file) {
@@ -373,7 +373,7 @@ Status PlatformDarwin::GetSharedModule(
           new_module_spec.GetFileSpec() = bundle_directory;
           if (Host::ResolveExecutableInBundle(new_module_spec.GetFileSpec())) {
             Status new_error(Platform::GetSharedModule(
-                new_module_spec, process, module_sp, nullptr, old_module_sp_ptr,
+                new_module_spec, process, module_sp, nullptr, old_modules,
                 did_create_ptr));
 
             if (module_sp)
@@ -400,8 +400,8 @@ Status PlatformDarwin::GetSharedModule(
                 ModuleSpec new_module_spec(module_spec);
                 new_module_spec.GetFileSpec() = new_file_spec;
                 Status new_error(Platform::GetSharedModule(
-                    new_module_spec, process, module_sp, nullptr,
-                    old_module_sp_ptr, did_create_ptr));
+                    new_module_spec, process, module_sp, nullptr, old_modules,
+                    did_create_ptr));
 
                 if (module_sp) {
                   module_sp->SetPlatformFileSpec(new_file_spec);
@@ -1639,8 +1639,8 @@ PlatformDarwin::LaunchProcess(lldb_private::ProcessLaunchInfo &launch_info) {
 
 lldb_private::Status PlatformDarwin::FindBundleBinaryInExecSearchPaths(
     const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
-    const FileSpecList *module_search_paths_ptr, ModuleSP *old_module_sp_ptr,
-    bool *did_create_ptr) {
+    const FileSpecList *module_search_paths_ptr,
+    llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
   const FileSpec &platform_file = module_spec.GetFileSpec();
   // See if the file is present in any of the module_search_paths_ptr
   // directories.
@@ -1697,9 +1697,9 @@ lldb_private::Status PlatformDarwin::FindBundleBinaryInExecSearchPaths(
         if (FileSystem::Instance().Exists(path_to_try)) {
           ModuleSpec new_module_spec(module_spec);
           new_module_spec.GetFileSpec() = path_to_try;
-          Status new_error(Platform::GetSharedModule(
-              new_module_spec, process, module_sp, nullptr, old_module_sp_ptr,
-              did_create_ptr));
+          Status new_error(
+              Platform::GetSharedModule(new_module_spec, process, module_sp,
+                                        nullptr, old_modules, did_create_ptr));
 
           if (module_sp) {
             module_sp->SetPlatformFileSpec(path_to_try);

lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.h

@@ -46,7 +46,7 @@ class PlatformDarwin : public PlatformPOSIX {
   GetSharedModule(const lldb_private::ModuleSpec &module_spec,
                   lldb_private::Process *process, lldb::ModuleSP &module_sp,
                   const lldb_private::FileSpecList *module_search_paths_ptr,
-                  lldb::ModuleSP *old_module_sp_ptr,
+                  llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
                   bool *did_create_ptr) override;
 
   size_t GetSoftwareBreakpointTrapOpcode(
@@ -132,7 +132,7 @@ class PlatformDarwin : public PlatformPOSIX {
   virtual lldb_private::Status GetSharedModuleWithLocalCache(
       const lldb_private::ModuleSpec &module_spec, lldb::ModuleSP &module_sp,
       const lldb_private::FileSpecList *module_search_paths_ptr,
-      lldb::ModuleSP *old_module_sp_ptr, bool *did_create_ptr);
+      llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules, bool *did_create_ptr);
 
   struct SDKEnumeratorInfo {
     lldb_private::FileSpec found_path;
@@ -158,7 +158,7 @@ class PlatformDarwin : public PlatformPOSIX {
       const lldb_private::ModuleSpec &module_spec,
       lldb_private::Process *process, lldb::ModuleSP &module_sp,
       const lldb_private::FileSpecList *module_search_paths_ptr,
-      lldb::ModuleSP *old_module_sp_ptr, bool *did_create_ptr);
+      llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules, bool *did_create_ptr);
 
   static std::string FindComponentInPath(llvm::StringRef path,
                                          llvm::StringRef component);

lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.cpp

@@ -644,8 +644,8 @@ bool PlatformDarwinKernel::KernelHasdSYMSibling(const FileSpec &kernel_binary) {
 
 Status PlatformDarwinKernel::GetSharedModule(
     const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
-    const FileSpecList *module_search_paths_ptr, ModuleSP *old_module_sp_ptr,
-    bool *did_create_ptr) {
+    const FileSpecList *module_search_paths_ptr,
+    llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
   Status error;
   module_sp.reset();
   const FileSpec &platform_file = module_spec.GetFileSpec();
@@ -676,7 +676,7 @@ Status PlatformDarwinKernel::GetSharedModule(
     // framework on macOS systems, a chance.
     error = PlatformDarwin::GetSharedModule(module_spec, process, module_sp,
                                            module_search_paths_ptr,
-                                           old_module_sp_ptr, did_create_ptr);
+                                           old_modules, did_create_ptr);
     if (error.Success() && module_sp.get()) {
       return error;
     }

lldb/source/Plugins/Platform/MacOSX/PlatformDarwinKernel.h

@@ -57,7 +57,7 @@ class PlatformDarwinKernel : public PlatformDarwin {
   GetSharedModule(const lldb_private::ModuleSpec &module_spec,
                   lldb_private::Process *process, lldb::ModuleSP &module_sp,
                   const lldb_private::FileSpecList *module_search_paths_ptr,
-                  lldb::ModuleSP *old_module_sp_ptr,
+                  llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
                   bool *did_create_ptr) override;
 
   bool GetSupportedArchitectureAtIndex(uint32_t idx,

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.cpp

@@ -292,10 +292,10 @@ lldb_private::Status PlatformMacOSX::GetSharedModule(
     const lldb_private::ModuleSpec &module_spec, Process *process,
     lldb::ModuleSP &module_sp,
     const lldb_private::FileSpecList *module_search_paths_ptr,
-    lldb::ModuleSP *old_module_sp_ptr, bool *did_create_ptr) {
-  Status error = GetSharedModuleWithLocalCache(
-      module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-      did_create_ptr);
+    llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules, bool *did_create_ptr) {
+  Status error = GetSharedModuleWithLocalCache(module_spec, module_sp,
+                                               module_search_paths_ptr,
+                                               old_modules, did_create_ptr);
 
   if (module_sp) {
     if (module_spec.GetArchitecture().GetCore() ==
@@ -306,15 +306,16 @@ lldb_private::Status PlatformMacOSX::GetSharedModule(
         ModuleSpec module_spec_x86_64(module_spec);
         module_spec_x86_64.GetArchitecture() = ArchSpec("x86_64-apple-macosx");
         lldb::ModuleSP x86_64_module_sp;
-        lldb::ModuleSP old_x86_64_module_sp;
+        llvm::SmallVector<lldb::ModuleSP, 1> old_x86_64_modules;
         bool did_create = false;
         Status x86_64_error = GetSharedModuleWithLocalCache(
             module_spec_x86_64, x86_64_module_sp, module_search_paths_ptr,
-            &old_x86_64_module_sp, &did_create);
+            &old_x86_64_modules, &did_create);
         if (x86_64_module_sp && x86_64_module_sp->GetObjectFile()) {
           module_sp = x86_64_module_sp;
-          if (old_module_sp_ptr)
-            *old_module_sp_ptr = old_x86_64_module_sp;
+          if (old_modules)
+            old_modules->append(old_x86_64_modules.begin(),
+                                old_x86_64_modules.end());
           if (did_create_ptr)
             *did_create_ptr = did_create;
           return x86_64_error;
@@ -324,7 +325,9 @@ lldb_private::Status PlatformMacOSX::GetSharedModule(
   }
 
   if (!module_sp) {
-      error = FindBundleBinaryInExecSearchPaths (module_spec, process, module_sp, module_search_paths_ptr, old_module_sp_ptr, did_create_ptr);
+    error = FindBundleBinaryInExecSearchPaths(module_spec, process, module_sp,
+                                              module_search_paths_ptr,
+                                              old_modules, did_create_ptr);
   }
   return error;
 }

lldb/source/Plugins/Platform/MacOSX/PlatformMacOSX.h

@@ -40,7 +40,7 @@ class PlatformMacOSX : public PlatformDarwin {
   GetSharedModule(const lldb_private::ModuleSpec &module_spec,
                   lldb_private::Process *process, lldb::ModuleSP &module_sp,
                   const lldb_private::FileSpecList *module_search_paths_ptr,
-                  lldb::ModuleSP *old_module_sp_ptr,
+                  llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
                   bool *did_create_ptr) override;
 
   const char *GetDescription() override {

lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.cpp

@@ -504,8 +504,8 @@ Status PlatformRemoteDarwinDevice::GetSymbolFile(const FileSpec &platform_file,
 
 Status PlatformRemoteDarwinDevice::GetSharedModule(
     const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
-    const FileSpecList *module_search_paths_ptr, ModuleSP *old_module_sp_ptr,
-    bool *did_create_ptr) {
+    const FileSpecList *module_search_paths_ptr,
+    llvm::SmallVectorImpl<ModuleSP> *old_modules, bool *did_create_ptr) {
   // For iOS, the SDK files are all cached locally on the host system. So first
   // we ask for the file in the cached SDK, then we attempt to get a shared
   // module for the right architecture with the right UUID.
@@ -608,24 +608,25 @@ Status PlatformRemoteDarwinDevice::GetSharedModule(
   // This may not be an SDK-related module.  Try whether we can bring in the
   // thing to our local cache.
   error = GetSharedModuleWithLocalCache(module_spec, module_sp,
-                                        module_search_paths_ptr,
-                                        old_module_sp_ptr, did_create_ptr);
+                                        module_search_paths_ptr, old_modules,
+                                        did_create_ptr);
   if (error.Success())
     return error;
 
   // See if the file is present in any of the module_search_paths_ptr
   // directories.
   if (!module_sp)
-    error = PlatformDarwin::FindBundleBinaryInExecSearchPaths (module_spec, process, module_sp,
-            module_search_paths_ptr, old_module_sp_ptr, did_create_ptr);
+    error = PlatformDarwin::FindBundleBinaryInExecSearchPaths(
+        module_spec, process, module_sp, module_search_paths_ptr, old_modules,
+        did_create_ptr);
 
   if (error.Success())
     return error;
 
   const bool always_create = false;
-  error = ModuleList::GetSharedModule(
-      module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-      did_create_ptr, always_create);
+  error = ModuleList::GetSharedModule(module_spec, module_sp,
+                                      module_search_paths_ptr, old_modules,
+                                      did_create_ptr, always_create);
 
   if (module_sp)
     module_sp->SetPlatformFileSpec(platform_file);

lldb/source/Plugins/Platform/MacOSX/PlatformRemoteDarwinDevice.h

@@ -38,7 +38,7 @@ class PlatformRemoteDarwinDevice : public PlatformDarwin {
   GetSharedModule(const lldb_private::ModuleSpec &module_spec,
                   lldb_private::Process *process, lldb::ModuleSP &module_sp,
                   const lldb_private::FileSpecList *module_search_paths_ptr,
-                  lldb::ModuleSP *old_module_sp_ptr,
+                  llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules,
                   bool *did_create_ptr) override;
 
   void

lldb/source/Target/Platform.cpp

@@ -218,15 +218,14 @@ Platform::LocateExecutableScriptingResources(Target *target, Module &module,
 //    return PlatformSP();
 //}
 
-Status Platform::GetSharedModule(const ModuleSpec &module_spec,
-                                 Process *process, ModuleSP &module_sp,
-                                 const FileSpecList *module_search_paths_ptr,
-                                 ModuleSP *old_module_sp_ptr,
-                                 bool *did_create_ptr) {
+Status Platform::GetSharedModule(
+    const ModuleSpec &module_spec, Process *process, ModuleSP &module_sp,
+    const FileSpecList *module_search_paths_ptr,
+    llvm::SmallVectorImpl<lldb::ModuleSP> *old_modules, bool *did_create_ptr) {
   if (IsHost())
-    return ModuleList::GetSharedModule(
-        module_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-        did_create_ptr, false);
+    return ModuleList::GetSharedModule(module_spec, module_sp,
+                                       module_search_paths_ptr, old_modules,
+                                       did_create_ptr, false);
 
   // Module resolver lambda.
   auto resolver = [&](const ModuleSpec &spec) {
@@ -239,17 +238,17 @@ Status Platform::GetSharedModule(const ModuleSpec &module_spec,
       resolved_spec.GetFileSpec().PrependPathComponent(
           m_sdk_sysroot.GetStringRef());
       // Try to get shared module with resolved spec.
-      error = ModuleList::GetSharedModule(
-          resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-          did_create_ptr, false);
+      error = ModuleList::GetSharedModule(resolved_spec, module_sp,
+                                          module_search_paths_ptr, old_modules,
+                                          did_create_ptr, false);
     }
     // If we don't have sysroot or it didn't work then
     // try original module spec.
     if (!error.Success()) {
       resolved_spec = spec;
-      error = ModuleList::GetSharedModule(
-          resolved_spec, module_sp, module_search_paths_ptr, old_module_sp_ptr,
-          did_create_ptr, false);
+      error = ModuleList::GetSharedModule(resolved_spec, module_sp,
+                                          module_search_paths_ptr, old_modules,
+                                          did_create_ptr, false);
     }
     if (error.Success() && module_sp)
       module_sp->SetPlatformFileSpec(resolved_spec.GetFileSpec());