Static analysis meta-issue #1176
Comments
|
One of the cases of using uninitialized memory comes from types like HitType hit;
hit.setA(A);
hit.setB(B);it is really easy to construct a |
|
hey @EinarElen if you have the chance, can you review this issue and remove what was already done? should have this fall? (i.e. this is not runtime, right?) |
|
There are a couple of things you can run at compile time at the moment. I've checked off the ones I think are mostly clean but a lot of things have changed since them (including that you have patched a decent chunk of these on your own 😄). There are a couple of things we can do when it comes to static analysis, and ideally we would be running our CI with as many of these enabled as possible. I would probably try and squash them in steps but if you feel brave you can run them all at once, feel free. To make warnings a hard error, you can add // Build with clang and check that we don't get any warnings
just configure -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang
just build
// Clean build directory and increase warning levels and add LTO, see CompilerWarnings.cmake/InterProceduralOptimization.cmake
rm -rf build
just configure -DADDITIONAL_WARNINGS=ON -DENABLE_LTO=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang
just build
// Clean build directory and add clang-tidy (see StaticAnalysis.cmake)
rm -rf build
just configure -DADDITIONAL_WARNINGS=ON -DENABLE_CLANG_TIDY=ON -DENABLE_LTO=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang
just build
// And switch back to a build with GCC
rm -rf build
just configure -DADDITIONAL_WARNINGS=ON -DENABLE_CLANG_TIDY=ON -DENABLE_LTO=ON
just buildI think that if we can get a clean build with these, we are in a really good place. I would start with clang because, in general, i think the builds with clang are a bit faster to run and a lot of the warnings will overlap between the two. There are some warnings that are from ROOT and ACTS and... I don't know what we can do about those |
|
Instead of using warnings as errors while working on this, I would pipe stderr/stdout to a logfile :) |
|
Thanks @EinarElen !
Yes I'm a bit worried about what we should do if we enable these settings at the CI (after the ldmx-sw problems are fixed) |
|
I would suggest having an equivalent to the gold logs for the compilation steps. So them being there isnt the end of the world, but if any new ones show up we'll be notified |
|
OK so running gives me this list
I think we should add into |
|
With |
|
Another question to Tom was this meant to be an And then also and |
|
For the first one, we are checking if a bit is set, so - bool rid_ok = (w >> (shift_in_word + 7)) & packing::utility::mask<1> == 1;
+ bool rid_ok = ((w >> (shift_in_word + 7)) & packing::utility::mask<1>) == 1;For the others, I wanted to show how the bit shift was being calculated so the numbers being summed should have parentheses around them so that they are added before applied as the shift. - word |= (1 << 12 + 1 + 6 + 8);
+ word |= (1 << (12 + 1 + 6 + 8)); - word |= (fpga_id & packing::utility::mask<8>) << 12 + 1 + 6; // FPGA
+ word |= (fpga_id & packing::utility::mask<8>) << (12 + 1 + 6); // FPGA |
### What are the issues that this addresses? This is part of #1176 Specifically running ``` just configure -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang just build ``` Errors/warnings are gone, except for #1176 (comment) I moved *every* instances of `final override` to `override` now in bb2eef4 based on the guidelines in https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rh-override
|
Thanks Tom, I'll have this included in the next batch! |
|
Here is part-2 |
|
@EinarElen I'm in the last bit of this, but your last line is the same as the one before. What did you mean to have there? |
|
Was supposed to be with the clang/clang++ parts removed! |
ok I updated your msg to |
Last part of #1176 * Fix warning from clang-tidy * Update justfile with new compile settings
Similar to #1166 I'm creating this issue to track warnings from compilers or static analysis. Generally these aren't urgent to patch (I'll note if/when I think they need more immediate attention) but for future validation/PR workflows reducing the amount of these will allow us to use warnings that currently generate too many false-ish positives (true positives but probably not all that impactful) to be useful.
To do this I'm currently building a custom version of the container environment with some additional tools (clang, clang-tidy so-far). If this turns out to be something we want to use in the future we could consider adding to the regular container. I'm also adding some additional support for adding additional warnings in the cmake module while working on LDMX-Software/cmake#17.
Whatever we decide for an approach for #1168 probably should guide if/how we would apply this.
Potential bugs/worth looking into
-Wreturn-typeControl reaching the end of non-void functions.-Wmaybe-uninitializedUsing or recording uninitialized variables can be a real issue.Medium priority (fix when convenient)
virtual void onFileOpen() {}instead ofvirtual void onFileOpen(EventFile &eventFile) {}-WunusedUnused variables. Often just left over from development but can also be a mistake.-Wreorder: C++ member variables are always initialized in the order they are declared, even if the constructor initializer list has a different order. Usually harmless but if there are any dependencies on the order of parameters then it can be a big problemLow priority (might not be worth fixing)
Related issues/PRs
PRs with warning cleanup as part of the PR
The text was updated successfully, but these errors were encountered: