Correctly use GetLoadedModuleList to take advantage of libraries-svr4

[aadsm]

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.

[labath]

I don't have time to do a full review of this today, but this seems pretty good at a first glance...

lldb/include/lldb/Target/Process.h

684 ↗

(On Diff #207315)

Since you're already changing the signature, let's make this return an llvm::Error.

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

184–206 ↗

(On Diff #207315)

I like that you've factored out the decoding of the states, but I think it would be better to make this a separate function returning an enum (GetAction ?). This will make it more obvious which combinations of flags are covered. Also, StateIsTakeSnapshot reads weirdly...

[aadsm]

lldb/include/lldb/Target/Process.h

684 ↗

(On Diff #207315)

Ah yeah ok. It still weirds me out that an Error object might not be an error. In this regard I actually much prefer the Status object as it much better conveys the meaning of the return object. Is the plan to move from Status to Error in lldb?

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

184–206 ↗

(On Diff #207315)

Sounds good. I didn't want to use the word consistent, I wanted to match rest of the language used in the code. Originally I had ShouldX (e.g.: ShouldTakeSnapshot) but I didn't think it was clear when to call it (or that it was derived from the state). I like the GetAction idea, thanks.

[labath]

lldb/include/lldb/Target/Process.h

684 ↗

(On Diff #207315)

Yep. lldb_private::Status used to be called lldb_private::Error, but it was renamed to avoid confusing with the llvm version.

[aadsm]

Address comments, add GetAction that returns an enum with what to do.

[aadsm]

@labath can I ask for a re-review on this one please? :)

[labath]

Looks mostly good. Just a couple of style comments.

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

185–207 ↗

(On Diff #207455)

This logic is pretty hard to follow. I'd suggest trying to structure as some switch statements. Something like:

switch(current_state) {
case Consistent:
  switch (previous_state) {
    case Consistent: return TakeSnapshot;
    case Add: return Add;
    case Remove: return Remove;
  }
case Add: case Remove:
  @#%@#%!@%
}

240 ↗

(On Diff #207455)

Use a switch here.

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

265 ↗

(On Diff #207455)

We don't usually use the enum tag here.

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

2731 ↗

(On Diff #207455)

You'll need to somehow handle the case when GetLoadedModuleList fails (maybe by logging the error).

[aadsm]

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

185–207 ↗

(On Diff #207455)

Interesting, I actually think it's easier to follow than breaking it down to nested switches. I wanted to code to reflect the mental model that decides which action to take.

That's why I structured it in the way of:

if (state A) do action 1
if (state B) do action 2
...

The switch statement basically creates a logic decision tree that I personally find harder to follow. If I want to know what states makes us take a snapshot I need to search the entire code for return eTakeSnapshot and then go up the nested switch for each one of them and collect all the cases. In the other one I can focus my attention just to the condition for "do action 1".

Here's how it looks like:

switch (m_current.state) {
    case eConsistent:
      switch (m_previous.state) {
        case eConsistent: return eTakeSnapshot;
        case eAdd: return eAddModules;
        case eDelete: return eRemoveModules;
      }
      break;
    case eAdd:
    case eDelete: 
      switch (m_previous.state) {
        case eConsistent: return eTakeSnapshot;
        case eAdd: if (m_current.state == eDelete)
          return eTakeSnapshot;
      }
  }
  
  return eNoAction;

I also wanted to make the hacks a bit more obvious so I came up with this which I think strikes a good balance in making the decision clear and isolates the hack we have for android loaders:

if (m_current.state == 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;
    }
    return eNoAction;
  }
  
  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 eNoAction;

    return eTakeSnapshot;
  }
  
  return eNoAction;

Thoughts?

[aadsm]

Address comments.
In the end I used nested switches for the GetAction function. I thought about it and I guess it boils down to personal preference. It's still harder for me to read than the if version so I put some empty lines to improve that.

[aadsm]

Missed one function

[labath]

lgtm

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

185–207 ↗

(On Diff #207455)

This would be fine too. I was mainly concerned about the huge if condition with embedded comments. I was staring at it for 10 minutes, and I still wasn't sure I understood the conditions it would fire in...

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

2383 ↗

(On Diff #210422)

btw, LLDB_LOG macros are smart enough to embed file and function information into the log message themselves (if you use "log enable -F"), so you don't need to repeat it here.

[aadsm]

Address comments, also checks the LoadModules return on DynamicLoaderWindowsDYLD

[aadsm]

Handle error returned by GetLoadedModuleList

[aadsm]

Rebase

lldb/trunk/include/lldb/Target/Process.h

/// UnregisterNotificationCallbacks (const Notifications&)
 
   /// Sometimes processes know how to retrieve and load shared libraries. This
   /// is normally done by DynamicLoader plug-ins, but sometimes the connection
   /// to the process allows retrieving this information. The dynamic loader
   /// plug-ins can use this function if they can't determine the current
   /// shared library load state.
   ///
   /// \return
-  ///    The number of shared libraries that were loaded
-  virtual size_t LoadModules() { return 0; }
+  ///    A status object indicating if the operation was sucessful or not.
+  virtual llvm::Error LoadModules() {
+    return llvm::make_error<llvm::StringError>("Not implemented.",
+                                               llvm::inconvertibleErrorCode());
+  }
 
-  virtual size_t LoadModules(LoadedModuleInfoList &) { return 0; }
+  /// Query remote GDBServer for a detailed loaded library list
+  /// \return
+  ///    The list of modules currently loaded by the process, or an error.
+  virtual llvm::Expected<LoadedModuleInfoList> GetLoadedModuleList() {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Not implemented");
+  }
 
 protected:
   virtual JITLoaderList &GetJITLoaders();
 
 public:
   /// Get the system runtime plug-in for this process.
   ///
   /// \return

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

protected:
   /// 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 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();
 
   bool RemoveSOEntries();
 
   void UpdateBaseAddrIfNecessary(SOEntry &entry, std::string const &file_path);
 
   bool SOEntryIsMainExecutable(const SOEntry &entry);
 
   /// Reads the current list of shared objects according to the link map
   /// supplied by the runtime linker.
   bool TakeSnapshot(SOEntryList &entry_list);
 
   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

lldb/trunk/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))
+  if (m_current.map_addr == 0)
+    return false;
+
+  if (UpdateSOEntriesFromRemote())
     return true;
 
   return UpdateSOEntries();
 }
 
 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)
-    return false;
-
-  if (!fromRemote && 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);
+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.
-  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.
+    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 false;
+      return eNoAction;
 
-    m_soentries.clear();
-    if (fromRemote)
-      return SaveSOEntriesFromRemote(module_list);
+    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();
-    return TakeSnapshot(m_soentries);
+    // 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;
   }
-  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();
+  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();
+    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;
+  }
+}
 
 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;
 
bool DYLDRendezvous::FillSOEntryFromModuleInfo(
   entry.next = 0;
   entry.prev = 0;
   entry.path_addr = 0;
 
   return true;
 }
 
 bool DYLDRendezvous::SaveSOEntriesFromRemote(
-    LoadedModuleInfoList &module_list) {
+    const 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) {
+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) {
       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))
+    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) {
+    const 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_removed_soentries.push_back(entry);
     }
   }
 
   m_loaded_modules = module_list;
   return true;
 }
 
 bool DYLDRendezvous::AddSOEntries() {

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

LLDB_LOGF(log, "DynamicLoaderPOSIXDYLD::%s() pid %" PRIu64, __FUNCTION__,
             m_process ? m_process->GetID() : LLDB_INVALID_PROCESS_ID);
   m_auxv = llvm::make_unique<AuxVector>(m_process->GetAuxvData());
 
   LLDB_LOGF(
       log, "DynamicLoaderPOSIXDYLD::%s pid %" PRIu64 " reloaded auxv data",
       __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);
 
   // find the main process load offset
   addr_t load_offset = ComputeLoadOffset();
   LLDB_LOGF(log,
             "DynamicLoaderPOSIXDYLD::%s pid %" PRIu64

lldb/trunk/source/Plugins/DynamicLoader/Windows-DYLD/DynamicLoaderWindowsDYLD.cpp

if (image_base == load_addr)
       return;
 
     // Rebase the process's modules if there is a mismatch.
     UpdateLoadedSections(executable, LLDB_INVALID_ADDRESS, load_addr, false);
 
     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() {
   Log *log(GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER));
   LLDB_LOGF(log, "DynamicLoaderWindowsDYLD::%s()", __FUNCTION__);
 
   ModuleSP executable = GetTargetExecutable();
   if (!executable.get())
     return;
 
   lldb::addr_t load_addr = GetLoadAddress(executable);
   if (load_addr != LLDB_INVALID_ADDRESS) {
     // Update the loaded sections so that the breakpoints can be resolved.
     UpdateLoadedSections(executable, LLDB_INVALID_ADDRESS, load_addr, false);
 
     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}");
   }
 }
 
 Status DynamicLoaderWindowsDYLD::CanLoadImage() { return Status(); }
 
 ConstString DynamicLoaderWindowsDYLD::GetPluginName() {
   return GetPluginNameStatic();
 }

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

public:
   bool GetModuleSpec(const FileSpec &module_file_spec, const ArchSpec &arch,
                      ModuleSpec &module_spec) override;
 
   void PrefetchModuleSpecs(llvm::ArrayRef<FileSpec> module_file_specs,
                            const llvm::Triple &triple) override;
 
   llvm::VersionTuple GetHostOSVersion() override;
 
-  size_t LoadModules(LoadedModuleInfoList &module_list) override;
+  llvm::Error LoadModules() override;
 
-  size_t LoadModules() override;
+  llvm::Expected<LoadedModuleInfoList> GetLoadedModuleList() 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,
protected:
   bool GetGDBServerRegisterInfoXMLAndProcess(ArchSpec &arch_to_use,
                                              std::string xml_filename, 
                                              uint32_t &cur_reg_num,
                                              uint32_t &reg_offset);
 
   // Query remote GDBServer for register information
   bool GetGDBServerRegisterInfo(ArchSpec &arch);
 
-  // Query remote GDBServer for a detailed loaded library list
-  Status GetLoadedModuleList(LoadedModuleInfoList &);
-
   lldb::ModuleSP LoadModuleAtAddress(const FileSpec &file,
                                      lldb::addr_t link_map,
                                      lldb::addr_t base_addr,
                                      bool value_is_offset);
 
   Status UpdateAutomaticSignalFiltering() override;
 
   Status FlashErase(lldb::addr_t addr, size_t size);

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

while (stop_packet.GetNameColonValue(key, value)) {
         if (wp_sp)
           wp_index = wp_sp->GetHardwareIndex();
 
         reason = "watchpoint";
         StreamString ostr;
         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))
           expedited_register_map[reg] = std::move(value);
       }
     }
 
     if (tid == LLDB_INVALID_THREAD_ID) {

 }
 
 addr_t ProcessGDBRemote::GetImageInfoAddress() {
   // request the link map address via the $qShlibInfoAddr packet
   lldb::addr_t addr = m_gdb_comm.GetShlibInfoAddr();
 
   // 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;
 }
 
 void ProcessGDBRemote::WillPublicStop() {
   // See if the GDB remote client supports the JSON threads info. If so, we
   // gather stop info for all threads, expedited registers, expedited memory,
bool ProcessGDBRemote::GetGDBServerRegisterInfo(ArchSpec &arch_to_use) {
   uint32_t cur_reg_num = 0;
   uint32_t reg_offset = 0;
   if (GetGDBServerRegisterInfoXMLAndProcess (arch_to_use, "target.xml", cur_reg_num, reg_offset))
     this->m_register_info.Finalize(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");
     if (!main_lm.empty()) {
       list.m_link_map =
           StringConvert::ToUInt64(main_lm.data(), LLDB_INVALID_ADDRESS, 0);
     }
 
root_element.ForEachChildElementWithName(
                       name.c_str());
           }
 
           list.add(module);
           return true; // Keep iterating over all "library" elements in the root
                        // node
         });
 
+    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 {
           LoadedModuleInfoList::LoadedModuleInfo module;
 
           llvm::StringRef name = library.GetAttributeValue("name");
           module.set_name(name.str());
 
root_element.ForEachChildElementWithName(
                       (base_is_offset ? "offset" : "absolute"), name.c_str());
           }
 
           list.add(module);
           return true; // Keep iterating over all "library" elements in the root
                        // node
         });
 
+    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,
                                                      lldb::addr_t link_map,
                                                      lldb::addr_t base_addr,
                                                      bool value_is_offset) {
   DynamicLoader *loader = GetDynamicLoader();
   if (!loader)
     return nullptr;
 
   return loader->LoadModuleAtAddress(file, link_map, base_addr,
                                      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;
     bool mod_base_is_offset;
 
     bool valid = true;
     valid &= modInfo.get_name(mod_name);
     valid &= modInfo.get_base(mod_base);
new_modules.ForEach([&target](const lldb::ModuleSP module_sp) -> bool {
       target.SetExecutableModule(module_copy_sp, eLoadDependentsNo);
       return false;
     });
 
     loaded_modules.AppendIfNeeded(new_modules);
     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,
                                             bool &is_loaded,
                                             lldb::addr_t &load_addr) {
   is_loaded = false;
   load_addr = LLDB_INVALID_ADDRESS;