Page MenuHomePhabricator

Properly unload modules from target image list when using svr4 packets
ClosedPublic

Authored by fjricci on Apr 18 2016, 10:51 AM.

Details

Summary

When we receive an svr4 packet from the remote, we check for new modules
and add them to the list of images in the target. However, we did not
do the same for modules which have been removed.

This was causing TestLoadUnload to fail when using ds2, which uses
svr4 packets to communicate all library info on Linux. This patch fixes
the failing test.

Diff Detail

Repository
rL LLVM

Event Timeline

fjricci updated this revision to Diff 54083.Apr 18 2016, 10:51 AM
fjricci retitled this revision from to Properly unload modules from target image list when using svr4 packets.
fjricci updated this object.
fjricci added reviewers: ADodds, zturner, tfiala.
fjricci added subscribers: sas, lldb-commits.
tfiala added inline comments.Apr 18 2016, 1:47 PM
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
4907 ↗(On Diff #54083)

It looks like this code will unload any modules currently listed as loaded via m_process->GetTarget().GetImages(), if they do not appear in the module_list argument to this function. Is that correct behavior? (It might be, but that's not what I would have guessed without digging deeper).

I might be not following the flow here correctly, though. Can you clarify? Did the full test suite run with this change? Thanks!

fjricci added inline comments.Apr 18 2016, 4:25 PM
source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
4907 ↗(On Diff #54083)

So yes, this will remove any existing modules that are not in the svr4 packet, provided that there were modules listed in the svr4 packet (indicating that the remote is using the packet) - if new_modules.GetSize() == 0, we won't remove anything.

As far as I can tell from the gdb protocol specs, the svr4 packet should contain a list of all loaded libraries, so as long as the svr4 packet contains libraries, I believe that removing modules which are no longer listed is the correct behavior.

I did run the full suite on linux (with lldb-server), and it passes with this change.

As a side note, the module_list argument is actually an output parameter, filled by GetLoadedModuleList().

tfiala accepted this revision.Apr 18 2016, 8:14 PM
tfiala edited edge metadata.

LGTM.

source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
4907 ↗(On Diff #54083)

Oh of course I see the flow.

This revision is now accepted and ready to land.Apr 18 2016, 8:14 PM
ADodds accepted this revision.Apr 19 2016, 3:44 AM
ADodds edited edge metadata.

Looks fine to me.

This revision was automatically updated to reflect the committed changes.