BUG: Initialize member variables#2347
BUG: Initialize member variables#2347thewtex merged 1 commit intoInsightSoftwareConsortium:masterfrom
Conversation
|
|
||
| Array<RealType> m_MaxDistance; | ||
| Array<IdentifierType> m_PixelCount; | ||
| Array<IdentifierType> m_PixelCount = Array<RealType>::Fill(NumericTraits<IdentifierType>::ZeroValue()); |
There was a problem hiding this comment.
They (m_PixelCount, m_MaxDistance, m_Sum) are initialized to size of number of threads in ::BeforeThreadedGenerateData() . Here they have zero size. I guess this can not compile too, AFAIK.
There was a problem hiding this comment.
You are right; I had a look at BeforeThreadedGenerateData() but here I assumed that the same thinking could not be applied. At the same time, I questioned myself about the dynamic analysis warning if the array has no allocated size at this point/when calling it for a newly created class instance to print its members. I will need to investigate more.
|
|
||
| Array<RealType> m_MaxDistance; | ||
| Array<IdentifierType> m_PixelCount; | ||
| Array<IdentifierType> m_PixelCount = Array<RealType>::Fill(NumericTraits<IdentifierType>::ZeroValue()); |
There was a problem hiding this comment.
I don't understand this. This is in the class declaration, for a non-static member so the initialization here is the default for any constructor that doesn't explicitly give an initialization for this member in the : member0(value0), member1(value1) part. But the old code with the zero-number-of-arguments constructor did so with that constructor; so I don't understand why this is better. Also, the right hand side is invoking Fill without a this pointer available, which I don't understand since Fill is not a static method.
There was a problem hiding this comment.
Right, calling Fill was not correct. The old code is producing warnings in the dynamic analysis. As said before, I ignore why since the array has no allocated size. Explicitly initialized to a zero size in ddd0e27.
ddd0e27 to
afee6cc
Compare
afee6cc to
0fa8eba
Compare
|
I am trying with simple own test
and Valgrind defects are still here, with BTW simple doesn't produce any Valgrind error. Prints |
|
@issakomi thanks for trying the fixes. Highly appreciated 💯. I think removing the variables from the If the current fix does not work, then other parts of the code where the same convention is used to initialize an What if instead of the current fix the following is done in the constructor? |
setting this in constructor (also removed Edit: only exactly like above, if you let Edit: |
|
Thanks again @issakomi 💯.
Nice. Not sure why other member variables of the same type and initialized with the empty braces are not throwing Valgrind defects, though 😔.
Worrying if they refer to
The ITK SW Guides states that all member variables should be printed whether they are publicly exposed or not (section C.21). I see your point; maybe others have better arguments than me to illustrate the need. |
OK, i see. |
|
Wait... looks like it is possible to avoid Valgring detect only if remove MaxDistance: [6.93676334907487e-310] That is what causing Valgrind error, at least in my tests, nothing else in .h, with Edit: IMHO worth to try, size 1 is anyway overridden later in BeforeThreadedGenerateData() |
Initialize member variables. Fixes: ``` UMC ==4407== Conditional jump or move depends on uninitialised value(s) ==4407== at 0xC3A9316: double_conversion::DoubleToStringConverter::ToShortestIeeeNumber(double, double_conversion::StringBuilder*, double_conversion::DoubleToStringConverter::DtoaMode) const (double-to-string.cc:172) ==4407== by 0x6835CE6: double_conversion::DoubleToStringConverter::ToShortest(double, double_conversion::StringBuilder*) const (double-to-string.h:166) ==4407== by 0x68354F6: (anonymous namespace)::ConvertToShortest(double_conversion::DoubleToStringConverter const&, double, double_conversion::StringBuilder&) (itkNumberToString.cxx:31) ==4407== by 0x68355F8: std::string (anonymous namespace)::FloatingPointNumberToString(double) (itkNumberToString.cxx:55) ==4407== by 0x683555B: itk::NumberToString::operator()(double) const (itkNumberToString.cxx:71) ==4407== by 0x6834EC5: std::ostream& itk::operator<< (std::ostream&, itk::Array const&) (itkArrayOutputSpecialization.cxx:37) ==4407== by 0x250C46: itk::DirectedHausdorffDistanceImageFilter, itk::Image >::PrintSelf(std::ostream&, itk::Indent) const (itkDirectedHausdorffDistanceImageFilter.hxx:222) ==4407== by 0x67FBB8D: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:125) ==4407== by 0x24E25F: itkDirectedHausdorffDistanceImageFilterTest(int, char**) (itkDirectedHausdorffDistanceImageFilterTest.cxx:60) ==4407== by 0x1D634B: main (ITKDistanceMapTestDriver.cxx:217) ``` and ``` UMC ==5645== Conditional jump or move depends on uninitialised value(s) ==5645== at 0x927FDB0: std::ostreambuf_iterator > std::num_put > >::_M_insert_int(std::ostreambuf_iterator >, std::ios_base&, char, >unsigned long) const (in /usr/lib64/libstdc++.so.6.0.19) ==5645== by 0x928004C: std::num_put > >::do_put(std::ostreambuf_iterator >, std::ios_base&, char, unsigned long) >const (in /usr/lib64/libstdc++.so.6.0.19) ==5645== by 0x928C2ED: std::ostream& std::ostream::_M_insert(unsigned long) (in /usr/lib64/libstdc++.so.6.0.19) ==5645== by 0x24F2B6: std::ostream& itk::operator<< <2u>(std::ostream&, itk::Size<2u> const&) (itkSize.h:412) ==5645== by 0x2C6068: itk::ImagePCAShapeModelEstimator, itk::Image >::PrintSelf(std::ostream&, itk::Indent) const >(itkImagePCAShapeModelEstimator.hxx:52) ==5645== by 0x6DBFB8D: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:125) ==5645== by 0x2C2F09: itkImagePCAShapeModelEstimatorTest(int, char**) (itkImagePCAShapeModelEstimatorTest.cxx:141) ==5645== by 0x20DA6B: main (ITKImageStatisticsTestDriver.cxx:272) ``` and ``` UMC ==6426== Conditional jump or move depends on uninitialised value(s) ==6426== at 0x10E3942C: __printf_fp_l (in /usr/lib64/libc-2.17.so) ==6426== by 0x10E38526: vfprintf (in /usr/lib64/libc-2.17.so) ==6426== by 0x10E63178: vsnprintf (in /usr/lib64/libc-2.17.so) ==6426== by 0x1095263C: ??? (in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x10958CA3: std::ostreambuf_iterator > std::num_put > >::_M_insert_float(std::ostreambuf_iterator >, std::ios_base&, char, char, >double) const (in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x10958F8F: std::num_put > >::do_put(std::ostreambuf_iterator >, std::ios_base&, char, double) const >(in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x10964B94: std::ostream& std::ostream::_M_insert(double) (in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x738573: itk::MultiphaseSparseFiniteDifferenceImageFilter, itk::Image, itk::Image, itk::ScalarChanAndVeseLevelSetFunction, itk::Image, itk::ConstrainedRegionBasedLevelSetFunctionSharedData, itk::Image, itk::ScalarChanAndVeseLevelSetFunctionData, itk::Image > > >, unsigned int>::PrintSelf(std::ostream&, itk::Indent) const (itkMultiphaseSparseFiniteDifferenceImageFilter.hxx:1421) ==6426== by 0xE497B8D: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:125) ==6426== by 0x736F6D: itkScalarChanAndVeseSparseLevelSetImageFilterTest1(int, char**) (itkScalarChanAndVeseSparseLevelSetImageFilterTest1.cxx:54) ==6426== by 0x331ADE: main (ITKReviewTestDriver.cxx:327) UMC ==6426== Conditional jump or move depends on uninitialised value(s) ==6426== at 0x10957DB0: std::ostreambuf_iterator > std::num_put > >::_M_insert_int(std::ostreambuf_iterator >, std::ios_base&, char, >unsigned long) const (in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x1095804C: std::num_put > >::do_put(std::ostreambuf_iterator >, std::ios_base&, char, unsigned long) >const (in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x109642ED: std::ostream& std::ostream::_M_insert(unsigned long) (in /usr/lib64/libstdc++.so.6.0.19) ==6426== by 0x7385BB: itk::MultiphaseSparseFiniteDifferenceImageFilter, itk::Image, itk::Image, itk::ScalarChanAndVeseLevelSetFunction, itk::Image, itk::ConstrainedRegionBasedLevelSetFunctionSharedData, itk::Image, itk::ScalarChanAndVeseLevelSetFunctionData, itk::Image > > >, unsigned int>::PrintSelf(std::ostream&, itk::Indent) const (itkMultiphaseSparseFiniteDifferenceImageFilter.hxx:1422) ==6426== by 0xE497B8D: itk::LightObject::Print(std::ostream&, itk::Indent) const (itkLightObject.cxx:125) ==6426== by 0x736F6D: itkScalarChanAndVeseSparseLevelSetImageFilterTest1(int, char**) (itkScalarChanAndVeseSparseLevelSetImageFilterTest1.cxx:54) ==6426== by 0x331ADE: main (ITKReviewTestDriver.cxx:327 ``` signaled at: https://open.cdash.org/viewDynamicAnalysis.php?buildid=7070588
0fa8eba to
f60a74e
Compare
Makes sense now ! Double checked the faulty line in the Valgrind message, and it was pointing to So, @issakomi 🏅 for the relentless effort made on this. Force pushed a commit removing the single element allocation call from the implementation file; in my mind makes more sense to have these variable not to have any elements when instantiating this class. Thanks to all for the patience and for bearing with the process. |
|
Great work tracking this down! I saw some arrays of number of threads. I glances at the code, and each thread is accessing it's id's index concurrently. This causes false sharing of memory between threads and can be a significant performance hinderance. Additionally, it restricts the algorithm to use the older fixed threading model. It looks like this filter could use some updating and improvements. |
Interesting. Thanks for casting light to this Brad ! Looks like that would deserve opening an issue to keep track of it? |
|
Merging for ITK 5.2rc03. |
…xUninitializedVariables
Initialize member variables.
Fixes:
and
and
signaled at:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=7070588
PR Checklist