Static analysis meta-issue

[EinarElen]

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

• [ ] -Wreturn-type Control reaching the end of non-void functions.
  • [ ] GetGammaProcessManager (SimCore)
  • [ ] getVisAttributes (SimCore)
  • [ ] rotateGlobalToLocalBarPosition/getScintillatorOrientation (DetDescr)
• [ ] -Wmaybe-uninitialized Using or recording uninitialized variables can be a real issue.

Medium priority (fix when convenient)

• [ ] Missing virtual for destructors. These are all over the place, mostly in processors, and probably not an issue most of the time. However, if you have a destructor that actually needs to do something then this could be a problem. This seems straight-forward to patch whenever you are touching a corresponding part of the code
• [ ] Shadowed virtual functions (e.g. virtual void onFileOpen() {} instead of virtual void onFileOpen(EventFile &eventFile) {}
• [ ] -Wunused Unused 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 problem

Low priority (might not be worth fixing)

• [ ] Signed/unsigned int conversions. These are also all over the place but I don't think these need to be patched in the same way. It comes up a lot when dealing with DetectorIDs (where most things are unsigned internally)
• [ ] Double promotion. This I suspect is even less important than the signed/unsigned int conversions, we can probably just turn this off (note: this is not narrowing from double to float, which could be an issue)
• [ ]

Related issues/PRs

• [ ] https://github.com/LDMX-Software/ldmx-sw/issues/1208
• [ ] https://github.com/LDMX-Software/Framework/issues/70
• [ ] https://github.com/LDMX-Software/ldmx-sw/issues/1199, https://github.com/LDMX-Software/ldmx-sw/pull/1207
• [ ] https://github.com/LDMX-Software/ldmx-sw/issues/1172

PRs with warning cleanup as part of the PR

• [ ] https://github.com/LDMX-Software/Hcal/pull/70
• [ ] https://github.com/LDMX-Software/TrigScint/pull/54
• [ ] https://github.com/LDMX-Software/SimCore/pull/95

[EinarElen]

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]

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]

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]

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

[tvami]

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]

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]

OK so running

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

gives me this list

• issue with TString.h, RStringView.hxx --> Outside of our scope
• nested namespace --> I guess I can fix this, but it should be fine too as is

/sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/Framework/include/Framework/Performance/Timer.h:10:20: warning: nested namespace definition is a C++17 extension; define each namespace separately [-Wc++17-extensions]
namespace framework::performance {
                   ^~~~~~~~~~~~~
                    { namespace performance
1 warning and 12 errors generated.

• missing optional --> I think this is fine in newer versions of clang, and we are using an older version?

In file included from /sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/build/Framework/EventDict.cxx:68:
/sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/Tracking/include/Tracking/Event/Track.h:86:8: error: no template named 'optional' in namespace 'std'
  std::optional<TrackState> getTrackState(TrackStateType tstype) const {
  ~~~~~^
/sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/Tracking/include/Tracking/Event/Track.h:88:45: error: no member named 'optional' in namespace 'std'
      if (ts.ts_type == tstype) return std::optional<TrackState>(ts);
                                       ~~~~~^
/sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/Tracking/include/Tracking/Event/Track.h:88:54: error: 'TrackState' does not refer to a value
      if (ts.ts_type == tstype) return std::optional<TrackState>(ts);
                                                     ^
/sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/Tracking/include/Tracking/Event/Track.h:57:10: note: declared here
  struct TrackState {
         ^
/sdf/home/t/tamasvami/iss1176-part1-uninitialized/ldmx-sw/Tracking/include/Tracking/Event/Track.h:90:17: error: no member named 'nullopt' in namespace 'std'
    return std::nullopt;
           ~~~~~^

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]

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

[tvami]

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]

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]

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

[tvami]

Here is part-2 log-part2.txt

[tvami]

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

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

[tvami]

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