Page MenuHomePhabricator

[lldb] Add SBModule::GarbageCollectAllocatedModules and clear modules after each test run
ClosedPublic

Authored by teemperor on Jul 15 2020, 8:00 AM.

Details

Summary

Right now the only places in the SB API where lldb:: ModuleSP instances are destroyed are in SBDebugger::MemoryPressureDetected
(where it's just attempted but not guaranteed) and in SBDebugger::DeleteTarget (which will be removed in D83933).
Tests that directly create an lldb::ModuleSP and never create a target therefore currently leak lldb::Module instances. This triggers the sanity checks in lldbtest that make sure
that the global module list is empty after a test.

This patch adds SBModule::GarbageCollectAllocatedModules as an explicit way to clean orphaned lldb::ModuleSP instances. Also we now
start calling this method at the end of each test run and move the sanity check behind that call to make this work. This way
even tests that don't create targets can pass the sanity check.

This fixes TestUnicodeSymbols.py when D83865 is applied (which makes that the sanity checks actually fail the test).

Diff Detail

Event Timeline

teemperor created this revision.Jul 15 2020, 8:00 AM
teemperor edited the summary of this revision. (Show Details)

I have no opinion on this. Jim may have one...

teemperor planned changes to this revision.Jul 15 2020, 8:28 AM

I'm actually thinking that this maybe should go into the SBDebugger::Destroy functions. This way we also get all lldb::ModuleSP instances that are freed by the Debugger destructor.

jingham requested changes to this revision.Jul 15 2020, 10:10 AM

We certainly don't want to clear the shared module cache when the Debugger or the SBDebugger is destroyed. Most IDE's that use LLDB as a library use a debugger per debugging session, and destroy the debugger when the debugging session ends. As it stands now, when the first debugging session ends we don't remove any of the already parsed modules, so that the second debugging session is much faster. That would not be true if you did RemoveOrphanedSharedModules in Debugger::Destroy. 99% of all the libraries that get loaded by an app don't change over an Xcode run, and are in fact shared among most of the targets you might debug on a given platform. So clearing the cache after every run would result in a lot of pointless reparsing of system libraries.

The choice to clear the shared module cache should always be left up to the client.

We certainly don't want to clear the shared module cache when the Debugger or the SBDebugger is destroyed. Most IDE's that use LLDB as a library use a debugger per debugging session, and destroy the debugger when the debugging session ends. As it stands now, when the first debugging session ends we don't remove any of the already parsed modules, so that the second debugging session is much faster. That would not be true if you did RemoveOrphanedSharedModules in Debugger::Destroy. 99% of all the libraries that get loaded by an app don't change over an Xcode run, and are in fact shared among most of the targets you might debug on a given platform. So clearing the cache after every run would result in a lot of pointless reparsing of system libraries.

The choice to clear the shared module cache should always be left up to the client.

So I guess that only leaves an explicit function for that as an option.

Also, if that's the concept then we should also delete the same code in DeleteTarget. I'll make a review for that.

teemperor updated this revision to Diff 278407.Jul 16 2020, 2:40 AM
teemperor retitled this revision from [lldb] Clean orphaned modules in Debugger::Destroy to [lldb] Add SBModule::ClearCachedModules and clear modules after each test run.
teemperor edited the summary of this revision. (Show Details)
  • Moved to ClearCachedModules
JDevlieghere added inline comments.
lldb/bindings/interface/SBModule.i
365

Maybe we should call this ClearAllocatedModules for consistency with the other APIs?

teemperor marked an inline comment as done.Jul 16 2020, 11:10 AM
teemperor added inline comments.
lldb/bindings/interface/SBModule.i
365

Not sure what's a good name. I would assume from the name ClearAllocatedModules that i would delete all allocated modules, but it actually only clears modules that are only kept around for the sake of caching. So this would not actually always work (but it should in the test suite):

assert(GetNumberAllocatedModules != 0);
ClearAllocatedModules();
assert(GetNumberAllocatedModules == 0); // boom if any of the allocated modules is still needed.

(FWIW, LLDB internally calls this "orphaned" modules but that doesn't seem very intuitive either).

"MaybeClearAllocatedModules", "TryClearAllocatedModules", "ClearOrphanedModules", etc.

JDevlieghere added inline comments.Jul 16 2020, 11:11 AM
lldb/bindings/interface/SBModule.i
365

How about GarbageCollectAllocatedModules?

teemperor marked an inline comment as done.Jul 16 2020, 11:56 AM
teemperor added inline comments.
lldb/bindings/interface/SBModule.i
365

I actually like this one, I'll update the patch!

teemperor updated this revision to Diff 285938.Aug 17 2020, 1:55 AM
teemperor retitled this revision from [lldb] Add SBModule::ClearCachedModules and clear modules after each test run to [lldb] Add SBModule::GarbageCollectAllocatedModules and clear modules after each test run.
teemperor edited the summary of this revision. (Show Details)
  • Rename to GarbageCollectAllocatedModules
This revision was not accepted when it landed; it landed in state Needs Review.Aug 17 2020, 2:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 17 2020, 2:01 AM