[Lldb-commits] [PATCH] D133534: Complete support of loading a darwin kernel over a live gdb-remote connection given the address of a mach-o fileset

[Nico Weber via Phabricator via lldb-commits]

thakis added inline comments.


================
Comment at: lldb/source/Plugins/Platform/MacOSX/CMakeLists.txt:47
     lldbUtility
+    lldbPluginDynamicLoaderDarwinKernel
+    lldbPluginObjectContainerMachOFileset
----------------
jasonmolenda wrote:
> jasonmolenda wrote:
> > thakis wrote:
> > > This causes a dependency cycle:
> > > 
> > >   //lldb/source/Plugins/Platform/MacOSX:MacOSX ->
> > >   //lldb/source/Plugins/DynamicLoader/Darwin-Kernel:Darwin-Kernel ->
> > >   //lldb/source/Plugins/Platform/MacOSX:MacOSX
> > Ach, naturally.  DynamicLoaderDarwinKernel has to create a PlatformDarwinKernel to set in the Target when it is initializing itself.  :/  Maybe I'll just add DynamicLoaderDarwinKernel to the unit tests that have PlatformMacOSX in them.
> I removed the dependency in DynamicLoaderDarwinKernel, a very specialized plugin, and left the dependency in PlatformMacOSX which includes all of the darwin platforms and is a common one to import.  I believe any target that is linking against DynamicLoaderDarwinKernel will also have a dependency on PlatformMacOSX already.  I landed this as 30578c08568bc8de79dea72e41f49899ba10ea55 to make sure this causes no problems, we can fix it better if someone has a suggestion.
Thanks!

> I believe anything linking the darwin kernel DynamicLoader plugin
> will already have lldbPluginPlatformMacOSX in its dependency list,
> so not explicitly expressing this dependency is safe.

That sounds like it doesn't really remove the dependency, it just lies about the build system about it and things still happen to work :)

Normally, if you have a cycle between libraries A and B, you'd either:

- decide which of the two should depend on the other
- make the "lower" library expose some virtual delegate
- implement that in the "higher" library

…or move the needed shared bits to an even lower library that both A and B naturally depend on already.

https://chromium.googlesource.com/chromium/src/+/lkgr/docs/patterns/inversion-of-control.md has some (slightly, but not overly so) chromium-specific notes on this.

But layering in lldb is generally a mess, so I think your workaround is ok for now. It'd be nice to clean up lldb's layering at some point!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133534/new/

https://reviews.llvm.org/D133534