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

1 files changed, 46 insertions, 33 deletions

diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index ce2073a91f4..5cb5f147367 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -2390,7 +2390,12 @@ StateType ProcessGDBRemote::SetThreadStopInfo(StringExtractor &stop_packet) {
         ostr.Printf("%" PRIu64 " %" PRIu32, wp_addr, wp_index);
         description = ostr.GetString();
       } else if (key.compare("library") == 0) {
-        LoadModules();
+        auto error = LoadModules();
+        if (error) {
+          Log *log(
+              ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
+          LLDB_LOG_ERROR(log, std::move(error), "Failed to load modules: {0}");
+        }
       } else if (key.size() == 2 && ::isxdigit(key[0]) && ::isxdigit(key[1])) {
         uint32_t reg = UINT32_MAX;
         if (!key.getAsInteger(16, reg))
@@ -2742,9 +2747,13 @@ addr_t ProcessGDBRemote::GetImageInfoAddress() {
 
   // the loaded module list can also provides a link map address
   if (addr == LLDB_INVALID_ADDRESS) {
-    LoadedModuleInfoList list;
-    if (GetLoadedModuleList(list).Success())
-      addr = list.m_link_map;
+    llvm::Expected<LoadedModuleInfoList> list = GetLoadedModuleList();
+    if (!list) {
+      Log *log(ProcessGDBRemoteLog::GetLogIfAllCategoriesSet(GDBR_LOG_PROCESS));
+      LLDB_LOG_ERROR(log, list.takeError(), "Failed to read module list: {0}");
+    } else {
+      addr = list->m_link_map;
+    }
   }
 
   return addr;
@@ -4682,39 +4691,43 @@ bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
   return m_register_info.GetNumRegisters() > 0;
 }
 
-Status ProcessGDBRemote::GetLoadedModuleList(LoadedModuleInfoList &list) {
+llvm::Expected<LoadedModuleInfoList> ProcessGDBRemote::GetLoadedModuleList() {
   // Make sure LLDB has an XML parser it can use first
   if (!XMLDocument::XMLEnabled())
-    return Status(0, ErrorType::eErrorTypeGeneric);
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "XML parsing not available");
 
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_PROCESS);
   LLDB_LOGF(log, "ProcessGDBRemote::%s", __FUNCTION__);
 
+  LoadedModuleInfoList list;
   GDBRemoteCommunicationClient &comm = m_gdb_comm;
   bool can_use_svr4 = GetGlobalPluginProperties()->GetUseSVR4();
 
   // check that we have extended feature read support
   if (can_use_svr4 && comm.GetQXferLibrariesSVR4ReadSupported()) {
-    list.clear();
-
     // request the loaded library list
     std::string raw;
     lldb_private::Status lldberr;
 
     if (!comm.ReadExtFeature(ConstString("libraries-svr4"), ConstString(""),
                              raw, lldberr))
-      return Status(0, ErrorType::eErrorTypeGeneric);
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Error in libraries-svr4 packet");
 
     // parse the xml file in memory
     LLDB_LOGF(log, "parsing: %s", raw.c_str());
     XMLDocument doc;
 
     if (!doc.ParseMemory(raw.c_str(), raw.size(), "noname.xml"))
-      return Status(0, ErrorType::eErrorTypeGeneric);
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Error reading noname.xml");
 
     XMLNode root_element = doc.GetRootElement("library-list-svr4");
     if (!root_element)
-      return Status();
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Error finding library-list-svr4 xml element");
 
     // main link map structure
     llvm::StringRef main_lm = root_element.GetAttributeValue("main-lm");
@@ -4778,28 +4791,31 @@ Status ProcessGDBRemote::GetLoadedModuleList(LoadedModuleInfoList &list) {
                        // node
         });
 
-    LLDB_LOGF(log, "found %" PRId32 " modules in total",
-              (int)list.m_list.size());
+    if (log)
+      LLDB_LOGF(log, "found %" PRId32 " modules in total",
+                (int)list.m_list.size());
+    return list;
   } else if (comm.GetQXferLibrariesReadSupported()) {
-    list.clear();
-
     // request the loaded library list
     std::string raw;
     lldb_private::Status lldberr;
 
     if (!comm.ReadExtFeature(ConstString("libraries"), ConstString(""), raw,
                              lldberr))
-      return Status(0, ErrorType::eErrorTypeGeneric);
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Error in libraries packet");
 
     LLDB_LOGF(log, "parsing: %s", raw.c_str());
     XMLDocument doc;
 
     if (!doc.ParseMemory(raw.c_str(), raw.size(), "noname.xml"))
-      return Status(0, ErrorType::eErrorTypeGeneric);
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Error reading noname.xml");
 
     XMLNode root_element = doc.GetRootElement("library-list");
     if (!root_element)
-      return Status();
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "Error finding library-list xml element");
 
     root_element.ForEachChildElementWithName(
         "library", [log, &list](const XMLNode &library) -> bool {
@@ -4836,13 +4852,14 @@ Status ProcessGDBRemote::GetLoadedModuleList(LoadedModuleInfoList &list) {
                        // node
         });
 
-    LLDB_LOGF(log, "found %" PRId32 " modules in total",
-              (int)list.m_list.size());
+    if (log)
+      LLDB_LOGF(log, "found %" PRId32 " modules in total",
+                (int)list.m_list.size());
+    return list;
   } else {
-    return Status(0, ErrorType::eErrorTypeGeneric);
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Remote libraries not supported");
   }
-
-  return Status();
 }
 
 lldb::ModuleSP ProcessGDBRemote::LoadModuleAtAddress(const FileSpec &file,
@@ -4857,17 +4874,18 @@ lldb::ModuleSP ProcessGDBRemote::LoadModuleAtAddress(const FileSpec &file,
                                      value_is_offset);
 }
 
-size_t ProcessGDBRemote::LoadModules(LoadedModuleInfoList &module_list) {
+llvm::Error ProcessGDBRemote::LoadModules() {
   using lldb_private::process_gdb_remote::ProcessGDBRemote;
 
   // request a list of loaded libraries from GDBServer
-  if (GetLoadedModuleList(module_list).Fail())
-    return 0;
+  llvm::Expected<LoadedModuleInfoList> module_list = GetLoadedModuleList();
+  if (!module_list)
+    return module_list.takeError();
 
   // get a list of all the modules
   ModuleList new_modules;
 
-  for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list.m_list) {
+  for (LoadedModuleInfoList::LoadedModuleInfo &modInfo : module_list->m_list) {
     std::string mod_name;
     lldb::addr_t mod_base;
     lldb::addr_t link_map;
@@ -4934,12 +4952,7 @@ size_t ProcessGDBRemote::LoadModules(LoadedModuleInfoList &module_list) {
     m_process->GetTarget().ModulesDidLoad(new_modules);
   }
 
-  return new_modules.GetSize();
-}
-
-size_t ProcessGDBRemote::LoadModules() {
-  LoadedModuleInfoList module_list;
-  return LoadModules(module_list);
+  return llvm::ErrorSuccess();
 }
 
 Status ProcessGDBRemote::GetFileLoadAddress(const FileSpec &file,