Page MenuHomePhabricator

Avoid calling LoadModules twice when modules change
AbandonedPublic

Authored by aadsm on May 27 2019, 3:40 PM.

Details

Summary

This is the sixth and final patch to improve module loading in a series that started here (where I explain the motivation and solution): D62499

When xfer:libraries-svr4 is available the Process class has a LoadModules function that requests this packet and then loads or unloads modules based on the current list of loaded modules by the process.
This is function is used by DYLDRendezvous to get the list of loaded modules before and after the module is loaded. However, this is really not needed since the LoadModules function already loaded or unloaded the modules accordingly. I changed this strategy to call LoadModules only once (after the process has loaded the module).
I also removed the FromRemote functions & friends because they were only maintaining the m_soentries structure that is never being used in this context. (I hope I'm not missing anything obvious here).

Event Timeline

aadsm created this revision.May 27 2019, 3:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 27 2019, 3:40 PM
aadsm updated this revision to Diff 203761.Jun 9 2019, 6:38 PM

Do not try to use LoadModules again if they failed in the past

ping @labath @clayborg any issues with this one?

Sorry, I missed this patch. Overall, this seems fine to me. I have only two comments about it:

  • I'm not sure the CanLoadModules functions is really needed. It seems like we could just call LoadModules directly and check for error result. The XMLEnabled() && ... check could be done at the start of the LoadModules function. It looks like it already is there ProcessGDBRemote::GetLoadedModuleList, but it looks like that function accidentally returns a success error code instead of failure.
  • Looking at the implementation of ProcessGDBRemote::LoadModules, I get the feeling it will return 0 if you call it when a module gets unloaded (although it will correctly unload the module, and correctly *not* load any new module). This is not a bug in this patch, so I'd be fine with checking it in without fixing that, but this sounds like something you would want to fix, as it will cause you to stop using the gdb-remote packet as soon as the first module is unloaded.

It looks like a single solution fixing both of these issues would be to change the LoadModules function to return Expected<size_t>. That way it could report the error states explicitly (such as when the svr packet is not supported or it just returns bogus data), and zero when it correctly did not load anything. (Or, given that the number of loaded modules is not particularly useful, and it does even not capture the effect of the function very precisely, the result could just be an Error).

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

use a real type here.

So it does fix the massive regression of D62503 similarly as my D63868 but it still has 2 regressions there (=Fedora 30 x86_64, after reapplication of D62503), I haven't yet investigated why:

lldb-Suite :: functionalities/load_unload/TestLoadUnload.py
lldb-Suite :: functionalities/load_using_paths/TestLoadUsingPaths.py
aadsm added a comment.Thu, Jun 27, 8:58 AM

@jankratochvil I imagine the first test to be failing because this patch doesn't handle correctly the unload (or rather as soon as it unloads one shared object it stops working as expected).
I'm going to look at this today (right now it's 9am for me).

I looked into this a bit today and at one point in time (when the list of modules is being processed) it seems that some previously loaded modules are somehow removed (at least I stop seeing them in the image list) including the one where the rendezvous breakpoint is so that breakpoint becomes "unresolved" and that's why it doesn't stop anymore. I'll need to check why this is the case tomorrow.

aadsm abandoned this revision.Mon, Jul 1, 8:06 AM

Replaced by D62503