[clang] Only build static analyzer sources if requested

[tbaederr]

There used to be a patch similar to this on Phabricator. I asked a long time ago if I can pick it up but the author told me they will work on it, which never happened.

IIRC there also was a problem with this simple patch since some other component depends on the static analyzer being built. Let's see what CI says.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Timm Baeder (tbaederr)

Changes

There used to be a patch similar to this on Phabricator. I asked a long time ago if I can pick it up but the author told me they will work on it, which never happened.

IIRC there also was a problem with this simple patch since some other component depends on the static analyzer being built. Let's see what CI says.

---

Full diff: https://github.com/llvm/llvm-project/pull/71653.diff

1 Files Affected:

• (modified) clang/lib/CMakeLists.txt (+3-1)

diff --git a/clang/lib/CMakeLists.txt b/clang/lib/CMakeLists.txt
index 1526d65795f8adf..e897fbb16c60d78 100644
--- a/clang/lib/CMakeLists.txt
+++ b/clang/lib/CMakeLists.txt
@@ -23,7 +23,9 @@ add_subdirectory(Tooling)
 add_subdirectory(DirectoryWatcher)
 add_subdirectory(Index)
 add_subdirectory(IndexSerialization)
-add_subdirectory(StaticAnalyzer)
+if(CLANG_ENABLE_STATIC_ANALYZER)
+  add_subdirectory(StaticAnalyzer)
+endif()
 add_subdirectory(Format)
 if(CLANG_INCLUDE_TESTS)
   add_subdirectory(Testing)

[llvm-beanz]

This approach slightly bothers me. One of the mistakes I think we (and by we I mean me) have made in the build system is allowing lots of things to be disabled from the configuration.

In general I think it is desirable to build and test code always, but to support modularity in what gets built into the final products.

I realize there is some contention between that approach having fast build and test cycles, but the tradeoff between fast and adequate coverage is not always an easy line to draw.

(cc @petrhosek & @smeenai)

[tbaederr]

Well yes, but I'm not introducing the option here, I'm just making it work.

[smeenai]

I agree with Chris in general. This case is a bit different because the static analyzer sources would be included in Clang otherwise instead of being a standalone target you could just not build (roughly analogous to LLVM_TARGETS_TO_BUILD), but what are the concrete savings (build time, binary size, etc.) from the option, to weigh against the added build complexity?

I see some unguarded uses of the static analyzer libraries, e.g. clang/tools/clang-check/CMakeLists.txt unconditionally references clangStaticAnalyzerFrontend, so those would need to be adjusted as well if we went this route. It seems like that can get easily broken in the future as well though.

[llvm-beanz]

Well yes, but I'm not introducing the option here, I'm just making it work.

My argument here would be that if the option doesn't work, maybe we shouldn't fix it. In particular I think having the static analyzer be conditional in the testing tools makes for a more complicated testing matrix. If you disable the static analyzer in the build you also need to disable it in the testing tools and the test cases.

This becomes a more complicated issue because changes in the AST library can impact correctness of the static analyzer, so should we really be supporting disabling the static analyzer from the testing?

It seems to me that we should maybe support CLANG_ENABLE_STATIC_ANALYZER as a way to remove the static analyzer from the clang built product, but not from the build, testing tools, or test cases.

[petrhosek]

I agree with @llvm-beanz but I'd propose going even further and removing this option altogether. Rather than having separate CLANG_ENABLE_<TOOL> options for each tool which doesn't scale very well, we should encourage developers to use LLVM_DISTRIBUTION_COMPONENTS to control which tools to build.