Unify+fix remote XML libraries handling with the legacy one

[jankratochvil]

Summary

D62503 broke Fedora 30 x86_64. That is because it fixed failing ReadMemory (due to PAGE_SIZE size reading shared library name overlapping to a next page which is accidentally unmapped) there and thus enabling to really use the XML libraries protocol which broke LLDB because then DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit happens only once and any further library updates are ignored.
So D62503 should be reapplied after this fix as it is safe then.
In this patch I do not understand why there should be any special add/remove handling in ProcessGDBRemote::LoadModules, it is just a new retrieved list of libraries and loading their modules is handled by later code the same way as it always worked with legacy memory reading from the list at _r_debug.
There are some more opportunities how to unify the XML protocol vs. legacy libraries handling but I wanted to first fix this pending regression.

[jankratochvil]

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

185

This change is not really required but I do not see why the map_addr check should differ when the remote XML protocol is in use.

[aadsm]

I'm not sure why DynamicLoaderPOSIXDYLD::RendezvousBreakpointHit happens only once, I thought I had fixed that here: D62168.
I don't think we can stop loading/unloading the libraries in ProcessGDBRemote::LoadModules. The windows dynamic loader relies on this and the library entry on the stop packet does as well: https://github.com/llvm-mirror/lldb/blob/9689521b477c75b52257fba9655cabefc3db676c/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp#L2374-L2375

I actually proposed a different way of loading the libraries here (and it's faster because it reduces the generation of libraries-svr4 in half): D62504. What do you think?

[jankratochvil]

In D63868#1560872, @aadsm wrote:

I don't think we can stop loading/unloading the libraries in ProcessGDBRemote::LoadModules. The windows dynamic loader relies on this

Then it could be Windows-specific.

and the library entry on the stop packet does as well: https://github.com/llvm-mirror/lldb/blob/9689521b477c75b52257fba9655cabefc3db676c/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp#L2374-L2375

When is this library entry being used?

For implementing fast shared libraries introspection one should implement what glibc+gdb have - STAP (SystemTap) probes: Those are few breakpoints each directly reporting the one added/removed library as one can see from GDB:

(gdb) maintenance info breakpoints
(gdb) info probes
Num     Type                  Disp Enb Address            What
Type Provider   Name                   Where              Semaphore Object   
-2      shlib events          keep y   0x00007ffff7fd67a5 <dl_main+8517> inf 1
stap rtld       init_complete          0x00007ffff7fd67a5           /lib64/ld-linux-x86-64.so.2
-5      shlib events          keep y   0x00007ffff7fe61a0 <dl_open_worker+1488> inf 1
stap rtld       reloc_complete         0x00007ffff7fe61a0           /lib64/ld-linux-x86-64.so.2
-7      shlib events          keep y   0x00007ffff7fe715e <_dl_close_worker+3070> inf 1
stap rtld       unmap_complete         0x00007ffff7fe715e           /lib64/ld-linux-x86-64.so.2

[aadsm]

The library is part of the GDB protocol itself: https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets and it's to be used with conjunction of qXfer:libraries:read (or libraries-svr4 like we do here).

Interesting, I'm not familiar with STAP but will take a look. I do think however that changing how we load the libraries is outside the scope of this diff. My main goal is to add the libraries-svr4 packet, but while doing so I found a few bugs in lldb, which I'm addressing here.

[labath]

@jankratochvil wrote:

That is because it fixed failing ReadMemory (due to PAGE_SIZE size reading shared library name overlapping to a next page which is accidentally unmapped)

Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?

In D63868#1560990, @jankratochvil wrote:

In D63868#1560872, @aadsm wrote:

I don't think we can stop loading/unloading the libraries in ProcessGDBRemote::LoadModules. The windows dynamic loader relies on this

Then it could be Windows-specific.

I also think it would be good to move this stuff into the windows loader. Right now we have a lot of confusion about who is responsible for "loading" modules into a target. Some processes (I think mostly post-mortem plugins) do the loading themselves. Others create dynamic loader plugins to do that. And in the case of svr4, we have ProcessGDBRemote creating a DynamicLoader plugin, which then calls back into the process to do the actual loading. That seems too convoluted to me...

I think it would be better to do the loading inside the DynamicLoader plugin, and have ProcessGDBRemote::LoadModules be responsible only for sending the appropriate qXfer packet and parsing the response. The fact that loading the libraries from a LoadedModuleInfoList is largely similar for posix&windows can be handled (at a later date) by factoring the common code into a utility function, a common base class or something else...

[jankratochvil]

In D63868#1563802, @labath wrote:

Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?

OK, I will try to reproduce it - you are right GDB documentation says for the m packet: "The reply may contain fewer addressable memory units than requested if the server was able to read only part of the region of memory. "

And LLDB is using ReadMemory in NativeProcessProtocol::ReadMemoryWithoutTrap from GDBRemoteCommunicationServerLLGS::Handle_memory_read.

I think it would be better to do the loading inside the DynamicLoader plugin, and have ProcessGDBRemote::LoadModules be responsible only for sending the appropriate qXfer packet and parsing the response. The fact that loading the libraries from a LoadedModuleInfoList is largely similar for posix&windows can be handled (at a later date) by factoring the common code into a utility function, a common base class or something else...

IIUC you believe this patch should be more refactored? I do not have much overview of this code of LLDB (+Windows libraries in general). IIUC you propose:

• The current libraries loading code from LoadedModuleInfoList of ProcessGDBRemote::LoadModules moved to lldb/source/Plugins/Platform/Windows/
• later: DynamicLoaderPOSIXDYLD::RefreshModules and DynamicLoaderPOSIXDYLD::LoadAllCurrentModules somehow reusing that code (after it is moved back to some common library)? But that looks to me as too little code to make it worth it.

[labath]

In D63868#1563873, @jankratochvil wrote:

In D63868#1563802, @labath wrote:

Does this mean that there is a bug in lldb-server, where some memory read requests fail if they span an unreadable page. Can this also be triggered by a $m packet? Would you be interested in distilling that into a test?

OK, I will try to reproduce it - you are right GDB documentation says for the m packet: "The reply may contain fewer addressable memory units than requested if the server was able to read only part of the region of memory. "

And LLDB is using ReadMemory in NativeProcessProtocol::ReadMemoryWithoutTrap from GDBRemoteCommunicationServerLLGS::Handle_memory_read.

Thanks. To deterministically reproduce an unmapped page of memory, the best approach I can think of is to first mmap a larger block of memory, and then munmap a part of it.

I think it would be better to do the loading inside the DynamicLoader plugin, and have ProcessGDBRemote::LoadModules be responsible only for sending the appropriate qXfer packet and parsing the response. The fact that loading the libraries from a LoadedModuleInfoList is largely similar for posix&windows can be handled (at a later date) by factoring the common code into a utility function, a common base class or something else...

IIUC you believe this patch should be more refactored? I do not have much overview of this code of LLDB (+Windows libraries in general). IIUC you propose:

• The current libraries loading code from LoadedModuleInfoList of ProcessGDBRemote::LoadModules moved to lldb/source/Plugins/Platform/Windows/

Yes, that's more or less it. Though I am open to being convinced otherwise...

• later: DynamicLoaderPOSIXDYLD::RefreshModules and DynamicLoaderPOSIXDYLD::LoadAllCurrentModules somehow reusing that code (after it is moved back to some common library)? But that looks to me as too little code to make it worth it.

Possibly. If the same code is useful for both windows and posix, then it might make sense to put it into the base DynamicLoader class, which means we wouldn't have to create new plugins or whatnot. Or we might actually want to create a DynamicLoaderSVR4, which only works with SVR4, and have these delegate to it. But that's a discussion for another day...

[labath]

This should be no longer relevant, right?

[jankratochvil]

OK, right, closing it - I no longer see any functionality problem. Maybe one could do some more code refactorization but I do not have time for that now anyway.

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

 bool DYLDRendezvous::IsValid() {
   return m_rendezvous_addr != LLDB_INVALID_ADDRESS;
 }
 
 bool DYLDRendezvous::UpdateSOEntries(bool fromRemote) {
   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)
+  if (m_current.map_addr == 0)
     return false;
 
-  if (!fromRemote && m_current.map_addr == 0)
+  // If we can't get the SO info from the remote, return failure.
+  if (fromRemote && m_process->LoadModules(module_list) == 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);
 
   // 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);
+    return fromRemote ? SaveSOEntriesFromRemote(module_list)
+                      : 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();
   else if (m_previous.state == eDelete)
for (auto const &modInfo : module_list.m_list) {
     if (found)
       continue;
 
     SOEntry entry;
     if (!FillSOEntryFromModuleInfo(modInfo, entry))
       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;
   return true;
 }
 
 bool DYLDRendezvous::RemoveSOEntriesFromRemote(
     LoadedModuleInfoList &module_list) {
for (auto const &existing : m_loaded_modules.m_list) {
 
     // 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_removed_soentries.push_back(entry);
     }
   }
 
   m_loaded_modules = module_list;
   return true;
 }
 
 bool DYLDRendezvous::AddSOEntries() {

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

 
 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
-  ModuleList new_modules;
-
-  for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list.m_list) {
-    std::string mod_name;
-    lldb::addr_t mod_base;
-    lldb::addr_t link_map;
-    bool mod_base_is_offset;
-
-    bool valid = true;
-    valid &= modInfo.get_name(mod_name);
-    valid &= modInfo.get_base(mod_base);
-    valid &= modInfo.get_base_is_offset(mod_base_is_offset);
-    if (!valid)
-      continue;
-
-    if (!modInfo.get_link_map(link_map))
-      link_map = LLDB_INVALID_ADDRESS;
-
-    FileSpec file(mod_name);
-    FileSystem::Instance().Resolve(file);
-    lldb::ModuleSP module_sp =
-        LoadModuleAtAddress(file, link_map, mod_base, mod_base_is_offset);
-
-    if (module_sp.get())
-      new_modules.Append(module_sp);
-  }
-
-  if (new_modules.GetSize() > 0) {
-    ModuleList removed_modules;
-    Target &target = GetTarget();
-    ModuleList &loaded_modules = m_process->GetTarget().GetImages();
-
-    for (size_t i = 0; i < loaded_modules.GetSize(); ++i) {
-      const lldb::ModuleSP loaded_module = loaded_modules.GetModuleAtIndex(i);
-
-      bool found = false;
-      for (size_t j = 0; j < new_modules.GetSize(); ++j) {
-        if (new_modules.GetModuleAtIndex(j).get() == loaded_module.get())
-          found = true;
-      }
-
-      // The main executable will never be included in libraries-svr4, don't
-      // remove it
-      if (!found &&
-          loaded_module.get() != target.GetExecutableModulePointer()) {
-        removed_modules.Append(loaded_module);
-      }
-    }
-
-    loaded_modules.Remove(removed_modules);
-    m_process->GetTarget().ModulesDidUnload(removed_modules, false);
-
-    new_modules.ForEach([&target](const lldb::ModuleSP module_sp) -> bool {
-      lldb_private::ObjectFile *obj = module_sp->GetObjectFile();
-      if (!obj)
-        return true;
-
-      if (obj->GetType() != ObjectFile::Type::eTypeExecutable)
-        return true;
-
-      lldb::ModuleSP module_copy_sp = module_sp;
-      target.SetExecutableModule(module_copy_sp, eLoadDependentsNo);
-      return false;
-    });
-
-    loaded_modules.AppendIfNeeded(new_modules);
-    m_process->GetTarget().ModulesDidLoad(new_modules);
-  }
-
-  return new_modules.GetSize();
+  return module_list.m_list.size();
 }
 
 size_t ProcessGDBRemote::LoadModules() {
   LoadedModuleInfoList module_list;
   return LoadModules(module_list);
 }
 
 Status ProcessGDBRemote::GetFileLoadAddress(const FileSpec &file,