Page MenuHomePhabricator

[lldb/CMake] Auto-generate the Initialize and Terminate calls for plugin
Needs ReviewPublic

Authored by JDevlieghere on Mon, Jan 20, 4:03 PM.

Details

Summary

This patch changes the way we initialize and terminate the plugins in the system initializer. It uses a similar approach as LLVM's TARGETS_TO_BUILD with a def file that enumerates the configured plugins. To make this work I had to rename some of the plugins. I went for the solution with the least churn, but we should probably clean this up in the future as separate patches.

Diff Detail

Event Timeline

JDevlieghere created this revision.Mon, Jan 20, 4:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Jan 20, 4:03 PM
JDevlieghere marked an inline comment as done.Mon, Jan 20, 4:07 PM
JDevlieghere added inline comments.
lldb/source/API/SystemInitializerFull.cpp
28

I used the namespaces to limit the churn, but I would prefer to just remove them altogether form the plugins, or at least have the main entry point live under lldb_private.

Do we need to worry about ordering of the plugins?

lldb/source/Plugins/Platform/CMakeLists.txt
9 ↗(On Diff #239206)

I would use STREQUAL rather than MATCHES.

JDevlieghere retitled this revision from [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugins to [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugins (WIP).Mon, Jan 20, 4:35 PM
JDevlieghere updated this revision to Diff 239217.EditedMon, Jan 20, 5:14 PM

Do we need to worry about ordering of the plugins?

Yes, it appears to matter... I'm still seeing test failures which I need to debug. I'm not sure yet if they're because of missing initializers (that don't have a corresponding CMake plugin) or because of the ordering.

Overall, I like this, but I have a lot of comments:

  • I actually very much like the plugin namespace idea -- it makes it harder to use plugin code from non-plugin code, and allows plugins of the same category (e.g. ProcessLinux and ProcessNetBSD) to define similar concepts without fear of name clashes. I do see how that gets in the way of auto-generation though, so putting the initialization endpoints into a more predictable place seems like a good compromise. I wouldn't put the entire main class into the lldb_private namespace though, as that is also something that should not be accessed by generic code. Ideally, I'd just take the Initialize and Terminate (TBH, I don't think we really need the terminate function, but that may be more refactoring than you're ready for right now), and put it into a completely separate, predictibly-named file (void lldb_private::InitializeProcessLinux() in Plugins/Process/Linux/Initialization.h ?). That way the "main" file could be free to include anything it wants, without fear of polluting the namespace of anything, and we could get rid of the ScriptInterpreterPythonImpl thingy.
  • it would be nice to auto-generate the #include directives too. Including everything and then not using it sort of works, but it does not feel right. It should be fairly easy to generate an additional .def file with just the includes...
  • The way you disable ScriptInterpreterPython/Lua plugins is pretty hacky. Maybe the .def file should offer a way to exclude entire plugin classes?
#ifdef LLDB_WANT_SCRIPT_INTERPRETERS
ScriptInterpreterPython::Initialize(); // or whatever
#endif
  • some of the things you initialize are definitely not plugins (e.g. Plugins/Process/Utilty, Plugins/Language/ClangCommon). I think that things which don't need to register their entry points anywhere should not need to have the Initialize/Terminate goo... Can we avoid that, perhaps by making the "plugin" special at the cmake level. Since this code is used only by other plugins, it makes sense to have it somewhere under Plugins/, but it is not really a plugin, it does not need to register anything, and we do not need to be able to explicitly disable it (as it will get enabled/disabled automatically when used by the other plugins).
  • Having this as one bit patch is fine for now, but once we agree on the general idea, it should be split off into smaller patches, as some of the changes are not completely trivial (like the bunching of the macos platform plugins for instance)

Do we need to worry about ordering of the plugins?

Yes, it appears to matter... I'm still seeing test failures which I need to debug. I'm not sure yet if they're because of missing initializers (that don't have a corresponding CMake plugin) or because of the ordering.

Order of plugins within a kind should not matter, but it's possible it does. Some of the entry points may need to be changed so that they are less grabby. I'm not sure if order of plugin kinds matters right now, but I don't think it would be wrong if it did (e.g. have symbol files depend on object files). However, it looks like this approach already supports that...

Since the initialization function will have to have predictable names anyway, instead of generating the #include directives, we could just automatically forward declare the needed functions. So, the .def file would expand to something like:

extern lldb_initialize_objectfile_elf();
lldb_initialize_objectfile_elf();
extern lldb_initialize_objectfile_macho();
lldb_initialize_objectfile_macho();
...

That way we would have only one generated file, and we could ditch the Initialization.h file for every plugin (which would contain just the two forward declarations anyway). And this approach would be pretty close to what we'd need to do for dynamically loaded plugins (where instead of an extern declaration we'd have a dlsym lookup).

Complete rewrite based on the external forward declarations.

JDevlieghere retitled this revision from [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugins (WIP) to [lldb/CMake] Auto-generate the Initialize and Terminate calls for plugin.Wed, Feb 12, 3:58 PM

This looks like a bad upload -- based on some earlier version of this patch and not master..

Squash commits into single diff

This looks like a bad upload -- based on some earlier version of this patch and not master..

Yup, you're right, forgot to squash the commits.

I think this is great. Just a bit of polishing...

lldb/include/lldb/Core/PluginManager.h
25–29

This macro should forward to the "ADV" (advanced?) one.

lldb/source/API/SystemInitializerFull.cpp
27–29

I don't like this recycling of the name LLDB_PLUGIN. Maybe rename this to LLDB_PLUGIN_OP, or the "other" macro to LLDB_PLUGIN_DEFINE ?

lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp
58

missing _ADV. You might want to double check that the build with all targets enabled still works.

lldb/source/Plugins/Plugins.def.in
13

Maybe also mention LLDB_SCRIPT_PLUGIN ?

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

It looks like you're including the def file as Plugins/Plugins.def in SystemInitializerFull. Can we do the same thing here and avoid this LLDB_PLUGINS_INCLUDES business ?

Address code review feedback