Add optional GCC static analyzer support to CMake and Conan builds#817
Add optional GCC static analyzer support to CMake and Conan builds#817pnoltes merged 7 commits intoapache:masterfrom
Conversation
pnoltes
left a comment
There was a problem hiding this comment.
Thanks for the PR :)
Some small remarks and a question.
Is it also possible to enable one of the CI builds (ubuntu, gcc) with the analyzer option?
And then suppress the warnings we have not yet fixed (-Wno-analyzer-double-fclose, etc).
This can also help in follow up task (enable the analyzer warnings, flag per flag).
| set(CMAKE_C_FLAGS "-fanalyzer ${CMAKE_C_FLAGS}") | ||
| set(CMAKE_CXX_FLAGS "-fanalyzer ${CMAKE_CXX_FLAGS}") | ||
| else() | ||
| message(WARNING "ENABLE_GCC_ANALYZER is only supported with GCC compiler 10.0.0 OR higher.") |
There was a problem hiding this comment.
| message(WARNING "ENABLE_GCC_ANALYZER is only supported with GCC compiler 10.0.0 OR higher.") | |
| message(WARNING "ENABLE_GCC_ANALYZER is only supported with GCC") |
I would not mention the version, because that is not what is tested in the if.
|
Hi @pnoltes, thanks for the review. i can surely enable the analyzer in the Ubuntu GCC CI build and suppress the warnings, but i have a question. By reviewing the docs of -fanalyzer see Options That Control Static Analysis, how do you suggest adding these flags to the build in efficient way like to make it easy to suppress every flag one after the other ? my approach is 07c3f50 if there is a better and more efficient approach i will be happy to learn and apply |
02744ae to
2f9845d
Compare
I think this approach is good. I like to usage of 'add_compile_options' and the split-up of suppressing a warning per line. |
|
the CI build |
I am not sure, I was also looking at this. I should not be a time limit, this is set on 120 minutes. Seeing actions/runner-images#6680 (comment) I expect it is maybe due to reaching a the memory resource limit. Could you try using |
|
hi @pnoltes i tried ninja -j2 and |
I see that the debug one ran over the configured 120minute limit. I think we can test a bit more and then discuss whether we want gcc analyse in our builds and if so how. Could you provide an update with -j4 to check if that helps (a bit faster, but hopefully not parallel enough to eat too much memory)? I had a quick look and there are also ASF infra hosted runners (https://cwiki.apache.org/confluence/display/INFRA/ASF+Infra+provided+self-hosted+runners), so this could be another route. Not yet sure if this is (memory wise) a step up from the GitHub hosted runners. Maybe already good to minimise the usage of the gcc static analyzer to a single build? I think, not sure, that there is no benefit to run the gcc static analyzer in the debug build (compared to the RelWithDebInfo build) and seeing the latest build I expect the analyzer performs better with a RelWithDebInfo build. |
+1 Moreover, it does not make sense to enable analyzer for tests. WDYT @pnoltes |
added the -wno for the shift registers Fix: suppress static analyzer warning in tlsf.c added the suppresser for shift count reister added the suppresser for shift count register added -j8 for ninja build testing ninja -j2 testing ninja -j4 static analyzer only on RelWithDebInfo only testing ninja -j8 static analyzer only on RelWithDebInfo only testing ninja static analyzer only on RelWithDebInfo only testing ninja -j8 static analyzer only on RelWithDebInfo only enabling the gcc static analyzer on RelWithDebInfo enabline gcc static analyzer on RelWithDebInfo build only
dc4ffc2 to
1fbdb32
Compare
|
i tried to build with ninja -j4 and it worked and i tried with -j8 and it worked too |
|
well the CI workflow |
|
I’m actually in favor of including the test code. While we could create a specific CI pipeline with tests disabled, the goal of enabling the GCC static analyzer is to catch and fix issues as we write new code. Since development happens with tests enabled, the test code should be compatible with the analyzer as well. In my view, we should either accept a slower build (limiting parallelism to (.i.e.) -j4) or postpone the analyzer's introduction. I personally prefer the slower build. We can look into ASF-hosted runners later to optimize. Another option is running the analyzer only on PRs, but I’m not a fan of that approach as it loses immediate feedback during development. |
| -DCMAKE_BUILD_TYPE=${{ matrix.type }} | ||
| -DENABLE_CCACHE=ON | ||
| -DCMAKE_POLICY_VERSION_MINIMUM=3.5 | ||
| -DENABLE_GCC_ANALYZER=${{matrix.type == 'RelWithDebInfo' && 'ON' || 'OFF'}} |
There was a problem hiding this comment.
👍 , Nice clean way to do this
pnoltes
left a comment
There was a problem hiding this comment.
Looks good. IMO using -j8 for gcc static analyzer and only using it on RelWithDebInfo is for now the correct approach.
well i tried to make it slow by enable -j2 and i ran the workflow on my forked repo the debug build reached memory limit and has |
Maybe I missing something, but the current build uses gcc static analyzer with RelWithDebInfo and So looks good to me. I want to wait till we have 2 approvals and then merge the PR. |
|
@moelksasbyahmed Thanks for the contribution :) |
|
@pnoltes @PengZheng thanks for giving me oppertunity to contributoe to celix and answer my question |
This PR adds optional support for GCC’s static analyzer (-fanalyzer) to the Celix build system.
What’s included
Introduces a new
CMakeoption:ENABLE_GCC_ANALYZER (OFF by default)
When enabled and using
GCC,-fanalyzeris added to:CMAKE_C_FLAGSCMAKE_CXX_FLAGSExtends conanfile.py with a corresponding option
(enable_gcc_analyzer)so the analyzer can also be enabled in Conan-based builds.What’s not included
No analyzer warnings are fixed in this PR.
The analyzer is not enabled in CI.
No behavior or runtime changes.
GCC’s static analyzer has been improving significantly in recent releases and can help detect issues such as possible null dereferences and resource leaks.
This PR lays the groundwork for experimenting with the analyzer locally and for potential future CI integration.
Analyzer warnings can be addressed incrementally in follow-up PRs once there is agreement on scope and expectations.