Page MenuHomePhabricator

Allow gdbremote process to read modules from memory
ClosedPublic

Authored by fjricci on Mar 28 2016, 4:47 PM.

Details

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.

Diff Detail

Event Timeline

fjricci updated this revision to Diff 51854.Mar 28 2016, 4:47 PM
fjricci retitled this revision from to Allow gdbremote process to read modules from memory.
fjricci updated this object.
fjricci added reviewers: tberghammer, ADodds, tfiala.
fjricci added subscribers: sas, lldb-commits.
tberghammer edited edge metadata.Mar 29 2016, 6:47 AM

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).

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.

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.

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 updated this revision to Diff 51962.Mar 29 2016, 11:45 AM
fjricci edited edge metadata.

Refactor to remove code duplication

tfiala edited edge metadata.Mar 29 2016, 5:19 PM
tfiala added a subscriber: 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

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

ted edited edge metadata.EditedMar 30 2016, 11:20 AM

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.

ted accepted this revision.Mar 30 2016, 11:21 AM
ted edited edge metadata.
This revision is now accepted and ready to land.Mar 30 2016, 11:21 AM
sas closed this revision.Apr 5 2016, 10:31 AM

Committed as r265418.