[LLDB][NFC] Remove Debugger dependency in SystemLifetimeManager

[slydiman]

It reduces the memory usage in lldb-server.
Later I will try to remove the rest Debugger dependencies to reduce lldb-server size.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Dmitry Vasilyev (slydiman)

Changes

It reduces the memory usage in lldb-server.
Later I will try to remove the rest Debugger dependencies to reduce lldb-server size.

---

Full diff: https://github.com/llvm/llvm-project/pull/134383.diff

5 Files Affected:

• (modified) lldb/include/lldb/Initialization/SystemLifetimeManager.h (+4-1)
• (added) lldb/include/lldb/Initialization/SystemLifetimeManagerDbg.h (+36)
• (modified) lldb/source/API/SBDebugger.cpp (+2-2)
• (modified) lldb/source/Initialization/SystemLifetimeManager.cpp (+2-3)
• (modified) lldb/tools/lldb-test/lldb-test.cpp (+2-2)

diff --git a/lldb/include/lldb/Initialization/SystemLifetimeManager.h b/lldb/include/lldb/Initialization/SystemLifetimeManager.h
index 06328e60133fe..55138b33be712 100644
--- a/lldb/include/lldb/Initialization/SystemLifetimeManager.h
+++ b/lldb/include/lldb/Initialization/SystemLifetimeManager.h
@@ -21,7 +21,7 @@ namespace lldb_private {
 class SystemLifetimeManager {
 public:
   SystemLifetimeManager();
-  ~SystemLifetimeManager();
+  virtual ~SystemLifetimeManager();
 
   llvm::Error Initialize(std::unique_ptr<SystemInitializer> initializer,
                          LoadPluginCallbackType plugin_callback);
@@ -32,6 +32,9 @@ class SystemLifetimeManager {
   std::unique_ptr<SystemInitializer> m_initializer;
   bool m_initialized = false;
 
+  virtual void InitializeDebugger(LoadPluginCallbackType plugin_callback) {};
+  virtual void TerminateDebugger() {};
+
   // Noncopyable.
   SystemLifetimeManager(const SystemLifetimeManager &other) = delete;
   SystemLifetimeManager &operator=(const SystemLifetimeManager &other) = delete;
diff --git a/lldb/include/lldb/Initialization/SystemLifetimeManagerDbg.h b/lldb/include/lldb/Initialization/SystemLifetimeManagerDbg.h
new file mode 100644
index 0000000000000..81e0b9a382b70
--- /dev/null
+++ b/lldb/include/lldb/Initialization/SystemLifetimeManagerDbg.h
@@ -0,0 +1,36 @@
+//===-- SystemLifetimeManagerDbg.h ------------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLDB_INITIALIZATION_SYSTEMLIFETIMEMANAGERDBG_H
+#define LLDB_INITIALIZATION_SYSTEMLIFETIMEMANAGERDBG_H
+
+#include "lldb/Initialization/SystemLifetimeManager.h"
+#include "lldb/Core/Debugger.h"
+
+namespace lldb_private {
+
+class SystemLifetimeManagerDbg : SystemLifetimeManager {
+public:
+  SystemLifetimeManagerDbg() : SystemLifetimeManager() {};
+
+private:
+  virtual void InitializeDebugger(LoadPluginCallbackType plugin_callback) override {
+    Debugger::Initialize(plugin_callback);
+  };
+
+  virtual void TerminateDebugger() override {
+    Debugger::Terminate();
+  };
+
+  // Noncopyable.
+  SystemLifetimeManagerDbg(const SystemLifetimeManagerDbg &other) = delete;
+  SystemLifetimeManagerDbg &operator=(const SystemLifetimeManagerDbg &other) = delete;
+};
+}
+
+#endif
diff --git a/lldb/source/API/SBDebugger.cpp b/lldb/source/API/SBDebugger.cpp
index e646b09e05852..55ccfd415ca47 100644
--- a/lldb/source/API/SBDebugger.cpp
+++ b/lldb/source/API/SBDebugger.cpp
@@ -44,7 +44,7 @@
 #include "lldb/Host/Config.h"
 #include "lldb/Host/StreamFile.h"
 #include "lldb/Host/XML.h"
-#include "lldb/Initialization/SystemLifetimeManager.h"
+#include "lldb/Initialization/SystemLifetimeManagerDbg.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/OptionArgParser.h"
 #include "lldb/Interpreter/OptionGroupPlatform.h"
@@ -66,7 +66,7 @@
 using namespace lldb;
 using namespace lldb_private;
 
-static llvm::ManagedStatic<SystemLifetimeManager> g_debugger_lifetime;
+static llvm::ManagedStatic<SystemLifetimeManagerDbg> g_debugger_lifetime;
 
 SBError SBInputReader::Initialize(
     lldb::SBDebugger &sb_debugger,
diff --git a/lldb/source/Initialization/SystemLifetimeManager.cpp b/lldb/source/Initialization/SystemLifetimeManager.cpp
index f9de41a675356..b07fe71affec7 100644
--- a/lldb/source/Initialization/SystemLifetimeManager.cpp
+++ b/lldb/source/Initialization/SystemLifetimeManager.cpp
@@ -8,7 +8,6 @@
 
 #include "lldb/Initialization/SystemLifetimeManager.h"
 
-#include "lldb/Core/Debugger.h"
 #include "lldb/Initialization/SystemInitializer.h"
 
 #include <utility>
@@ -36,7 +35,7 @@ llvm::Error SystemLifetimeManager::Initialize(
     if (auto e = m_initializer->Initialize())
       return e;
 
-    Debugger::Initialize(plugin_callback);
+    InitializeDebugger(plugin_callback);
   }
 
   return llvm::Error::success();
@@ -46,7 +45,7 @@ void SystemLifetimeManager::Terminate() {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
 
   if (m_initialized) {
-    Debugger::Terminate();
+    TerminateDebugger();
     m_initializer->Terminate();
 
     m_initializer.reset();
diff --git a/lldb/tools/lldb-test/lldb-test.cpp b/lldb/tools/lldb-test/lldb-test.cpp
index 1960240dc4151..81ac0b8898d6e 100644
--- a/lldb/tools/lldb-test/lldb-test.cpp
+++ b/lldb/tools/lldb-test/lldb-test.cpp
@@ -17,7 +17,7 @@
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Expression/IRMemoryMap.h"
-#include "lldb/Initialization/SystemLifetimeManager.h"
+#include "lldb/Initialization/SystemLifetimeManagerDbg.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
 #include "lldb/Interpreter/CommandReturnObject.h"
 #include "lldb/Symbol/CompileUnit.h"
@@ -1245,7 +1245,7 @@ int main(int argc, const char *argv[]) {
 
   cl::ParseCommandLineOptions(argc, argv, "LLDB Testing Utility\n");
 
-  SystemLifetimeManager DebuggerLifetime;
+  SystemLifetimeManagerDbg DebuggerLifetime;
   if (auto e = DebuggerLifetime.Initialize(
           std::make_unique<SystemInitializerTest>(), nullptr)) {
     WithColor::error() << "initialization failed: " << toString(std::move(e))

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[slydiman]

Note currently I see the following Debugger dependencies in lldb-server:

std::__1::__throw_bad_weak_ptr[abi:nn210000](): referenced by Section.cpp lldb_private::Section::GetLoadBaseAddress(lldb_private::Target*) const
lldb_private::Debugger::GetCurrentProgressReport() const: referenced by FormatEntity.cpp lldb_private::FormatEntity::Format(lldb_private::FormatEntity::Entry const&, lldb_private::Stream&, lldb_private::SymbolContext const*, lldb_private::ExecutionContext const*, lldb_private::Address const*, lldb_private::ValueObject*, bool, bool)
lldb_private::Debugger::GetDisassemblyFormat() const: referenced by ThreadPlanTracer.cpp lldb_private::ThreadPlanAssemblyTracer::Log()
lldb_private::Debugger::GetSelectedExecutionContext(): referenced by CommandInterpreter.cpp lldb_private::CommandInterpreter::GetExecutionContext() const
lldb_private::Debugger::ReportWarning(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::optional<unsigned long>, std::__1::once_flag*): referenced by Module.cpp lldb_private::Module::ReportWarningUnsupportedLanguage(lldb::LanguageType, std::__1::optional<unsigned long>)

[labath]

I think this is too mechanical and has too many layers of abstraction. SystemInitializer was supposed to be the customizable part of the initialization machinery. Please try to find a way to move this call there.

[JDevlieghere]

I wanted to make this point in #132274, but it fits here as well. I understand the need to reduce the binary size of lldb-server and I'm totally supportive of it. Moving some code around to avoid pulling in some symbols makes sense as a proof-of-concept, to show the cost of an unnecessary dependency (e.g. lldb-server relies on Debugger which makes the binary 1MB bigger).

However that approach isn't sustainable. Without a way to enforce this, it is going to regress again. Additionally, LLDB developers shouldn't have to have a mental model of the transitive dependencies of what lldb-server pulls in. For example, I shouldn't have to wonder whether I can use Debugger in FormatEntity, who both live in the Core library, because something in lldb-server is using one but not the other.

A good example of how to do this is the NO_PLUGIN_DEPENDENCIES that @bulbazord added to CMake to prevent libraries from introducing new plugin dependencies. Although it would be nice, that's not to say that this needs to be fully automated. For example, there's nothing (automatically) preventing you from introducing a dependency from Utility to Host but that's still fairly easy to spot in a code review.

It seems like we should address this issue by first fixing the existing layering violations, then introducing more layering for lldb-server so that we bring the granularity to the library level, not the individual object file (i.e. source file) level which is too granular. Maybe you have a higher level plan to deal with this (and I missed it) and this is a step in that direction?

[slydiman]

It seems like we should address this issue by first fixing the existing layering violations, then introducing more layering for lldb-server so that we bring the granularity to the library level, not the individual object file (i.e. source file) level which is too granular.

I don't see much difference between using libraries or a number of object file. If the code references a function, we need to provide that function anyway, whether it's a library or a separate object file. To break dependency we need to separate the interface and the implementation. The class Debugger is used directly in mane places. We don't need the Debugger plug-in. But we need to define DebuggerInterface and use it instead.

[JDevlieghere]

I don't see much difference between using libraries or a number of object file.

It's easier to reason about libraries and library dependencies. Not counting the plugins, we have ~10 libraries and ~1000 object files. The difference is knowing what can depend on what. Expecting developers to have a mental model of 10 libraries is reasonable and documentable. Expecting to know that you can't use a very common class within the same library is not.

If the code references a function, we need to provide that function anyway, whether it's a library or a separate object file. To break dependency we need to separate the interface and the implementation. The class Debugger is used directly in mane places. We don't need the Debugger plug-in. But we need to define DebuggerInterface and use it instead.

I'm arguing that is solving the wrong problem. I assume that lldb-server doesn't need Debugger. I understand it's a lot of work to remove all the places we depend on it, but doing anything else is just working around the problem. I don't mind being pragmatic, but without a mechanism to make sure that workaround keeps its value, we're making the code more complex for some temporary reprieve.

[labath]

I agree with everything Jonas said here (and that's why I am very reluctant to approve #132274). The layering should be between libraries (like they are in the rest of llvm), not individual object files. My mental model for a "dependency" from library A to library B is "library A (it's source files) "mentions" an entity defined in library B". It doesn't matter whether this pulls in some object files/functions/etc. from the other library during linking or not. If there's no dependency, it shouldn't even be aware of its existence. You should be able to delete all files from library B, and library A should still build fine. I'm not saying I won't approve anything else, but I consider anything below this a compromise.

Now, when it comes to the "Initialization" library(*), before it "plugin load" callback was introduced, it actually satisfied this criterion. The overall idea was that the individual users (lldb-server, liblldb, etc.) put their initialization code into their SystemInitializer subclass. SystemLifetimeManager was just a RAII wrapper around that class. The plugin load patch broke that by adding the dependency directly into the SystemLifetimeManager.

This patch removes the dependency as far as the linker is concerned, but doesn't remove the dependency according to the definition above. The SystemLifetimeManager (and the Initialization library by extension) still "depends" on the the Debugger object through LoadPluginCallbackType typedef (the debugger is an argument of the callback). (The LoadPluginCallbackType type itself is also a problem since it's an liblldb-specific concept and lldb-server wants nothing to do with that).

The right way to fix this is to bypass the SystemLifetimeManager class completely, and pass the callback directly into SystemInitializerFull. The only thing which makes that difficult is that the callback is a lambda defined in SBDebugger.cpp. However, there's no reason it has to be defined there -- the lambda itself doesn't depend on anything, and it could easily be moved to SystemInitializerFull.cpp.

TL;DR: I believe you should move the lambda to SystemInitializerFull.cpp, so that it can be directly referenced from SystemInitializerFull::Initialize. I believe this should also address Jonas's concerns about library dependencies.

(*) I think having an entire library dedicated to "initialization" is overkill. AFAICT, after doing the above, this code will not depend on anything else, so I think it'd be best to just move it into the Utility library. I'm saying this is because I think the main cause of this problem is that people aren't aware the intention behind this library (just in case you're wondering, this wasn't my idea), but I think that by now, people have internalized the idea that "Utility" should have no external dependencies.

[slydiman]

and that's why I am very reluctant to approve #132274

#132274 is most important now because it gives the significant effect.
I have refactored CPlusPlusLanguage::MethodName and don't just splite a CU now. RichManglingContext.cpp uses Language::FindPlugin(eLanguageTypeC_plus_plus), etc.

This patch is almost cosmetics. It removes unnecessary Debugger from lldb-server initialization chain. This speeds up and reduces memory usage fot now, but the effect is not noticeable.

So, I will move LoadPlugin lambda to SystemInitializerFull.

Note lldbInitialization depends on lldbPluginProcessGDBRemote, lldbPluginProcessPOSIX and lldbPluginProcessWindowsCommon. SystemInitializerCommon::Initialize() calls process_gdb_remote::ProcessGDBRemoteLog::Initialize(), ProcessPOSIXLog::Initialize() and ProcessWindowsLog::Initialize().
If it is OK for Utility, I will move lldbInitialization into lldbUtility.
I would prefer to move lldbInitialization into lldbUtility with a separate patch.

[labath]

So, I will move LoadPlugin lambda to SystemInitializerFull.

Thanks, this LGTM now, but do wait for @JDevlieghere's review as well.

Note lldbInitialization depends on lldbPluginProcessGDBRemote, lldbPluginProcessPOSIX and lldbPluginProcessWindowsCommon. SystemInitializerCommon::Initialize() calls process_gdb_remote::ProcessGDBRemoteLog::Initialize(), ProcessPOSIXLog::Initialize() and ProcessWindowsLog::Initialize(). If it is OK for Utility, I will move lldbInitialization into lldbUtility.

Ah yes, I'm not sure how I missed these, but that explains why these are a separate library. These deps aren't OK for Utililty, though I am not sure they actually need to be here. I think the log classes should be initialized from their respective plugins' Initialize methods. It also calls FileSystem and HostInfo initialization functions, which also aren't OK, but maybe we could do something about these too (in particular, we already need to jump through some hoops in order to pass arguments into HostInfo::Initialize). I may have some time to do some "casual coding" next week, so I'll try to come up with something here.

I would prefer to move lldbInitialization into lldbUtility with a separate patch.

definitely

[JDevlieghere]

Thanks, this LGTM and addresses my concerns!