Avoid calling LoadModules twice when modules change

[aadsm]

Summary

This is the sixth and final patch to improve module loading in a series that started here (where I explain the motivation and solution): D62499

When xfer:libraries-svr4 is available the Process class has a LoadModules function that requests this packet and then loads or unloads modules based on the current list of loaded modules by the process.
This is function is used by DYLDRendezvous to get the list of loaded modules before and after the module is loaded. However, this is really not needed since the LoadModules function already loaded or unloaded the modules accordingly. I changed this strategy to call LoadModules only once (after the process has loaded the module).
I also removed the FromRemote functions & friends because they were only maintaining the m_soentries structure that is never being used in this context. (I hope I'm not missing anything obvious here).

[aadsm]

Do not try to use LoadModules again if they failed in the past

[xiaobai]

ping @labath @clayborg any issues with this one?

[labath]

Sorry, I missed this patch. Overall, this seems fine to me. I have only two comments about it:

• I'm not sure the CanLoadModules functions is really needed. It seems like we could just call LoadModules directly and check for error result. The XMLEnabled() && ... check could be done at the start of the LoadModules function. It looks like it already is there ProcessGDBRemote::GetLoadedModuleList, but it looks like that function accidentally returns a success error code instead of failure.
• Looking at the implementation of ProcessGDBRemote::LoadModules, I get the feeling it will return 0 if you call it when a module gets unloaded (although it will correctly unload the module, and correctly *not* load any new module). This is not a bug in this patch, so I'd be fine with checking it in without fixing that, but this sounds like something you would want to fix, as it will cause you to stop using the gdb-remote packet as soon as the first module is unloaded.

It looks like a single solution fixing both of these issues would be to change the LoadModules function to return Expected<size_t>. That way it could report the error states explicitly (such as when the svr packet is not supported or it just returns bogus data), and zero when it correctly did not load anything. (Or, given that the number of loaded modules is not particularly useful, and it does even not capture the effect of the function very precisely, the result could just be an Error).

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp

177

use a real type here.

[jankratochvil]

So it does fix the massive regression of D62503 similarly as my D63868 but it still has 2 regressions there (=Fedora 30 x86_64, after reapplication of D62503), I haven't yet investigated why:

lldb-Suite :: functionalities/load_unload/TestLoadUnload.py
lldb-Suite :: functionalities/load_using_paths/TestLoadUsingPaths.py

[aadsm]

@jankratochvil I imagine the first test to be failing because this patch doesn't handle correctly the unload (or rather as soon as it unloads one shared object it stops working as expected).
I'm going to look at this today (right now it's 9am for me).

[aadsm]

I looked into this a bit today and at one point in time (when the list of modules is being processed) it seems that some previously loaded modules are somehow removed (at least I stop seeing them in the image list) including the one where the rendezvous breakpoint is so that breakpoint becomes "unresolved" and that's why it doesn't stop anymore. I'll need to check why this is the case tomorrow.

[aadsm]

Replaced by D62503

lldb/include/lldb/Target/Process.h

/// UnregisterNotificationCallbacks (const Notifications&)
   /// shared library load state.
   ///
   /// \return
   ///    The number of shared libraries that were loaded
   virtual size_t LoadModules() { return 0; }
 
   virtual size_t LoadModules(LoadedModuleInfoList &) { return 0; }
 
+  virtual bool CanLoadModules() { return false; }
+
 protected:
   virtual JITLoaderList &GetJITLoaders();
 
 public:
   /// Get the system runtime plug-in for this process.
   ///
   /// \return
   ///   Returns a pointer to the SystemRuntime plugin for this Process

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h

protected:
   /// List of SOEntry's added to the link map since the last call to
   /// Resolve().
   SOEntryList m_added_soentries;
 
   /// List of SOEntry's removed from the link map since the last call to
   /// Resolve().
   SOEntryList m_removed_soentries;
 
+  /// By default LoadModules is used (when available) to load or unload module
+  /// changes. If LoadModules fails for some reason then we stop using it.
+  bool m_should_load_modules = true;
+
   /// Threading metadata read from the inferior.
   ThreadInfo m_thread_info;
 
   /// Reads an unsigned integer of \p size bytes from the inferior's address
   /// space starting at \p addr.
   ///
   /// \returns addr + size if the read was successful and false otherwise.
   lldb::addr_t ReadWord(lldb::addr_t addr, uint64_t *dst, size_t size);
 
   /// Reads an address from the inferior's address space starting at \p addr.
   ///
   /// \returns addr + target address size if the read was successful and
   /// 0 otherwise.
   lldb::addr_t ReadPointer(lldb::addr_t addr, lldb::addr_t *dst);
 
   /// Reads a null-terminated C string from the memory location starting at @p
   /// addr.
   std::string ReadStringFromMemory(lldb::addr_t addr);
 
   /// Reads an SOEntry starting at \p addr.
   bool ReadSOEntryFromMemory(lldb::addr_t addr, SOEntry &entry);
 
   /// Updates the current set of SOEntries, the set of added entries, and the
   /// set of removed entries.
-  bool UpdateSOEntries(bool fromRemote = false);
-
-  bool FillSOEntryFromModuleInfo(
-      LoadedModuleInfoList::LoadedModuleInfo const &modInfo, SOEntry &entry);
-
-  bool SaveSOEntriesFromRemote(LoadedModuleInfoList &module_list);
-
-  bool AddSOEntriesFromRemote(LoadedModuleInfoList &module_list);
-
-  bool RemoveSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+  bool UpdateSOEntries();
 
   bool AddSOEntries();
 
   bool RemoveSOEntries();
 
   void UpdateBaseAddrIfNecessary(SOEntry &entry, std::string const &file_path);
 
   bool SOEntryIsMainExecutable(const SOEntry &entry);

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp

bool DYLDRendezvous::Resolve() {
   if (!(cursor = ReadPointer(cursor + padding, &info.ldbase)))
     return false;
 
   // The rendezvous was successfully read.  Update our internal state.
   m_rendezvous_addr = info_addr;
   m_previous = m_current;
   m_current = info;
 
-  if (UpdateSOEntries(true))
+  // Delegate the loading or unloading of modules to LoadModules when possible
+  // since it will be much faster than reading each library remotely one by one.
+  if (m_should_load_modules && m_process->CanLoadModules()) {
+    // No need to call LoadModules at any time other than eConsistent since
+    // nothing has changed until then.
+    if (m_current.state == eConsistent) {
+      auto modules_loaded = m_process->LoadModules();
+      // If LoadModules fails do not try to use it again.
+      m_should_load_modules = modules_loaded > 0;
+      return m_should_load_modules;
+    }
     return true;
+  }
 
   return UpdateSOEntries();
 }
 
 bool DYLDRendezvous::IsValid() {
   return m_rendezvous_addr != LLDB_INVALID_ADDRESS;
 }
 
-bool DYLDRendezvous::UpdateSOEntries(bool fromRemote) {
+bool DYLDRendezvous::UpdateSOEntries() {
   SOEntry entry;
   LoadedModuleInfoList module_list;
 
-  // If we can't get the SO info from the remote, return failure.
-  if (fromRemote && m_process->LoadModules(module_list) == 0)
-    return false;
-
-  if (!fromRemote && m_current.map_addr == 0)
+  if (m_current.map_addr == 0)
     return false;
 
   // When the previous and current states are consistent this is the first time
   // we have been asked to update.  Just take a snapshot of the currently
   // loaded modules.
   if (m_previous.state == eConsistent && m_current.state == eConsistent)
-    return fromRemote ? SaveSOEntriesFromRemote(module_list)
-                      : TakeSnapshot(m_soentries);
+    return TakeSnapshot(m_soentries);
 
   // If we are about to add or remove a shared object clear out the current
   // state and take a snapshot of the currently loaded images.
   if (m_current.state == eAdd || m_current.state == eDelete) {
     // Some versions of the android dynamic linker might send two notifications
     // with state == eAdd back to back. Ignore them until we get an eConsistent
     // notification.
     if (!(m_previous.state == eConsistent ||
           (m_previous.state == eAdd && m_current.state == eDelete)))
       return false;
 
     m_soentries.clear();
-    if (fromRemote)
-      return SaveSOEntriesFromRemote(module_list);
-
     m_added_soentries.clear();
     m_removed_soentries.clear();
     return TakeSnapshot(m_soentries);
   }
   assert(m_current.state == eConsistent);
 
   // Otherwise check the previous state to determine what to expect and update
   // accordingly.
   if (m_previous.state == eAdd)
-    return fromRemote ? AddSOEntriesFromRemote(module_list) : AddSOEntries();
+    return AddSOEntries();
   else if (m_previous.state == eDelete)
-    return fromRemote ? RemoveSOEntriesFromRemote(module_list)
-                      : RemoveSOEntries();
-
-  return false;
-}
-
-bool DYLDRendezvous::FillSOEntryFromModuleInfo(
-    LoadedModuleInfoList::LoadedModuleInfo const &modInfo, SOEntry &entry) {
-  addr_t link_map_addr;
-  addr_t base_addr;
-  addr_t dyn_addr;
-  std::string name;
-
-  if (!modInfo.get_link_map(link_map_addr) || !modInfo.get_base(base_addr) ||
-      !modInfo.get_dynamic(dyn_addr) || !modInfo.get_name(name))
-    return false;
-
-  entry.link_addr = link_map_addr;
-  entry.base_addr = base_addr;
-  entry.dyn_addr = dyn_addr;
-
-  entry.file_spec.SetFile(name, FileSpec::Style::native);
+    return RemoveSOEntries();
 
-  UpdateBaseAddrIfNecessary(entry, name);
-
-  // not needed if we're using ModuleInfos
-  entry.next = 0;
-  entry.prev = 0;
-  entry.path_addr = 0;
-
-  return true;
-}
-
-bool DYLDRendezvous::SaveSOEntriesFromRemote(
-    LoadedModuleInfoList &module_list) {
-  for (auto const &modInfo : module_list.m_list) {
-    SOEntry entry;
-    if (!FillSOEntryFromModuleInfo(modInfo, entry))
-      return false;
-
-    // Only add shared libraries and not the executable.
-    if (!SOEntryIsMainExecutable(entry))
-      m_soentries.push_back(entry);
-  }
-
-  m_loaded_modules = module_list;
-  return true;
-}
-
-bool DYLDRendezvous::AddSOEntriesFromRemote(LoadedModuleInfoList &module_list) {
-  for (auto const &modInfo : module_list.m_list) {
-    bool found = false;
-    for (auto const &existing : m_loaded_modules.m_list) {
-      if (modInfo == existing) {
-        found = true;
-        break;
-      }
-    }
-
-    if (found)
-      continue;
-
-    SOEntry entry;
-    if (!FillSOEntryFromModuleInfo(modInfo, entry))
-      return false;
-
-    // Only add shared libraries and not the executable.
-    if (!SOEntryIsMainExecutable(entry))
-      m_soentries.push_back(entry);
-  }
-
-  m_loaded_modules = module_list;
-  return true;
-}
-
-bool DYLDRendezvous::RemoveSOEntriesFromRemote(
-    LoadedModuleInfoList &module_list) {
-  for (auto const &existing : m_loaded_modules.m_list) {
-    bool found = false;
-    for (auto const &modInfo : module_list.m_list) {
-      if (modInfo == existing) {
-        found = true;
-        break;
-      }
-    }
-
-    if (found)
-      continue;
-
-    SOEntry entry;
-    if (!FillSOEntryFromModuleInfo(existing, entry))
-      return false;
-
-    // Only add shared libraries and not the executable.
-    if (!SOEntryIsMainExecutable(entry)) {
-      auto pos = std::find(m_soentries.begin(), m_soentries.end(), entry);
-      if (pos == m_soentries.end())
-        return false;
-
-      m_soentries.erase(pos);
-    }
-  }
-
-  m_loaded_modules = module_list;
-  return true;
 }
 
 bool DYLDRendezvous::AddSOEntries() {
   SOEntry entry;
   iterator pos;
 
   assert(m_previous.state == eAdd);
 

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

void PrefetchModuleSpecs(llvm::ArrayRef<FileSpec> module_file_specs,
                            const llvm::Triple &triple) override;
 
   llvm::VersionTuple GetHostOSVersion() override;
 
   size_t LoadModules(LoadedModuleInfoList &module_list) override;
 
   size_t LoadModules() override;
 
+  bool CanLoadModules() override;
+
   Status GetFileLoadAddress(const FileSpec &file, bool &is_loaded,
                             lldb::addr_t &load_addr) override;
 
   void ModulesDidLoad(ModuleList &module_list) override;
 
   StructuredData::ObjectSP
   GetLoadedDynamicLibrariesInfos(lldb::addr_t image_list_address,
                                  lldb::addr_t image_count) override;

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

lldb::ModuleSP ProcessGDBRemote::LoadModuleAtAddress(const FileSpec &file,
   DynamicLoader *loader = GetDynamicLoader();
   if (!loader)
     return nullptr;
 
   return loader->LoadModuleAtAddress(file, link_map, base_addr,
                                      value_is_offset);
 }
 
+bool ProcessGDBRemote::CanLoadModules() {
+  return XMLDocument::XMLEnabled() && m_gdb_comm.GetQXferLibrariesSVR4ReadSupported();
+}
+
 size_t ProcessGDBRemote::LoadModules(LoadedModuleInfoList &module_list) {
   using lldb_private::process_gdb_remote::ProcessGDBRemote;
 
   // request a list of loaded libraries from GDBServer
   if (GetLoadedModuleList(module_list).Fail())
     return 0;
 
   // get a list of all the modules