BUG: Fix uninitialized value `ImageRegistrationMethodv4::m_NumberOfLevels` by jhlegarreta · Pull Request #3845 · InsightSoftwareConsortium/ITK

jhlegarreta · 2023-01-03T18:19:31Z

Fix uninitialized value warning for
itk::ImageRegistrationMethodv4::m_NumberOfLevels.

Fixes:

UMC ==16463== Conditional jump or move depends on uninitialised value(s)
==16463==    at 0x9BD983: itk::ImageRegistrationMethodv4, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::SetNumberOfLevels(unsigned long) (itkImageRegistrationMethodv4.hxx:881)
==16463==    by 0x9C44B6: itk::ImageRegistrationMethodv4, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::ImageRegistrationMethodv4() (itkImageRegistrationMethodv4.hxx:103)
==16463==    by 0x9BBFA0: itk::SyNImageRegistrationMethod, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::SyNImageRegistrationMethod() (itkSyNImageRegistrationMethod.hxx:41)
==16463==    by 0x9B5974: itk::SyNImageRegistrationMethod, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::New() (itkSyNImageRegistrationMethod.h:84)
==16463==    by 0xA15CD4: itkSyNPointSetRegistrationTest(int, char**) (itkSyNPointSetRegistrationTest.cxx:132)
==16463==    by 0x317927: main (ITKRegistrationMethodsv4TestDriver.cxx:222)

and similar warnings triggered by registration testing code.

Visible in:
https://open.cdash.org/viewDynamicAnalysis.php?buildid=8374098

Introduced in d1c46d6.

Recover the comment highlighting the initialization to a 3-level
registration, and reword to make the need for the call (with a value
different from the current value) even more explicit.

PR Checklist

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)

N-Dekker · 2023-01-03T18:41:27Z


-  // By default we set up a 3-level image registration.
-  this->SetNumberOfLevels(3);
+  this->SetNumberOfLevels(m_NumberOfLevels);


Please consider removing the this->SetNumberOfLevels call. The function call seems completely redundant now that this->m_NumberOfLevels is already initialized by the in-class { 3 } default member initialization.

@N-Dekker please have a closer look at the SetNumberOfLevels method implementation.

It is guarded by if (this->m_NumberOfLevels != numberOfLevels), which makes this call redundant? If we want this to be called, it would make sense to make in-class initialization to 0.

Uops you're right. Thanks for the heads-up @dzenanz ! Amended the commit to initialize it to 0 in 54367db.

Thanks for your effort so far, Jon. Do I understand correctly that this PR now changes the NumberOfLevels of a default constructed ImageRegistrationMethodv4 from 3 to 0? If so, is that intended?

😅 again my mistake ✋. Thanks for the head-up @N-Dekker. Fixed in e939dab. I think the initialization needs to be reworked to make it more consistent, as the m_SmoothingSigmasPerLevel and the m_MetricSamplingPercentagePerLevel ivars are initialized both in the constructor and the method at issue here to different values. But I'd leave it for a separate PR.

I think the initialization needs to be reworked to make it more consistent, as the m_SmoothingSigmasPerLevel and the m_MetricSamplingPercentagePerLevel ivars are initialized both in the constructor and the method at issue here to different values. But I'd leave it for a separate PR.

PR #3876.

N-Dekker · 2023-01-03T18:42:57Z

Thanks Jon! One more little suggestion, could you please make the commit text a little bit more specific?

For example:

BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels

Update: Thanks for adjusting the commit subject line this way, Jon 👍

dzenanz · 2023-01-03T19:11:22Z


-  // By default we set up a 3-level image registration.
-  this->SetNumberOfLevels(3);
+  this->SetNumberOfLevels(m_NumberOfLevels);


It is guarded by if (this->m_NumberOfLevels != numberOfLevels), which makes this call redundant? If we want this to be called, it would make sense to make in-class initialization to 0.

dzenanz · 2023-01-03T19:13:02Z

@@ -532,7 +532,7 @@ class ITK_TEMPLATE_EXPORT ImageRegistrationMethodv4 : public ProcessObject
  SetMetricSamplePoints();

  SizeValueType m_CurrentLevel;


Wouldn't it make sense to move all of these ivar initializations here?

That is a style change. I used the initialization list to fix the bug because the initialization trend that is being pushed in the code base is that one. IMHO best would be to propose the style change you propose in a separate PR. I could do a separate commit within this PR, but I prefer the bug to be fixed and merged as soon as possible.

I also realize that the implementation file contains method documentation blocks. Following the previous comment, best would be to do a separate PR to address that.

Anybody is welcome to contribute with them.

jhlegarreta · 2023-01-03T23:23:25Z

#3845 (comment) @N-Dekker Done in e847af4.

Fix uninitialized value warning for `itk::ImageRegistrationMethodv4::m_NumberOfLevels`. Fixes: ``` UMC ==16463== Conditional jump or move depends on uninitialised value(s) ==16463== at 0x9BD983: itk::ImageRegistrationMethodv4, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::SetNumberOfLevels(unsigned long) (itkImageRegistrationMethodv4.hxx:881) ==16463== by 0x9C44B6: itk::ImageRegistrationMethodv4, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::ImageRegistrationMethodv4() (itkImageRegistrationMethodv4.hxx:103) ==16463== by 0x9BBFA0: itk::SyNImageRegistrationMethod, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::SyNImageRegistrationMethod() (itkSyNImageRegistrationMethod.hxx:41) ==16463== by 0x9B5974: itk::SyNImageRegistrationMethod, itk::Image, itk::DisplacementFieldTransform, itk::Image, itk::PointSet > >::New() (itkSyNImageRegistrationMethod.h:84) ==16463== by 0xA15CD4: itkSyNPointSetRegistrationTest(int, char**) (itkSyNPointSetRegistrationTest.cxx:132) ==16463== by 0x317927: main (ITKRegistrationMethodsv4TestDriver.cxx:222) ``` and similar warnings triggered by registration testing code. Visible in: https://open.cdash.org/viewDynamicAnalysis.php?buildid=8374098 Introduced in d1c46d6. Recover the comment highlighting the initialization to a 3-level registration, and reword to make the need for the call (with a value different from the current value) even more explicit.

N-Dekker

Thanks Jon 👍

jhlegarreta · 2023-01-09T13:57:12Z

This is ready to be merged. IMO should be merged asap to prevent the addressed valgrind defects from being present across commits.

When a data member is set by a "setter" member function (`Set##name`, `On`, or `Off`), it is still recommended to initialize the member according to this guideline. Doing so prevents problems like the one addressed by pull request InsightSoftwareConsortium/ITK#3845 commit InsightSoftwareConsortium/ITK@a31e8bf "BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels" by Jon Haitz Legarreta Gorroño, 10 January 2023

jhlegarreta · 2023-01-16T03:46:08Z

I also realize that the implementation file contains method documentation blocks. Following the previous comment, best would be to do a separate PR to address that.

PR #3886.

When a data member is set by a "setter" member function (`Set##name`, `On`, or `Off`), it is still recommended to initialize the member according to this guideline. Doing so prevents problems like the one addressed by pull request InsightSoftwareConsortium/ITK#3845 commit InsightSoftwareConsortium/ITK@a31e8bf "BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels" by Jon Haitz Legarreta Gorroño, 10 January 2023

jhlegarreta mentioned this pull request Jan 3, 2023

PERF: Remove immediately modified ivar assignment #3834

Merged

2 tasks

github-actions bot added area:Registration Issues affecting the Registration module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances labels Jan 3, 2023

N-Dekker reviewed Jan 3, 2023

View reviewed changes

dzenanz reviewed Jan 3, 2023

View reviewed changes

jhlegarreta force-pushed the InitializeImageRegistrationMethodIvar branch 2 times, most recently from 54367db to e847af4 Compare January 3, 2023 23:20

jhlegarreta changed the title ~~BUG: Fix uninitialized value warning~~ BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels Jan 3, 2023

jhlegarreta changed the title ~~BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels~~ BUG: Fix uninitialized value ImageRegistrationMethodv4::m_NumberOfLevels Jan 3, 2023

jhlegarreta force-pushed the InitializeImageRegistrationMethodIvar branch from e847af4 to e939dab Compare January 3, 2023 23:51

N-Dekker approved these changes Jan 4, 2023

View reviewed changes

N-Dekker mentioned this pull request Jan 4, 2023

STYLE: Add in-class {} member initializers to objects created by New() #3851

Merged

N-Dekker mentioned this pull request Jan 9, 2023

DOC: Adjust initialization rule for padding data, smart pointers, etc. InsightSoftwareConsortium/ITKSoftwareGuide#192

Merged

dzenanz approved these changes Jan 10, 2023

View reviewed changes

dzenanz merged commit a31e8bf into InsightSoftwareConsortium:master Jan 10, 2023

jhlegarreta deleted the InitializeImageRegistrationMethodIvar branch January 10, 2023 14:00

N-Dekker mentioned this pull request Jan 15, 2023

DOC: Remove mistaken exception to data member initialization guideline InsightSoftwareConsortium/ITKSoftwareGuide#196

Merged

		@@ -532,7 +532,7 @@ class ITK_TEMPLATE_EXPORT ImageRegistrationMethodv4 : public ProcessObject
		SetMetricSamplePoints();

		SizeValueType m_CurrentLevel;

Uh oh!

Conversation

jhlegarreta commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

N-Dekker Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-Dekker commented Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhlegarreta Jan 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhlegarreta commented Jan 3, 2023

Uh oh!

N-Dekker left a comment

Choose a reason for hiding this comment

Uh oh!

jhlegarreta commented Jan 9, 2023

Uh oh!

jhlegarreta commented Jan 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jhlegarreta commented Jan 3, 2023 •

edited

Loading

N-Dekker Jan 3, 2023 •

edited

Loading

N-Dekker commented Jan 3, 2023 •

edited

Loading

jhlegarreta Jan 3, 2023 •

edited

Loading