llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.62k stars 11.83k forks source link

[PCH] __clang__analyzer__ is not defined when using a pch with --analyze #55723

Open wolfy1961 opened 2 years ago

wolfy1961 commented 2 years ago

The attached repro demonstrates that when we use clang with --analyze, the predefined macro __clanganalyzer_\ does not seem to be defined when we use a precompiled header. It is defined when compile without the pch. repro.zip

llvmbot commented 2 years ago

@llvm/issue-subscribers-clang-static-analyzer

steakhal commented 2 years ago

Can you reproduce this on linux systems?

wolfy1961 commented 2 years ago

Can you reproduce this on linux systems?

Yes. See the attached repro.

steakhal commented 2 years ago

My bad. Indeed weird.

clang/lib/Frontend/InitPreprocessor.cpp clang::InitializePreprocessor() supposed to initialize the defines, hence the __clang_analyzer__ is added in InitializePredefinedMacros(). After this, it reads the pch using the AddImplicitIncludePCH() It turns out if I print the value of PP.getPreprocessorOpts().SetUpStaticAnalyzer, it is true, which means that the macros is defined, yet something is wrong. I'm not sure where to look ATM, sorry.

jonesmz commented 2 weeks ago

This situation had me down a pretty deep rabbit hole today.

My project uses cmake for our builds, and has some custom cmake scripting around providing "analyze_target_name" targets to run clang-tidy against a particular cmake target. These targets refer to the cmake generated compile_commands.json file, which includes the commands for including the precompiled header that my targets use.

the precompiled header is, of course, built normally, and then included by clang-tidy when doing analysis.

This means that the precompiled header doesn't have __clang_analyzer__ defined, but the rest of the translation unit does.

This manifests with weird behavior where a macro that is different depending on __clang_analyzer__ will potentially see the wrong version of everything else, giving you "impossible" compile errors.

Given that my project uses the compile_commands.json and the normally built pch file, the only solution(s) i can come up with revolve around defining the same target twice, once for normal builds and once for analysis, or modifying the compile_commands.json to strip out the PCH file stuff, or to modify it to point to an alternative PCH file generated with __clang_analyzer__.

None of those are particularly fun to do, as Cmake itself lacks fundemental functionality to enable doing any of that.

whisperity commented 2 weeks ago

@jonesmz Are you building the PCH yourself during your normal build process, or are you obtaining it from an external source? AFAIK, __clang_analyzer__ is just a conventional preprocessor macro #define, so it should be possible to configure a version of your build (or say that in CMAKE_BUILD_TYPE=Debug it's always there...) that calls add_compile_definition() in a blanket way for your entire project. (For example, in LLVM, assert() is turned on by default in Debug builds, but you can still enable them in other build combinations with a CMake-level flag.) Or is CMake not respecting such flags when instructing the compiler to do PCH? Then it's likely an issue with CMake and we should start discussions with them as well.

If you obtain the PCH from an external source, it's a wholly different problem, though.

steakhal commented 2 weeks ago

That define is not supposed to by defined by hand. The clang analyzer should just do the right thing. To workaround the issue, of course one can directly define this in commandline or in cmake like you said - but the point is that shouldn't be necessary. Speaking of the bug, it may be related to the fact that macros can and should affect the AST. And if we have a a multiplexing ast consumer, then we may want to codegen and do the analysis on the same AST - while we actually want a slightly different AST for analysis, in particular we want this special define to be in effect. So, under the hood, the analyzer might get overruled by other ASTConsumers for setting up the preprocessor defines. I have no clues, it's just a theory.

whisperity commented 2 weeks ago

If I understood @jonesmz correctly, something had already produced the PCHs before executing the analysis, and the analysed TUs are only consuming the PCHs. At that moment, the analyser can no longer "do the right thing", because the PCHs are there as generated (without the right macros). So the only ways forward are to either delete the PCHs and trick the parser into falling back to the headers normally (but now defining the right macros) or to generate an analysis-ready PCH on the build system level.

steakhal commented 2 weeks ago

Oh, I see. In that case there is not much we can do unfortunately. To pretend we don't have a pch and doing the textual inclusions is before the analyzer would kick in the pipeline. If we had our own setup for setting up the compiler instance, then we could do something about it, but we don't.

jonesmz commented 2 weeks ago

I'm using the precompiled header that is built as part of my normal build, with the compile_commands.json of the same, and then asking clang-tidy to analyze all that.

Clang-tidy // llvm isn't responsible fixing issues with cmake, and this is a less than well designed aspect of the already heavily criticized tool.

I was just point out that this situation impacted me, is all.

I've added __clang_analyzer__ as an extra arg to cland-tidy and that gets me.... More correct.

Clearly my targets with pch weren't enabling the macro. Since all my extra checks that are added when clang tidy is used only started activating for my pch targets when I did that.

Its not perfect, since I can't get cmake to cooperate and I don't want my developers to have to re-run configure and wait 4 hours for a new build without pch enabled, but its definately better.