Allow gdbremote process to read modules from memory

[fjricci]

Summary

The logic to read modules from memory was added to LoadModuleAtAddress
in the dynamic loader, but not in process gdb remote. This means that when
the remote uses svr4 packets to give library info, libraries only present
on the remote will not be loaded.

This patch therefore involves some code duplication from LoadModuleAtAddress
in the dynamic loader, but removing this would require some amount of code
refactoring.

[tberghammer]

What is your opinion about replacing the ProcessGDBRemote::LoadModuleAtAddress with the following code:

lldb::ModuleSP
ProcessGDBRemote::LoadModuleAtAddress (const FileSpec &file, lldb::addr_t base_addr, bool value_is_offset)
{
    DynamicLoader* loader = GetDynamicLoader();
    if (!loader)
        return nullptr;
    return loader->LoadModuleAtAddress(file, LLDB_INVALID_ADDRESS, base_addr, value_is_offset);
}

I think this should fix your problem while reducing the amount of code duplication we already has (I haven't tested it).

[fjricci]

That was actually one of the fixes I originally tried. My only concern is that LoadModuleAtAddress is a protected method in the dynamic loader. If you think it's fine to change it to public, I'll go ahead and do that.

[tberghammer]

I think it should be fine to make it public as it don't do anything what can mess up the private state of the DynamicLoader. In general visibility in LLDB are decided in a bit randomly so usually it is fine to change them if the implementation looks like it won't break some internal state.

[fjricci]

I see that DynamicLoaderHexagonDYLD also has a LoadModuleAtAddress function, which seems to be the same as the one in DynamicLoader, except that the member functions it calls are overloaded, and it is also missing the newly added ReadFromMemory code.

I think this function should be safe to remove, but I wanted to double check before I do that.

[fjricci]

Refactor to remove code duplication

[tfiala]

The general idea looks good. You'll want to reach out to the Hexagon dynamic loader author first on any proposed changes there, though.

Thanks!

-Todd

[tberghammer]

Looks good (I added Ted as a reviewer for hexagon).

[ted]

Thanks for adding me, Tamas.

It looks like removing the code in the Hexagon Dynamic Loader plugin will have lldb drill down into ObjectFileElf, and end up doing the same thing - set the load address of each section of the ELF file. So it should behave the same as before this patch. I don't think we'll ever need to load a module from memory, but if we do, we can now. LGTM.

[sas]

Committed as r265418.

include/lldb/Target/DynamicLoader.h

public:
     ///     LLDB_INVALID_ADDRESS is returned.
     //------------------------------------------------------------------
     virtual lldb::addr_t
     GetThreadLocalData (const lldb::ModuleSP module, const lldb::ThreadSP thread)
     {
         return LLDB_INVALID_ADDRESS;
     }
 
+    /// Locates or creates a module given by @p file and updates/loads the
+    /// resulting module at the virtual base address @p base_addr.
+    virtual lldb::ModuleSP
+    LoadModuleAtAddress(const lldb_private::FileSpec &file,
+                        lldb::addr_t link_map_addr,
+                        lldb::addr_t base_addr,
+                        bool base_addr_is_offset);
+
 protected:
     //------------------------------------------------------------------
     // Utility methods for derived classes
     //------------------------------------------------------------------
 
     /// Checks to see if the target module has changed, updates the target
     /// accordingly and returns the target executable module.
     lldb::ModuleSP
protected:
     /// @param module The module to traverse.
     virtual void
     UnloadSections(const lldb::ModuleSP module);
 
     // Utility method so base classes can share implementation of UnloadSections
     void
     UnloadSectionsCommon(const lldb::ModuleSP module);
 
-    /// Locates or creates a module given by @p file and updates/loads the
-    /// resulting module at the virtual base address @p base_addr.
-    virtual lldb::ModuleSP
-    LoadModuleAtAddress(const lldb_private::FileSpec &file,
-                        lldb::addr_t link_map_addr,
-                        lldb::addr_t base_addr,
-                        bool base_addr_is_offset);
-
     const lldb_private::SectionList *
     GetSectionListFromModule(const lldb::ModuleSP module) const;
 
     // Read an unsigned int of the given size from memory at the given addr.
     // Return -1 if the read fails, otherwise return the result as an int64_t.
     int64_t
     ReadUnsignedIntWithSizeInBytes(lldb::addr_t addr, int size_in_bytes);
 

source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.h

UpdateLoadedSections(lldb::ModuleSP module,
                          bool base_addr_is_offset) override;
 
     /// Removes the loaded sections from the target in @p module.
     ///
     /// @param module The module to traverse.
     void
     UnloadSections(const lldb::ModuleSP module) override;
 
-    /// Locates or creates a module given by @p file and updates/loads the
-    /// resulting module at the virtual base address @p base_addr.
-    lldb::ModuleSP
-    LoadModuleAtAddress(const lldb_private::FileSpec &file,
-                        lldb::addr_t link_map_addr,
-                        lldb::addr_t base_addr,
-                        bool base_addr_is_offset) override;
-
     /// Callback routine invoked when we hit the breakpoint on process entry.
     ///
     /// This routine is responsible for resolving the load addresses of all
     /// dependent modules required by the inferior and setting up the rendezvous
     /// breakpoint.
     static bool
     EntryBreakpointHit(void *baton, 
                        lldb_private::StoppointCallbackContext *context, 

source/Plugins/DynamicLoader/Hexagon-DYLD/DynamicLoaderHexagonDYLD.cpp

for (I = m_rendezvous.begin(), E = m_rendezvous.end(); I != E; ++I)
                 log->Printf("DynamicLoaderHexagonDYLD::%s failed loading module %s at 0x%" PRIx64,
                             __FUNCTION__, module_path, I->base_addr);
         }
     }
 
     m_process->GetTarget().ModulesDidLoad(module_list);
 }
 
-/// Helper for the entry breakpoint callback.  Resolves the load addresses
-/// of all dependent modules.
-ModuleSP
-DynamicLoaderHexagonDYLD::LoadModuleAtAddress(const FileSpec &file,
-                                              addr_t link_map_addr,
-                                              addr_t base_addr,
-                                              bool base_addr_is_offset)
-{
-    Target &target = m_process->GetTarget();
-    ModuleList &modules = target.GetImages();
-    ModuleSP module_sp;
-
-    ModuleSpec module_spec (file, target.GetArchitecture());
-
-    // check if module is currently loaded
-    if ((module_sp = modules.FindFirstModule (module_spec))) 
-    {
-        UpdateLoadedSections(module_sp, link_map_addr, base_addr, true);
-    }
-    // try to load this module from disk
-    else if ((module_sp = target.GetSharedModule(module_spec))) 
-    {
-        UpdateLoadedSections(module_sp, link_map_addr, base_addr, true);
-    }
-
-    return module_sp;
-}
-
 /// Computes a value for m_load_offset returning the computed address on
 /// success and LLDB_INVALID_ADDRESS on failure.
 addr_t
 DynamicLoaderHexagonDYLD::ComputeLoadOffset()
 {
     // Here we could send a GDB packet to know the load offset
     //
     // send:    $qOffsets#4b

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

protected:
     bool
     GetGDBServerRegisterInfo (ArchSpec &arch);
 
     // Query remote GDBServer for a detailed loaded library list
     Error
     GetLoadedModuleList (LoadedModuleInfoList &);
 
     lldb::ModuleSP
-    LoadModuleAtAddress (const FileSpec &file, lldb::addr_t base_addr, bool value_is_offset);
+    LoadModuleAtAddress (const FileSpec &file, lldb::addr_t link_map, lldb::addr_t base_addr,
+                         bool value_is_offset);
 
 private:
     //------------------------------------------------------------------
     // For ProcessGDBRemote only
     //------------------------------------------------------------------
     static bool
     NewThreadNotifyBreakpointHit (void *baton,
                          StoppointCallbackContext *context,

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

ProcessGDBRemote::GetLoadedModuleList (LoadedModuleInfoList & list)
     } else {
         return Error (0, ErrorType::eErrorTypeGeneric);
     }
 
     return Error();
 }
 
 lldb::ModuleSP
-ProcessGDBRemote::LoadModuleAtAddress (const FileSpec &file, lldb::addr_t base_addr, bool value_is_offset)
+ProcessGDBRemote::LoadModuleAtAddress (const FileSpec &file, lldb::addr_t link_map,
+                                       lldb::addr_t base_addr, bool value_is_offset)
 {
-    Target &target = m_process->GetTarget();
-    ModuleList &modules = target.GetImages();
-    ModuleSP module_sp;
+    DynamicLoader *loader = GetDynamicLoader();
+    if (!loader)
+        return nullptr;
 
-    bool changed = false;
-
-    ModuleSpec module_spec (file, target.GetArchitecture());
-    if ((module_sp = modules.FindFirstModule (module_spec)))
-    {
-        module_sp->SetLoadAddress (target, base_addr, value_is_offset, changed);
-    }
-    else if ((module_sp = target.GetSharedModule (module_spec)))
-    {
-        module_sp->SetLoadAddress (target, base_addr, value_is_offset, changed);
-    }
-
-    return module_sp;
+    return loader->LoadModuleAtAddress(file, link_map, base_addr, value_is_offset);
 }
 
 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;
+
         // hack (cleaner way to get file name only?) (win/unix compat?)
         size_t marker = mod_name.rfind ('/');
         if (marker == std::string::npos)
             marker = 0;
         else
             marker += 1;
 
         FileSpec file (mod_name.c_str()+marker, true);
-        lldb::ModuleSP module_sp = LoadModuleAtAddress (file, mod_base, mod_base_is_offset);
+        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)
     {
         Target &target = GetTarget();