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
20 stars 15 forks source link

Compiler warnings #1208

Open tvami opened 9 months ago

tvami commented 9 months ago

Describe the bug In the idea of having clean compiler logs, here are a few more warnings I see:

Type "control reaches end of non-void function"

/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx: In member function 'std::vector<double> ldmx::HcalGeometry::rotateGlobalToLocalBarPosition(const std::vector<double>&, const ldmx::HcalID&) const':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx:77:1: warning: control reaches end of non-void function [-Wreturn-type]
   77 | }
      | ^
/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx: In member function 'ldmx::HcalGeometry::ScintillatorOrientation ldmx::HcalGeometry::getScintillatorOrientation(ldmx::HcalID) const':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/DetDescr/src/DetDescr/HcalGeometry.cxx:128:1: warning: control reaches end of non-void function [-Wreturn-type]
  128 | }
      | ^

Type " declaration of 'm' shadows a global declaration "

/Users/tav/Documents/1Research/LDMX/ldmx-sw/SimCore/G4DarkBreM/app/simulate.cxx: In constructor 'g4db::example::Hunk::Hunk(double, const string&)':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/SimCore/G4DarkBreM/app/simulate.cxx:121:37: warning: declaration of 'm' shadows a global declaration [-Wshadow]
  121 |   Hunk(double d, const std::string& m)
      |                  ~~~~~~~~~~~~~~~~~~~^
In file included from /usr/local/include/Geant4/CLHEP/Units/PhysicalConstants.h:42,
                 from /usr/local/include/Geant4/G4ParticleDefinition.hh:58,
                 from /usr/local/include/Geant4/G4ParticleTable.hh:58,
                 from /usr/local/include/Geant4/G4VUserPhysicsList.hh:92,
                 from /usr/local/include/Geant4/G4VModularPhysicsList.hh:59,
                 from /usr/local/include/Geant4/QBBC.hh:42,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/SimCore/G4DarkBreM/app/simulate.cxx:11:
/usr/local/include/Geant4/CLHEP/Units/SystemOfUnits.h:108:23: note: shadowed declaration is here
  108 |   static const double m  = meter;
      |                       ^

Type "deprecated"

/Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx: In constructor 'ldmx::Ort::ONNXRuntime::ONNXRuntime(const string&, const Ort::SessionOptions*)':
/Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:70:30: warning: 'void Ort::detail::TensorTypeAndShapeInfoImpl<T>::GetDimensions(int64_t*, size_t) const [with T = Ort::detail::Unowned<const OrtTensorTypeAndShapeInfo>; int64_t = long int; size_t = long unsigned int]' is deprecated: use GetShape() [-Wdeprecated-declarations]
   70 |     tensor_info.GetDimensions(input_node_dims_[input_name].data(), num_dims);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/include/Tools/ONNXRuntime.h:10,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:2:
/usr/local/include/onnxruntime_cxx_api.h:858:41: note: declared here
  858 |   [[deprecated("use GetShape()")]] void GetDimensions(int64_t* values, size_t values_count) const;  ///< Wraps OrtApi::GetDimensions
      |                                         ^~~~~~~~~~~~~
/Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:92:30: warning: 'void Ort::detail::TensorTypeAndShapeInfoImpl<T>::GetDimensions(int64_t*, size_t) const [with T = Ort::detail::Unowned<const OrtTensorTypeAndShapeInfo>; int64_t = long int; size_t = long unsigned int]' is deprecated: use GetShape() [-Wdeprecated-declarations]
   92 |     tensor_info.GetDimensions(output_node_dims_[output_name].data(), num_dims);
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /usr/local/include/onnxruntime_cxx_api.h:2088,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/include/Tools/ONNXRuntime.h:10,
                 from /Users/tav/Documents/1Research/LDMX/ldmx-sw/Tools/src/Tools/ONNXRuntime.cxx:2:
/usr/local/include/onnxruntime_cxx_inline.h:1061:13: note: declared here
 1061 | inline void TensorTypeAndShapeInfoImpl<T>::GetDimensions(int64_t* values, size_t values_count) const {
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Additional context

Similar issue at https://github.com/LDMX-Software/ldmx-sw/issues/1199

tvami commented 9 months ago
EinarElen commented 9 months ago

Thanks for making this issue! Cleaning up our builds and allowing us to start making use of more static analysis as part of the CI is an important goal. There's a overarching issue for these things in https://github.com/LDMX-Software/ldmx-sw/issues/1176. I'll make a list of related issues in there and link this one

Since https://github.com/LDMX-Software/docker/pull/67 and https://github.com/LDMX-Software/cmake/pull/19 we have some additional tools in the development container that can give additional static analysis. For compiler warnings, there are some CMake flags you can enable to add additional warnings that you can see in https://github.com/LDMX-Software/cmake/blob/trunk/CompilerWarnings.cmake

You can also build ldmx-sw with the clang++ compiler instead of GCC. There will be some warnings that clang catches that GCC doesn't and vice versa and sometimes the different compilers give better/worse feedback (see e.g. the warning in https://github.com/LDMX-Software/Hcal/pull/70 and https://github.com/LDMX-Software/Framework/issues/70).

Finally, for memory-issues we also have https://github.com/LDMX-Software/ldmx-sw/issues/1172 so if you find anything in that realm it might be discussed in there already.

w.r.t. the first one here, I think reaching the end of these functions should clearly signal a bug. From looking at the code, it isn't immediately obvious to me if there is a way to reach the end of those functions with valid input so I would probably suggest raising an exception.

EinarElen commented 9 months ago

I should say that it isn't obvious to me there's a way to trigger the end of those functions doesn't mean that the warning is silly, there have been similar problems caught by this warning before https://github.com/LDMX-Software/ldmx-sw/pull/1182/commits/ebbe456cb6b2e6e102051aecf56f301ac0dc7be7

EinarElen commented 9 months ago

For building with clang, you may have to manually merge https://github.com/LDMX-Software/TrigScint/pull/54 first. This reminded me that we should probably get around to merging it anyways.

tomeichlersmith commented 9 months ago

The second one is in the G4DarkBreM repo which I can patch pretty quickly.

tomeichlersmith commented 9 months ago

For the 3rd...

I also am unsure if GetShape is just a drop-in replacement, but there is an additional complexity. The v3 development container images have ONNX v1.2 which does not have GetShape while the v4 images have ONNX v1.15 which issues this deprecation warning. The good news is that we pin the ONNX version so we can pretty safely assume that GetDimension won't disppear on us despite this deprecation warning. The bad news is that if we want to remove this deprecation warning, we'd need to either completely drop v3 images (not something I'm willing to do) or do some ugly preprocessor stuff to align the APIs like I do with getting names:

https://github.com/LDMX-Software/ldmx-sw/blob/ddcc4daff5c70b45a392c0dfce4dfdd26ebc3e51/Tools/src/Tools/ONNXRuntime.cxx#L15-L37

EinarElen commented 9 months ago

The other option is to disable the warning, but I would prefer doing some #pragma's if it isnt happening in a lot of places

https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

With some comment referencing this issue of course

tvami commented 3 weeks ago

hey @tomeichlersmith your point in https://github.com/LDMX-Software/ldmx-sw/issues/1208#issuecomment-1723386035 is now relevant again since you did drop v3 images recently, right?