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

[teemperor]

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

[labath]

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

[teemperor]

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]

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.

[teemperor]

In D83876#2153709, @jingham wrote:

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]

• Moved to ClearCachedModules

[JDevlieghere]

lldb/bindings/interface/SBModule.i

365 ↗

(On Diff #278407)

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

[teemperor]

lldb/bindings/interface/SBModule.i

365 ↗

(On Diff #278407)

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]

lldb/bindings/interface/SBModule.i

365 ↗

(On Diff #278407)

How about GarbageCollectAllocatedModules?

[teemperor]

lldb/bindings/interface/SBModule.i

365 ↗

(On Diff #278407)

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

[teemperor]

• Rename to GarbageCollectAllocatedModules

lldb/packages/Python/lldbsuite/test/lldbtest.py

def tearDown(self):
                 for dict in reversed(self.dicts):
                     self.cleanup(dictionary=dict)
 
         # This must be the last statement, otherwise teardown hooks or other
         # lines might depend on this still being active.
         lldb.SBDebugger.Destroy(self.dbg)
         del self.dbg
 
+        # Modules are not orphaned during reproducer replay because they're
+        # leaked on purpose.
+        if not configuration.is_reproducer():
+            # Assert that the global module cache is empty.
+            self.assertEqual(lldb.SBModule.GetNumberAllocatedModules(), 0)
+
+
     # =========================================================
     # Various callbacks to allow introspection of test progress
     # =========================================================
 
     def markError(self):
         """Callback invoked when an error (unexpected exception) errored."""
         self.__errored__ = True
         with recording(self, False) as sbuf:
def tearDown(self):
                 targets.append(target)
                 process = target.GetProcess()
                 if process:
                     rc = self.invoke(process, "Kill")
                     assert rc.Success()
         for target in targets:
             self.dbg.DeleteTarget(target)
 
-        # Modules are not orphaned during reproducer replay because they're
-        # leaked on purpose.
         if not configuration.is_reproducer():
             # Assert that all targets are deleted.
-            assert self.dbg.GetNumTargets() == 0
-            # Assert that the global module cache is empty.
-            assert lldb.SBModule.GetNumberAllocatedModules() == 0
+            self.assertEqual(self.dbg.GetNumTargets(), 0)
 
         # Do this last, to make sure it's in reverse order from how we setup.
         Base.tearDown(self)
 
     def switch_to_thread_with_stop_reason(self, stop_reason):
         """
         Run the 'thread list' command, and select the thread with stop reason as
         'stop_reason'.  If no such thread exists, no select action is done.

lldb/source/Core/Debugger.cpp

void Debugger::Destroy(DebuggerSP &debugger_sp) {
   debugger_sp->Clear();
 
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
     std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr);
     DebuggerList::iterator pos, end = g_debugger_list_ptr->end();
     for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) {
       if ((*pos).get() == debugger_sp.get()) {
         g_debugger_list_ptr->erase(pos);
-        return;
+        break;
       }
     }
   }
+
+  // Clean any modules that are orphaned.
+  bool mandatory = true;
+  ModuleList::RemoveOrphanSharedModules(mandatory);
 }
 
 DebuggerSP Debugger::FindDebuggerWithInstanceName(ConstString instance_name) {
   DebuggerSP debugger_sp;
   if (g_debugger_list_ptr && g_debugger_list_mutex_ptr) {
     std::lock_guard<std::recursive_mutex> guard(*g_debugger_list_mutex_ptr);
     DebuggerList::iterator pos, end = g_debugger_list_ptr->end();
     for (pos = g_debugger_list_ptr->begin(); pos != end; ++pos) {