[lldb] Don't delete orphaned shared modules in SBDebugger::DeleteTarget

[teemperor]

Summary

In D83876 the consensus seems that LLDB should never deleted orphaned modules implicitly. However, SBDebugger::DeleteTarget
is currently doing exactly that. This code was added in 753406221b55b95141c8c1239660dc4db4e35ea5 but I don't see any explanation in the commit,
so I think we should delete it.

[JDevlieghere]

In D83876#2153709, @jingham wrote:

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

I would argue that when the client explicitly deletes a target, it makes sense that its corresponding modules are removed as well, but only if there was a way to only remove the modules that were associate exclusively with that target. I don't think it's worth adding that bookkeeping, so this looks fine to me.

[JDevlieghere]

+ Greg to see if he remembers why he added that.

[clayborg]

We originally added this just to keep modules from existing forever in LLDB as you debugged over and over. If we didn't do this, and you did 10 edit/compile/debug cycles, you would end up with 10 copies of any shared libraries that have changed between builds. So the intention was to clear out these older versions of the changes shared libraries as you build and debug again. I can see the unintended consequence of this might be that all shared libraries get unloaded when a target is deleted.

So if we remove this, it might be a good idea to modify the global module cache to detect modified files and evict them from the global module cache as new versions get loaded. Removing it from the global module cache is fine as if there are still targets around that are using that old module, the target has a strong reference to the old module and it will live on until that targets goes away.

[clayborg]

So if we remove this I would suggest that when the global module cache is asked for a file and the modification time of the files doesn't match what is in the cache currently, we create a new module entry in the global file cache for this newer version of the file and evict the old one. This will keep us from keeping old versions of a library loaded after targets are deleted.

[clayborg]

And BTW: we do have a way to remove orphaned modules via the SB API:

void SBDebugger::MemoryPressureDetected() {
  LLDB_RECORD_STATIC_METHOD_NO_ARGS(void, SBDebugger, MemoryPressureDetected);

  // Since this function can be call asynchronously, we allow it to be non-
  // mandatory. We have seen deadlocks with this function when called so we
  // need to safeguard against this until we can determine what is causing the
  // deadlocks.

  const bool mandatory = false;

  ModuleList::RemoveOrphanSharedModules(mandatory);
}

[clayborg]

SBDebugger::MemoryPressureDetected() may or may not work since "mandatory" is set to false, but we do have an API for it. This was used by Xcode a long time ago, not sure if it still is.

[jingham]

Note, you want to be careful how you reap executables whose binary has changed. If you are debugging with the debug info in .o files, and you change one .cpp file & rebuild, you can throw away the Module for the executable, but you don't want to throw away the modules that back the .o files that haven't changed.

[jingham]

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

(lldb) file foo
(lldb) run
...exited
<go to another window, change some file & rebuild>
(lldb) run

Then you do that a whole bunch of times. You haven't ever deleted the Target in this scenario.

[clayborg]

In D83933#2156468, @jingham wrote:

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

(lldb) file foo
(lldb) run
...exited
<go to another window, change some file & rebuild>
(lldb) run

Then you do that a whole bunch of times. You haven't ever deleted the Target in this scenario.

You are correct. That isn't what this patch is trying to do. The global file cache might already be doing what I suggested (ejecting older versions from the global cache), so there may be nothing to do.

But I do think deleting a target is a nice time to do module some cleanup if possible, though it might be better controlled with a bool argument to delete target instead of always being done.

If you are shared library linking against LLDB.framework and you rarely use LLDB, but you use it for symbolication every once and a while, it might be nice to be able to regain memory by deleting the target with an argument instead of now having to call SBDebugger::MemoryPressureDetected() and hope it cleans things up.

[jingham]

In D83933#2156695, @clayborg wrote:

In D83933#2156468, @jingham wrote:

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

(lldb) file foo
(lldb) run
...exited
<go to another window, change some file & rebuild>
(lldb) run

Then you do that a whole bunch of times. You haven't ever deleted the Target in this scenario.

You are correct. That isn't what this patch is trying to do. The global file cache might already be doing what I suggested (ejecting older versions from the global cache), so there may be nothing to do.

But I do think deleting a target is a nice time to do module some cleanup if possible, though it might be better controlled with a bool argument to delete target instead of always being done.

If you are shared library linking against LLDB.framework and you rarely use LLDB, but you use it for symbolication every once and a while, it might be nice to be able to regain memory by deleting the target with an argument instead of now having to call SBDebugger::MemoryPressureDetected() and hope it cleans things up.

I am not sure I agree with this. It is fine to delete modules that are really unreachable (they have been replaced on the file system and no process we know about is using them...) But short of that, I think clearing modules is something that should be done as an explicit user gesture. If in the lldb command line driver we want to add some clean up policy, that's worth discussing. But at the library level, I don't think we should be doing that.

[clayborg]

In D83933#2156740, @jingham wrote:

In D83933#2156695, @clayborg wrote:

In D83933#2156468, @jingham wrote:

And if you really are trying to keep out-of-date modules from building up, DeleteTarget is not the correct time to do this. After all, this most commonly happens in command-line lldb when people do:

(lldb) file foo
(lldb) run
...exited
<go to another window, change some file & rebuild>
(lldb) run

Then you do that a whole bunch of times. You haven't ever deleted the Target in this scenario.

You are correct. That isn't what this patch is trying to do. The global file cache might already be doing what I suggested (ejecting older versions from the global cache), so there may be nothing to do.

But I do think deleting a target is a nice time to do module some cleanup if possible, though it might be better controlled with a bool argument to delete target instead of always being done.

If you are shared library linking against LLDB.framework and you rarely use LLDB, but you use it for symbolication every once and a while, it might be nice to be able to regain memory by deleting the target with an argument instead of now having to call SBDebugger::MemoryPressureDetected() and hope it cleans things up.

I am not sure I agree with this. It is fine to delete modules that are really unreachable (they have been replaced on the file system and no process we know about is using them...) But short of that, I think clearing modules is something that should be done as an explicit user gesture. If in the lldb command line driver we want to add some clean up policy, that's worth discussing. But at the library level, I don't think we should be doing that.

I don't have any real objections to this change, we will find out in the future if there is a memory regression if this was doing something useful. A static function on SBModule might be nice to add at some point if we need it.

[clayborg]

Who is calling delete target anyway? Can't be Xcode, because we have huge performance issues right now with Xcode 11.4.1 and later due to the "parse all line tables (and link them all if DWARF in .o files is being used) everywhere on the first source file and line breakpoint being set" and we don't see slowdowns in subsequent runs with Xcode (lldb-rpc-server).

[jingham]

In D83933#2156772, @clayborg wrote:

Who is calling delete target anyway? Can't be Xcode, because we have huge performance issues right now with Xcode 11.4.1 and later due to the "parse all line tables (and link them all if DWARF in .o files is being used) everywhere on the first source file and line breakpoint being set" and we don't see slowdowns in subsequent runs with Xcode (lldb-rpc-server).

Xcode doesn't call SBDebugger::DeleteTarget. You would do that if you were planning to reuse the SBDebugger, but Xcode always makes a new SBDebugger for each debug session, and uses it for just one process run, when they are done with the debugger they just call SBDebugger::Destroy.

[jingham]

BTW, note that target delete has a -c option which does:

Perform extra cleanup to minimize memory consumption after deleting the target. By default, LLDB will keep in memory any modules previously loaded by the target as well as all of its debug info. Specifying --clean will unload all of these

shared modules and cause them to be reparsed again the next time the target is run

So we already provide a way to do this cleanup in the command line driver.

[teemperor]

D83876 adds the requested SB API that allows clearing the cached modules, so I'll land this.

lldb/source/API/SBDebugger.cpp

bool SBDebugger::DeleteTarget(lldb::SBTarget &target) {
   bool result = false;
   if (m_opaque_sp) {
     TargetSP target_sp(target.GetSP());
     if (target_sp) {
       // No need to lock, the target list is thread safe
       result = m_opaque_sp->GetTargetList().DeleteTarget(target_sp);
       target_sp->Destroy();
       target.Clear();
-      const bool mandatory = true;
-      ModuleList::RemoveOrphanSharedModules(mandatory);
     }
   }
 
   Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API));
   LLDB_LOGF(log, "SBDebugger(%p)::DeleteTarget (SBTarget(%p)) => %i",
             static_cast<void *>(m_opaque_sp.get()),
             static_cast<void *>(target.m_opaque_sp.get()), result);