Page MenuHomePhabricator

clang-analyzer plugins require LLVM_ENABLE_PLUGINS also
ClosedPublic

Authored by vtjnash on Feb 11 2022, 1:45 PM.

Details

Summary

The clang-analyzer plugins are not linked to a particular tool, so they
can only be compiled if plugins are broadly supported. We could opt
instead to decide whether to link them to specifically against clang or
with undefined symbols, depending on the value of LLVM_ENABLE_PLUGINS,
but we do not currently expect there to be a use case for that rather
niche configuration.

@simon_tatham Thanks for the clear issue text. I believe this will fix both of the configuration issues you uncovered in D119199.

Diff Detail

Event Timeline

vtjnash created this revision.Feb 11 2022, 1:45 PM
vtjnash requested review of this revision.Feb 11 2022, 1:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 1:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vtjnash edited the summary of this revision. (Show Details)Feb 11 2022, 1:47 PM
vtjnash added a reviewer: simon_tatham.
simon_tatham added a comment.EditedFeb 14 2022, 1:49 AM

I gave this patch a test against our downstream code, and found that it doesn't stop me needing the change I suggested in D119199: in clang/tools/driver/CMakeLists.txt, I had to change

if(CLANG_PLUGIN_SUPPORT)
  export_executable_symbols_for_plugins(clang)
endif()

so that the if statement reads

if(CLANG_PLUGIN_SUPPORT OR LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)

because otherwise, the clang/examples builds will still try to link against the clang executable target, which needs its symbols to have been exported.

We are not going to start exporting plugin support if the user explicitly disabled it with CLANG_PLUGIN_SUPPORT=OFF, but why is it trying to read that directory at all, when it should be disallowed by this PR unless CLANG_PLUGIN_SUPPORT=ON

I ran another test of this patch, by trying the cross product of the following cmake configuration choices (on a build targeting Windows, with LLVM_ENABLE_PROJECTS=clang):

  • LLVM_EXPORT_SYMBOLS_FOR_PLUGINS set to each of ON and OFF
  • CLANG_BUILD_EXAMPLES set to each of ON and OFF
  • CLANG_PLUGIN_SUPPORT set to each of ON and OFF

3 of those 8 cases failed.

  • export-symbols OFF, build-examples ON, plugin-support OFF
  • export-symbols OFF, build-examples ON, plugin-support ON
  • export-symbols ON, build-examples ON, plugin-support OFF (which is the configuration we're currently using downstream)

In all three cases, I saw errors such as this (suggesting that a plugin is being added to the list of things to test even though it wasn't built):

CMake Error at cmake/modules/AddLLVM.cmake:1821 (add_dependencies):
  The dependency target "AnnotateFunctions" of target "check-all" does not
  exist.
Call Stack (most recent call first):
  CMakeLists.txt:1134 (add_lit_target)

Perhaps not all of the 8 configurations in this cross product are actually sensible? But if one of them is not sensible, it would be better to get a clear error message saying why, than to have to debug things like the above.

vtjnash updated this revision to Diff 409241.Feb 16 2022, 7:25 AM

add one more missing case to check

Okay, this should work now for those cases! Sorry, I kept thinking it was clang/examples that was broken, and missed that it was clang/test that was failing, so I failed to see what I needed to fix there too, so that we were not trying to build the tests in configurations where the functionality is disabled.

simon_tatham accepted this revision.Feb 16 2022, 8:56 AM

LGTM this time. Thanks very much for the rework!

This revision is now accepted and ready to land.Feb 16 2022, 8:56 AM
This revision was automatically updated to reflect the committed changes.