Correctly use GetLoadedModuleList to take advantage of libraries-svr4

Summary:
Here's a replacement for D62504. I thought I could use LoadModules to implement this but in reality I can't because there are at few issues with it:
* The LoadModules assumes that the list returned by GetLoadedModuleList is comprehensive in the sense that reflects all the mapped segments, however, this is not true, for instance VDSO entry is not there since it's loaded manually by LoadVDSO using GetMemoryRegionInfo and it doesn't represent a specific shared object in disk. Because of this LoadModules will unload the VDSO module.
* The loader (interpreter) module might have also been loaded using GetMemoryRegionInfo, this is true when we launch the process and the rendezvous structure is not yet available (done through LoadInterpreterModule()). The problem here is that this entry will point to the same file name as the one found in /proc/pid/maps, however, when we read the same module from the r_debug.link_map structure it might be under a different name. This is true at least on CentOS where the loader is a symlink. Because of this LoadModules will unload and load the module in a way where the rendezvous breakpoint is unresolved but not resolved again (because we add the new module first and remove the old one after).

The symlink issue might be fixable by first unloading the old and loading the news (but sounds super brittle), however, I'm not sure how to fix the VDSO issue.
Since I can't trust it I'm just going to use GetLoadedModuleList directly with the same logic that we use today for when we read the linked list in lldb. The only safe thing to do here is to only calculate differences between different snapshots of the svr4 packet itself. This will also cut the dependency this plugin has from LoadModules.

I separated the 2 logics into 2 different functions (remote and not remote) because I don't like mixing 2 different logics in the same function with if/else's. Two different functions makes it easier to reason with I believe. However, I did abstract away the logic that decides if we should take a snapshot or add/remove modules so both functions could reuse it.

The other difference between the two is that on the UpdateSOEntriesFromRemote I take the snapshot only once when state = Consistent because I didn't find a good reason to always update that, as we already got the list from state = Add | Remove. I probably should use the same logic on UpdateSOEntries though I don't see a reason not to since it's really using the same data, just read in different places. Any thoughts here?

It might also be worthwhile to add a test to make sure we don't unload modules that were not actually "unloaded" like the vdso. I haven't done this yet though.
This diff is also missing the option for svr4 like proposed in https://reviews.llvm.org/D62503#1564296, I'll start working on this but wanted to have this up first.

Reviewers: labath, jankratochvil, clayborg, xiaobai

Reviewed By: labath

Subscribers: srhines, JDevlieghere, lldb-commits

Tags: #lldb

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

llvm-svn: 367020

4 files changed, 109 insertions, 48 deletions

diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
index bc5b480c674..d935c99e4e7 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
@@ -160,7 +160,10 @@ bool DYLDRendezvous::Resolve() {
   m_previous = m_current;
   m_current = info;
 
-  if (UpdateSOEntries(true))
+  if (m_current.map_addr == 0)
+    return false;
+
+  if (UpdateSOEntriesFromRemote())
     return true;
 
   return UpdateSOEntries();
@@ -170,53 +173,89 @@ bool DYLDRendezvous::IsValid() {
   return m_rendezvous_addr != LLDB_INVALID_ADDRESS;
 }
 
-bool DYLDRendezvous::UpdateSOEntries(bool fromRemote) {
-  SOEntry entry;
-  LoadedModuleInfoList module_list;
+DYLDRendezvous::RendezvousAction DYLDRendezvous::GetAction() const {
+  switch (m_current.state) {
+
+  case eConsistent:
+    switch (m_previous.state) {
+    // 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.
+    case eConsistent:
+      return eTakeSnapshot;
+    // 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.
+    case eAdd:
+      return eAddModules;
+    case eDelete:
+      return eRemoveModules;
+    }
+    break;
 
-  // If we can't get the SO info from the remote, return failure.
-  if (fromRemote && m_process->LoadModules(module_list) == 0)
-    return false;
+  case eAdd:
+  case 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 eNoAction;
 
-  if (!fromRemote && m_current.map_addr == 0)
+    return eTakeSnapshot;
+  }
+
+  return eNoAction;
+}
+
+bool DYLDRendezvous::UpdateSOEntriesFromRemote() {
+  auto action = GetAction();
+
+  if (action == eNoAction)
     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);
-
-  // 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;
+  if (action == eTakeSnapshot) {
+    m_added_soentries.clear();
+    m_removed_soentries.clear();
+    // We already have the loaded list from the previous update so no need to
+    // find all the modules again.
+    if (!m_loaded_modules.m_list.empty())
+      return true;
+  }
+
+  llvm::Expected<LoadedModuleInfoList> module_list =
+      m_process->GetLoadedModuleList();
+  if (!module_list) {
+    llvm::consumeError(module_list.takeError());
+    return false;
+  }
 
+  switch (action) {
+  case eTakeSnapshot:
     m_soentries.clear();
-    if (fromRemote)
-      return SaveSOEntriesFromRemote(module_list);
+    return SaveSOEntriesFromRemote(*module_list);
+  case eAddModules:
+    return AddSOEntriesFromRemote(*module_list);
+  case eRemoveModules:
+    return RemoveSOEntriesFromRemote(*module_list);
+  case eNoAction:
+    return false;
+  }
+}
 
+bool DYLDRendezvous::UpdateSOEntries() {
+  switch (GetAction()) {
+  case eTakeSnapshot:
+    m_soentries.clear();
     m_added_soentries.clear();
     m_removed_soentries.clear();
     return TakeSnapshot(m_soentries);
+  case eAddModules:
+    return AddSOEntries();
+  case eRemoveModules:
+    return RemoveSOEntries();
+  case eNoAction:
+    return false;
   }
-  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();
-  else if (m_previous.state == eDelete)
-    return fromRemote ? RemoveSOEntriesFromRemote(module_list)
-                      : RemoveSOEntries();
-
-  return false;
 }
 
 bool DYLDRendezvous::FillSOEntryFromModuleInfo(
@@ -247,7 +286,7 @@ bool DYLDRendezvous::FillSOEntryFromModuleInfo(
 }
 
 bool DYLDRendezvous::SaveSOEntriesFromRemote(
-    LoadedModuleInfoList &module_list) {
+    const LoadedModuleInfoList &module_list) {
   for (auto const &modInfo : module_list.m_list) {
     SOEntry entry;
     if (!FillSOEntryFromModuleInfo(modInfo, entry))
@@ -262,7 +301,8 @@ bool DYLDRendezvous::SaveSOEntriesFromRemote(
   return true;
 }
 
-bool DYLDRendezvous::AddSOEntriesFromRemote(LoadedModuleInfoList &module_list) {
+bool DYLDRendezvous::AddSOEntriesFromRemote(
+    const LoadedModuleInfoList &module_list) {
   for (auto const &modInfo : module_list.m_list) {
     bool found = false;
     for (auto const &existing : m_loaded_modules.m_list) {
@@ -280,8 +320,10 @@ bool DYLDRendezvous::AddSOEntriesFromRemote(LoadedModuleInfoList &module_list) {
       return false;
 
     // Only add shared libraries and not the executable.
-    if (!SOEntryIsMainExecutable(entry))
+    if (!SOEntryIsMainExecutable(entry)) {
       m_soentries.push_back(entry);
+      m_added_soentries.push_back(entry);
+    }
   }
 
   m_loaded_modules = module_list;
@@ -289,7 +331,7 @@ bool DYLDRendezvous::AddSOEntriesFromRemote(LoadedModuleInfoList &module_list) {
 }
 
 bool DYLDRendezvous::RemoveSOEntriesFromRemote(
-    LoadedModuleInfoList &module_list) {
+    const LoadedModuleInfoList &module_list) {
   for (auto const &existing : m_loaded_modules.m_list) {
     bool found = false;
     for (auto const &modInfo : module_list.m_list) {
@@ -313,6 +355,7 @@ bool DYLDRendezvous::RemoveSOEntriesFromRemote(
         return false;
 
       m_soentries.erase(pos);
+      m_removed_soentries.push_back(entry);
     }
   }
 
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
index 993e62f5e9f..536eeeaaf33 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
@@ -222,16 +222,20 @@ protected:
 
   /// Updates the current set of SOEntries, the set of added entries, and the
   /// set of removed entries.
-  bool UpdateSOEntries(bool fromRemote = false);
+  bool UpdateSOEntries();
+
+  /// Same as UpdateSOEntries but it gets the list of loaded modules from the
+  /// remote debug server (faster when supported).
+  bool UpdateSOEntriesFromRemote();
 
   bool FillSOEntryFromModuleInfo(
       LoadedModuleInfoList::LoadedModuleInfo const &modInfo, SOEntry &entry);
 
-  bool SaveSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+  bool SaveSOEntriesFromRemote(const LoadedModuleInfoList &module_list);
 
-  bool AddSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+  bool AddSOEntriesFromRemote(const LoadedModuleInfoList &module_list);
 
-  bool RemoveSOEntriesFromRemote(LoadedModuleInfoList &module_list);
+  bool RemoveSOEntriesFromRemote(const LoadedModuleInfoList &module_list);
 
   bool AddSOEntries();
 
@@ -248,6 +252,17 @@ protected:
   enum PThreadField { eSize, eNElem, eOffset };
 
   bool FindMetadata(const char *name, PThreadField field, uint32_t &value);
+
+  enum RendezvousAction {
+    eNoAction,
+    eTakeSnapshot,
+    eAddModules,
+    eRemoveModules
+  };
+
+  /// Returns the current action to be taken given the current and previous
+  /// state
+  RendezvousAction GetAction() const;
 };
 
 #endif
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
index 3ca73e1af15..da8ba29125c 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -94,7 +94,8 @@ void DynamicLoaderPOSIXDYLD::DidAttach() {
       __FUNCTION__, m_process ? m_process->GetID() : LLDB_INVALID_PROCESS_ID);
 
   // ask the process if it can load any of its own modules
-  m_process->LoadModules();
+  auto error = m_process->LoadModules();
+  LLDB_LOG_ERROR(log, std::move(error), "Couldn't load modules: {0}");
 
   ModuleSP executable_sp = GetTargetExecutable();
   ResolveExecutableModule(executable_sp);
diff --git a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
index aa007805c9f..25ab30e9db9 100644
--- a/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp
@@ -146,7 +146,8 @@ void DynamicLoaderWindowsDYLD::DidAttach() {
     ModuleList module_list;
     module_list.Append(executable);
     m_process->GetTarget().ModulesDidLoad(module_list);
-    m_process->LoadModules();
+    auto error = m_process->LoadModules();
+    LLDB_LOG_ERROR(log, std::move(error), "failed to load modules: {0}");
 }
 
 void DynamicLoaderWindowsDYLD::DidLaunch() {
@@ -165,7 +166,8 @@ void DynamicLoaderWindowsDYLD::DidLaunch() {
     ModuleList module_list;
     module_list.Append(executable);
     m_process->GetTarget().ModulesDidLoad(module_list);
-    m_process->LoadModules();
+    auto error = m_process->LoadModules();
+    LLDB_LOG_ERROR(log, std::move(error), "failed to load modules: {0}");
   }
 }