[lldb/dyld-posix] Avoid reading the module list in inconsistent states

[labath]

Summary

New glibc versions (since 2.34 or including this
https://github.com/bminor/glibc/commit/ed3ce71f5c64c5f07cbde0ef03554ea8950d8f2c
patch) trigger the rendezvous breakpoint after they have already added
some modules to the list. This did not play well with our dynamic
loader plugin which was doing a diff of the the reported modules in the
before (RT_ADD) and after (RT_CONSISTENT) states. Specifically, it
caused us to miss some of the modules.

While I think the old behavior makes more sense, I don't think that lldb
is doing the right thing either, as the documentation states that we
should not be expecting a consistent view in the RT_ADD (and RT_DELETE)
states.

Therefore, this patch changes the lldb algorithm to compare the module
list against the previous consistent snapshot. This fixes the previous
issue, and I believe it is more correct in general. It also reduces the
number of times we are fetching the module info, which should speed up
the debugging of processes with many shared libraries.

The change in RefreshModules ensures we don't broadcast the loaded
notification for the dynamic loader (ld.so) module more than once.

[labath]

@emrekultursay: Mainly added you in case you want to try this out on android.

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

425

@mgorny: I suspect this workaround may no longer be needed.

[labath]

(This was breaking several shared library tests, most notably TestLoadUnload)

[emrekultursay]

Mainly added you in case you want to try this out on android.

Thanks. I tested this on a few Android API levels, and it looks good to me.

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

441

Use this variable below, instead of calling lock() again?

[labath]

reuse the module pointer

[labath]

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

441

Yes. In fact, that's why I created that variable in the first place. :)

[labath]

@mgorny, ping. Do you maybe want to try this out on freebsd?

[mgorny]

The patch itself looks good, however the workaround seems still required on FreeBSD.

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

425

Well, unless I've done it wrong, removing it causes a lot of test regressions.

[labath]

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

425

That is unfortunate, but thanks for checking it out. I still suspect there is some way we could tweak the code to make this unnecessary (IIUC, the main problem here was the lack of an "add" notification but this patch essentially makes lldb ignore all add notifications), but it needs more investigation.

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

case eAdd:
       return eAddModules;
     case eDelete:
       return eRemoveModules;
     }
     break;
 
   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;
-
-    return eTakeSnapshot;
   }
 
   return eNoAction;
 }
 
 bool DYLDRendezvous::UpdateSOEntriesFromRemote() {
   auto action = GetAction();
 
   if (action == eNoAction)
     return false;
 
-  if (action == eTakeSnapshot) {
-    m_added_soentries.clear();
-    m_removed_soentries.clear();
+  if (action == eTakeSnapshot) {
     // 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();
case eRemoveModules:
     return RemoveSOEntriesFromRemote(*module_list);
   case eNoAction:
     return false;
   }
   llvm_unreachable("Fully covered switch above!");
 }
 
 bool DYLDRendezvous::UpdateSOEntries() {
+  m_added_soentries.clear();
+  m_removed_soentries.clear();
   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;
   }

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

void DynamicLoaderPOSIXDYLD::RefreshModules() {
 
   if (m_rendezvous.ModulesDidLoad() || !m_initial_modules_added) {
     ModuleList new_modules;
 
     // If this is the first time rendezvous breakpoint fires, we need
     // to take care of adding all the initial modules reported by
     // the loader.  This is necessary to list ld-linux.so on Linux,
     // and all DT_NEEDED entries on *BSD.
     if (m_initial_modules_added) {
       I = m_rendezvous.loaded_begin();
       E = m_rendezvous.loaded_end();
     } else {
       I = m_rendezvous.begin();
       E = m_rendezvous.end();
       m_initial_modules_added = true;
     }
     for (; I != E; ++I) {
       ModuleSP module_sp =
           LoadModuleAtAddress(I->file_spec, I->link_addr, I->base_addr, true);
-      if (module_sp.get()) {
-        if (module_sp->GetObjectFile()->GetBaseAddress().GetLoadAddress(
-                &m_process->GetTarget()) == m_interpreter_base &&
-            module_sp != m_interpreter_module.lock()) {
-          if (m_interpreter_module.lock() == nullptr) {
-            m_interpreter_module = module_sp;
-          } else {
-            // If this is a duplicate instance of ld.so, unload it.  We may end
-            // up with it if we load it via a different path than before
-            // (symlink vs real path).
-            // TODO: remove this once we either fix library matching or avoid
-            // loading the interpreter when setting the rendezvous breakpoint.
-            UnloadSections(module_sp);
-            loaded_modules.Remove(module_sp);
-            continue;
-          }
-        }
+      if (!module_sp.get())
+        continue;
+
+              &m_process->GetTarget()) == m_interpreter_base) {
+        ModuleSP interpreter_sp = m_interpreter_module.lock();
+        } else if (module_sp == interpreter_sp) {
+          // Module already loaded.
+          continue;
 
-        loaded_modules.AppendIfNeeded(module_sp);
-        new_modules.Append(module_sp);
-      }
-    }
     m_process->GetTarget().ModulesDidLoad(new_modules);
   }
 
   if (m_rendezvous.ModulesDidUnload()) {
     ModuleList old_modules;
 
     E = m_rendezvous.unloaded_end();
     for (I = m_rendezvous.unloaded_begin(); I != E; ++I) {