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.

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

ProcessGDBRemote::LoadModuleAtAddress (const FileSpec &file, lldb::addr_t base_addr, bool value_is_offset)
     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);
     }
+    else
+    {
+        if (value_is_offset)
+        {
+            // Try to fetch the load address of the file from the process as we need absolute load
+            // address to read the file out of the memory instead of a load bias.
+            bool is_loaded;
+            lldb::addr_t load_addr;
+            Error error = GetFileLoadAddress (file, is_loaded, load_addr);
+            if (error.Success() && is_loaded)
+                base_addr = load_addr;
+        }
+
+        if ((module_sp = ReadModuleFromMemory (file, base_addr)))
+        {
+            module_sp->SetLoadAddress (target, base_addr, false, changed);
+            target.GetImages().AppendIfNeeded (module_sp);
+        }
+    }
 
     return module_sp;
 }
 
 size_t
 ProcessGDBRemote::LoadModules (LoadedModuleInfoList &module_list)
 {
     using lldb_private::process_gdb_remote::ProcessGDBRemote;