search

LDMX-Software / ldmx-sw

The Light Dark Matter eXperiment simulation and reconstruction framework.
https://ldmx-software.github.io
GNU General Public License v3.0
21 stars 19 forks source link

Static analysis meta-issue #1176

Closed EinarElen closed 9 hours ago

EinarElen commented 1 year ago

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 https://github.com/LDMX-Software/cmake/issues/17.

Whatever we decide for an approach for #1168 probably should guide if/how we would apply this.

Potential bugs/worth looking into

Medium priority (fix when convenient)

Low priority (might not be worth fixing)

Related issues/PRs

PRs with warning cleanup as part of the PR

EinarElen commented 1 year ago

One of the cases of using uninitialized memory comes from types like CalorimeterHit and its derived classes. Since several member variables don't have a default value and they are generally constructed like 

HitType hit;
hit.setA(A);
hit.setB(B);

it is really easy to construct a CalorimeterHit containing partially junk.

tvami commented 1 week ago

hey @EinarElen if you have the chance, can you review this issue and remove what was already done? Also do I understand correctly that running

denv cmake -B build -S . -Wmaybe-uninitialized
denv cmake --build build --target install -- -j$(nproc)

should have this fall? (i.e. this is not runtime, right?)

EinarElen commented 1 week ago

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 -DWARNINGS_AS_ERRORS=ON.

// 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 build

I 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

EinarElen commented 1 week ago

Instead of using warnings as errors while working on this, I would pipe stderr/stdout to a logfile :)

tvami commented 1 week ago

Thanks @EinarElen !

There are some warnings that are from ROOT and ACTS and... I don't know what we can do about those

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)

EinarElen commented 1 week ago

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

tvami commented 1 week ago

OK so running

just configure  -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang
just build

gives me this list

I think we should add 

set(CMAKE_CXX_STANDARD 17)

into ldmx-sw/CMakeLists.txt and run this command like that (then these above are gone and we have some other stuff coming in). Do you agree @EinarElen @tomeichlersmith ?

tvami commented 1 week ago

With set(CMAKE_CXX_STANDARD 17) the list is the following. log-part1.txt

tvami commented 1 week ago

Another question to Tom

Ecal/src/Ecal/EcalRawDecoder.cxx:133:48: warning: & has lower precedence than ==; == will be evaluated first [-Wparentheses]
      bool rid_ok = (w >> (shift_in_word + 7)) & packing::utility::mask<1> == 1;
                                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

was this meant to be an && (packing::utility::mask<1> == 1) ?

And then also 

Ecal/src/Ecal/EcalRawEncoder.cxx:138:32: warning: operator '<<' has lower precedence than '+'; '+' will be evaluated first [-Wshift-op-parentheses]
      word |= (1 << 12 + 1 + 6 + 8);

and

Ecal/src/Ecal/EcalRawEncoder.cxx:139:63: warning: operator '<<' has lower precedence than '+'; '+' will be evaluated first [-Wshift-op-parentheses]
      word |= (fpga_id & packing::utility::mask<8>) << 12 + 1 + 6;   // FPGA
tomeichlersmith commented 1 week ago

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
tvami commented 1 week ago

Thanks Tom, I'll have this included in the next batch!

tvami commented 1 week ago

Here is part-2 log-part2.txt

tvami commented 4 days ago

@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?

EinarElen commented 3 days ago

Was supposed to be with the clang/clang++ parts removed!

tvami commented 3 days ago

Was supposed to be with the clang/clang++ parts removed!

ok I updated your msg to 

just configure -DADDITIONAL_WARNINGS=ON  -DENABLE_CLANG_TIDY=ON  -DENABLE_LTO=ON