Page MenuHomePhabricator

[lldb/CMake] Make it possible to disable plugins at configuration time
Needs ReviewPublic

Authored by JDevlieghere on Jan 19 2020, 11:20 PM.

Details

Reviewers
xiaobai
labath
Group Reviewers
Restricted Project
Summary

This patch introduces a new CMake macro add_lldb_plugin_subdirectory to be used by the plugins. It's similar to add_lldb_tool_subdirectory which makes it possible to disable certain tools at configuration time. With the new macro, the same is possible for plugins. The difference is that it includes the plugin's parent directory in the name as well, so that the variable is named LDB_PLUGIN_SYMBOLFILE_PDF_BUILD rather than LDB_PLUGIN_PDB_BUILD.

I've updated the macOS CMake cache to give an idea of what this looks like.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jan 19 2020, 11:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2020, 11:20 PM

How does this deal with linking flags etc.? Your cmake cache disabled LLDB_PLUGIN_SYMBOLFILE_NATIVEPDB_BUILD (well, it would without the LDB typo) but that just gives me a build that fails to link with ld: error: unable to find library -llldbPluginSymbolFileNativePDB.

Do we want to deal with dependencies between plugins? E.g. Language/CPlusPlus depending on Language/ClangCommon, so will CPlusPlus disabled too if I disable ClangCommon?

lldb/cmake/caches/Apple-lldb-macOS.cmake
21

LDB -> LLDB (here and in your description).

In principle, I think this is a good idea. We do have many optional components (and even more components that should be optional but aren't), that one may not need/want when building lldb for a specific use case. Also, libLLVM is configurable in a similar way. However:

  • as Raphael pointed out, this is incomplete. I'm not too worried about managing dependencies -- I'd consider this an expert feature, and I am happy with leaving it up to the user to ensure he does not disable a plugin that is used elsewhere (he should get a link error if he does). But that still leaves the question of how to register the plugins at startup. We currently have a hand-curated list of plugins in the SystemInitializer classes, and that will need to change somehow. This is further complicated by the fact that some plugins are also needed for lldb-server, while others (most) are used in liblldb only. The simplest solution (for usage, but maybe not for implementation) would be to have the plugins auto-register themselves via a global constructor. Some parts of llvm already to that, and we could create a special class for this which would also carry the annotations necessary to surpress the global constructor warnings. The alternative is to throw in a bunch of ifdefs into the SystemInitializers, which isn't particularly nice (though maybe it could be generated in cmake or via some .def tricks).
  • there will also be a need to somehow disable the tests for the relevant plugins. For instance all the NativePDB tests will fail when disabling that plugin (even on darwin, as the tests are written in a way to not require a windows host). I don't think we need to annotate all tests with the exact list of plugins that the require (that can be up to whoever wants to ensure that some configuration keeps working), but we should create some mechanism to do that.
  • I also wouldn't advertise this too much. Using these settings can easily send someone into uncharted waters, with random unexpected build errors or test failures -- we have enough of those even without new settings. And unlike LLVM_TARGETS_TO_BUILD, I doubt disabling some plugins will have measurable impact on the build time...
  • I'm aware of the linker issue when you reconfigure. I've spent quite some time investigating this and this seems related to the way libraries to link are cached by CMake. I verified that we're no longer passing the disabled library to target_link_libraries and yet the linker error remains. It looks like this might be fix starting with CMake 3.12 (https://cmake.org/cmake/help/git-stage/policy/CMP0073.html).
  • I've uploaded a patch for the system initializers: https://reviews.llvm.org/D73067

Very strong +1 on your last two points. I think I'll experiment a bit and then see which plugins I care about and how they affect the tests. I'll add a comment to the macro saying that using this functionality is a 100% at your own risk.

  • I'm aware of the linker issue when you reconfigure. I've spent quite some time investigating this and this seems related to the way libraries to link are cached by CMake. I verified that we're no longer passing the disabled library to target_link_libraries and yet the linker error remains. It looks like this might be fix starting with CMake 3.12 (https://cmake.org/cmake/help/git-stage/policy/CMP0073.html).

Interesting. This looks like something that we could hac^H^H^Hwork around even with cmake<=3.12 by force-setting the appropriate variable to "". If we cared enough about this, that is...

  • Rebased.
  • Tested the cache configuration on macOS.
labath added a comment.Mar 2 2020, 1:00 AM

This looks fine, but I think you'll first need to handle the lit changes to allow skipping tests. Otherwise all of the pdb and breakpad tests will break once you do this. I guess the reason that this hasn't happened yet is because you have a typo in your cache file.

lldb/source/API/SBCommandReturnObject.cpp
98–101 ↗(On Diff #247552)

?

JDevlieghere planned changes to this revision.Mar 2 2020, 10:17 AM
JDevlieghere marked an inline comment as done.

This looks fine, but I think you'll first need to handle the lit changes to allow skipping tests. Otherwise all of the pdb and breakpad tests will break once you do this. I guess the reason that this hasn't happened yet is because you have a typo in your cache file.

Right, I fixed the cache and then applied an old diff again. Shouldn't have tried to squeeze this in on a Sunday night. With the right cache values lldb-server is failing to link, so I'll have to take a look at that too.

lldb/source/API/SBCommandReturnObject.cpp
98–101 ↗(On Diff #247552)

Ugh, this should've been stashed, I was digging into some reproducer failure.

I like the idea! The one thing that kind of bothers me about this is you can inadvertently break your build without realizing by disabling a plugin that some enabled plugins depend on. Hopefully one day we'll be able to improve the diagnostics around this.

labath added a comment.Mar 4 2020, 3:57 AM

Looking this over again, I do see two open questions.

The first is that there is a disconnect between the place which *declares* something to be a plugin (the PLUGIN argument to add_lldb_library), and the place which *disables* something as a plugin (the macro invocation in the parent CMakeLists.txt). This creates a lot of potential for confusion and inconsistencies -- and indeed this patch already introduces some of those.

The second is the boilerplate required to disable the relevant tests. Right now you've just added the couple of features that you needed to disable the plugins of your choosing, but if we did that for all plugins, the code to do that would become fairly repetitive.

I am wondering if there isn't a way to solve both of these problems. For example, if we moved the disabling logic to the add_lldb_library function, then we could guarantee that only "real" plugins are eligible for being disabled. Also, since we already maintain a list of all plugin libraries, we could use this list to generate the appropriate lit features.

This would mean that we would not be able to reuse the llvm logic for skipping a directory, but that is literally just 4 lines of code, so I don't think it would be much of an issue. That may even make sense, because that llvm machinery was meant for disabling tools or entire subprojects, and not as a fine-grained control mechanism like this. In fact, this feature is kind of more similar to one choosing which components go into libLLVM.so, which is handled by a separate mechanism (LLVM_DYLIB_COMPONENTS).

Finally (though I don't know if you would consider this a feature or not), this way we might be able to arrange things so that the relevant plugins still get built even though they don't make their way into liblldb. That way one could still ensure that the relevant plugins build properly, and their unit tests could still run.

lldb/source/Plugins/LanguageRuntime/CMakeLists.txt
1–3

This isn't right. The actual plugins are located one level down (CPlusPlus/ItaniumABI, ObjC/AppleObjCRuntime, RenderScript/RenderScriptRuntime)

lldb/source/Plugins/Process/CMakeLists.txt
1–19

Some of these (Utility, Linux, NetBSD) aren't "real" plugins.