labath wrote:
> This was what we were doing in the [initial
> PR](https://github.com/llvm/llvm-project/pull/98528/files#diff-20a2060f8e87c6742d6f2c7ae97e919f8485995d7808bd9fccbfbede697a9ec7)
> but Pavel had correctly pointed out that the architecture was unnecessarily
> "baroque". GIven there will be only one instance of the TelemetrManager at a
> time and that we will not be creating a new manager on the fly, none of those
> complex hooks are necessary.
I think you've summarized this correctly. I just want to elaborate/add some
colour a bit:
The (baroque) way that the other plugins work serves two purposes:
1. controlling the type (class) of the plugin being created based on some
runtime property
2. creating an arbitrary number of plugin instances
For telemetry "plugins", I don't think we want/need (1) because the type of the
plugin is determined by the vendor -- at build time. A user can't change that
at runtime. They may be able to disable collection, but they can't say "well,
today I feel like reporting my telemetry to company <X>" and change a setting
or something to make that happen. I mean, it's probably doable, but I don't
think anyone has that as a requirement (if you do, let us know).
The second question is I think more interesting, and I think it boils down to
"what is the 'scope' of a TelemetryManager object". I can see two options, and
I think both of them are reasonable. One (which is what this PR does) is to
make it a process-wide object (a singleton). The other is to create one
instance per Debugger object. The advantage of the first one is that the object
is always available -- there's no problem with accessing the object from parts
of code (e.g. everything under the `Module` class) which are not tied to a
specific debugger instance (I'm sure you remember the contortions that we go
through to report progress events related to debug info parsing). The advantage
of the second one is that it fits in better with our object model and the
events we do report are automatically organized into a user session.
The reason I think the first one is better is that organizing events into
"sessions" is still possible with this model (just add a debugger_id field to
the reported event), but it still allows you to report debugger-less events,
but I could be convinced otherwise. However, even if we do that, I still don't
think we need a *list* of plugins (due to the first item). All we'd need to do
is replace `setInstance(std::unique_ptr<TelemetryManager>)` with something like
`setInstanceCreateCallback(std::unique_ptr<TelemetryManager> (*)(Debugger&))`.
> More of a question for @labath but do we have an example of another place
> where we have "semi plugins" like this and if not, what are existing plugins
> that could be reworked to be lighter weight?
We sort of do have a precedent for that, but it's in an unlikely place -- the
Platform plugins. It sounds hard to believe because Platforms are probably the
most complicated kinds of plugins, but part of that complexity comes from the
fact that they are doing two mostly separate things:
1. Registering themselves to handle the remote systems of the given kind. This
part is pretty much the same as all other plugins and uses the PluginManager,
iteration, and whatnot.
2. Registering themselves as the "Host" platform, if they match the current
build host. This part is achieved by a call to Platform::SetPlatform (and
that's an analogue to TelemetryManager::setInstance).
Here's how the PlatformLinux::Initialize looks like. The first part is just us
being "baroque":
```
PlatformPOSIX::Initialize();
if (g_initialize_count++ == 0) {
```
Next it does the the item (2)
```
#if defined(__linux__) && !defined(__ANDROID__)
PlatformSP default_platform_sp(new PlatformLinux(true));
default_platform_sp->SetSystemArchitecture(HostInfo::GetArchitecture());
Platform::SetHostPlatform(default_platform_sp);
#endif
```
And finally item (1)
```
PluginManager::RegisterPlugin(
PlatformLinux::GetPluginNameStatic(false),
PlatformLinux::GetPluginDescriptionStatic(false),
PlatformLinux::CreateInstance, nullptr);
}
```
Another slightly similar mechanism are the process plugins. ProcessLinux and
Process***BSD are a sort of a plugin, but they aren't even linked into lldb.
They are used by lldb-server, which only links the plugin which matches the
current build host (there's no runtime choice). I think it's reasonable to call
these "plugins" even though their choice is completely static.
> I think we need to have a sufficiently strong motivation to diverge from the
> existing way of writing plugins (or have a renaissance where we improve it
> for all the plugins).
Does this sound compelling enough?
Another similar mechanism we have is the ["static
polymorphism"](https://github.com/llvm/llvm-project/blob/55b0fde20a2ba1c67313cb4c8d6a30316facd6ad/lldb/include/lldb/Host/HostInfo.h#L35)
used in the Host classes. I would like to avoid that one because it requires
modifying an existing file to add a new implementation. Since this is something
that is explicitly meant to be extensible downstream, I think it'd be nice to
not need to do that.
FWIW, I actually would like to do a bit of a renaissance in the plugin
architecture, to reduce the boilerplate present in their definitions. However,
it's not really relevant here since some of that complexity really is necessary.
https://github.com/llvm/llvm-project/pull/126588
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
