Page MenuHomePhabricator

[lldb] Add SystemInitializerAllPlugins and delete copy-pasted Init code in SystemInitializerFull and SystemInitializerTest
AbandonedPublic

Authored by teemperor on Jan 20 2020, 12:54 AM.

Details

Summary

SystemInitializerFull and SystemInitializerTest currently have copy-pasted content that is constantly going out of sync as people
only update one of them when they add a new subsystem.

This adds a SystemInitializerAllPlugins that initialises all the plugin subsystems which is the shared code between these
two initialisers (The "full" initializer also initialises the interpreters and JIT on top of that). This gets rid of all the copy-pasted
code.

The "AllPlugins" is a bit of a misnomer at the moment as we still initialise the Debugger which isn't technically a plugin.

I had to restructure the CMake code in the Initializer subfolder into two subfolders as we would otherwise have to work around
LLVM's 'unknown source file' CMake check.

Diff Detail

Event Timeline

teemperor created this revision.Jan 20 2020, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2020, 12:54 AM
teemperor edited the summary of this revision. (Show Details)Jan 20 2020, 12:55 AM

This seems like a good idea, but I really don't like the AllPlugins name, when it does not, in fact, initialize all plugins. I'm not really sure what to name it because criterion is pretty complex (though reasonable).

Another way to achieve something like this would be to create something like Plugins/ObjectFile/AllPlugins.h (or sth), which would initialize all object file plugins (and similar for other plugin kinds). Then the inividual "system initializers" would call these. That would still leave us with a bit of duplication between lldb-test and liblldb, but this could be justifiable as liblldb actually fully initializes some additional plugin kinds while lldb-test does not. (And we add new plugins much more often than we add a completely different plugin kinds).

lldb/tools/lldb-test/CMakeLists.txt
20

I guess you don't need this then...

teemperor updated this revision to Diff 239251.Jan 21 2020, 1:32 AM
  • Removed all the duplicated linking flags too.
teemperor marked an inline comment as done.Jan 21 2020, 1:35 AM

I moved the single non-plugin call back to the original Full/Test subclasses so the name is now correct. Also I removed all the duplicated linking flags that I forgot to remove before.

I also don't think we should make this a header. With a header we still need to copy the linking flags around and it also deviates from the existing approach of using subclasses like SystemInitializerCommon (which anyway looks nicer).

I moved the single non-plugin call back to the original Full/Test subclasses so the name is now correct. Also I removed all the duplicated linking flags that I forgot to remove before.

This still leaves the question of the script interpreter plugins, which are suspiciously *not* included in "all plugins". The script interpreters are quite special, so I think it's fine to handle them separately -- the question is just how to convey that distinction. Move them into a different top level folder? Call this SystemInitializerMostPlugins ?

However, an even bigger question is what is the relationship of this patch and D73067? Right now, they seem to be taking the plugin system in two different directions, so I don't think it makes sense to accept either one before we decide what that direction is...

lldb/source/API/SystemInitializerFull.h
21

Maybe call this SystemInitializerAPI, or SystemInitializer(Lib)LLDB.. Having both "Full" and "AllPlugins" is confusing...

This still leaves the question of the script interpreter plugins, which are suspiciously *not* included in "all plugins". The script interpreters are quite special, so I think it's fine to handle them separately -- the question is just how to convey that distinction. Move them into a different top level folder? Call this SystemInitializerMostPlugins ?

Yeah, those 'plugins' aren't actually standalone plugins but require the SWIG wrapper code to link (and the SWIG wrapper code is compiled in the API/ folder). We can fix this by moving the SWIG code into the AllPlugins folder (which would either be done in this commit or as a follow-up).

However, an even bigger question is what is the relationship of this patch and D73067? Right now, they seem to be taking the plugin system in two different directions, so I don't think it makes sense to accept either one before we decide what that direction is...

D73067 is more focused on making the list of plugins to init/terminate depend on the CMake configuration, but even with D73067 we still need to copy around the linking flags, include the def file and the other boilerplate. I don't think we should deduplicate identical code with macros (especially since the SystemInitializers already took the approach of making subclasses). So this patch solves the code duplication, D73067 solves the fact that the plugin itself is not dependent on the actual plugin list.

This still leaves the question of the script interpreter plugins, which are suspiciously *not* included in "all plugins". The script interpreters are quite special, so I think it's fine to handle them separately -- the question is just how to convey that distinction. Move them into a different top level folder? Call this SystemInitializerMostPlugins ?

Yeah, those 'plugins' aren't actually standalone plugins but require the SWIG wrapper code to link (and the SWIG wrapper code is compiled in the API/ folder). We can fix this by moving the SWIG code into the AllPlugins folder (which would either be done in this commit or as a follow-up).

I'm afraid it's not that simple, as the swig code in turn depends on all the lldb public interfaces, which are provided by the API folder. So, even if you somehow manage to link this, it would still be nonsensical from the POV of lldb-test, as it does not include the lldb APIs. (That's why I said it may make sense to put these closer to/into the API folder.)

However, an even bigger question is what is the relationship of this patch and D73067? Right now, they seem to be taking the plugin system in two different directions, so I don't think it makes sense to accept either one before we decide what that direction is...

D73067 is more focused on making the list of plugins to init/terminate depend on the CMake configuration, but even with D73067 we still need to copy around the linking flags, include the def file and the other boilerplate. I don't think we should deduplicate identical code with macros (especially since the SystemInitializers already took the approach of making subclasses). So this patch solves the code duplication, D73067 solves the fact that the plugin itself is not dependent on the actual plugin list.

Right, I now understand what you meant by the header file comment. Yes, these are somewhat independent subject, but they are still touching the exact same code (== rebase nightmare), and I don't even think they are fully orthogonal -- e.g. if we go through with the other patch, including the idea for generating the #include list, then the amount of boilerplate is reduced by like 90%. At that point, it's not clear to me whether introducing a separate "initializer" package is worth the final 10%. After that work is done, we may decide that we still want to do this, or we may see a better way to achieve the same goal (maybe something in cmake).

TTTT, though I agree that the current situation is bad, I also don't want to give too much emphasis on the "all plugins" case. I see being able to pick and choose what we include as a very nice property, and I'd rather encourage that instead. Selecting plugins at build time is one thing, but I'd like to also have it for different programs. We now just have three users (liblldb, lldb-server and lldb-test), but already each one requires different sets of plugins. If, in the future lldb-test continues to grow, then it may make sense to split it into multiple utilities (e.g. lldb-objdump instead of lldb-test object-file, where the former would only depend on the object file plugins). The main reason I haven't done anything about that yet is because in the current state of affairs, each tool would end up transitively depending on the whole world anyway...

I just thought of another corner case (which may be important for the other patch too): Plugins/Process/Linux and Plugins/Process/NetBSD are used in lldb-server and are not/should not be used in liblldb (or lldb-test). Plugins/Process/Windows is even weirder because it's used from both :/.

teemperor abandoned this revision.Feb 27 2020, 1:18 AM

This is no longer needed after Jonas' patches.