labath wrote: 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. https://github.com/llvm/llvm-project/pull/134383 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
