Skip to content

COMP: Fix uninitialized memory Valgrind defects #3933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dzenanz merged 6 commits into from
Mar 21, 2023
Merged

COMP: Fix uninitialized memory Valgrind defects #3933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0aef90c
COMP: Fix itkParallelSparseFieldLevelSetImageFilter uninitialized memory
jhlegarreta Feb 21, 2023
a14a495
COMP: Fix `itk::Histogram` uninitialized memory
jhlegarreta Feb 24, 2023
cbbbe49
STYLE: Remove an unnecessary static_cast from Histogram::PrintSelf
jhlegarreta Feb 21, 2023
d741ad0
STYLE: Prefer in-class `{}` member initializers
jhlegarreta Feb 24, 2023
05c7c5a
STYLE: Make `PrintSelf` implementation style consistent
jhlegarreta Feb 24, 2023
53d8dbe
STYLE: Remove duplicate statement in `itk::Histogram::Initialize`
jhlegarreta Feb 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Modules/Numerics/Statistics/include/itkHistogram.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,6 @@ Histogram<TMeasurement, TFrequencyContainer>::Initialize(const SizeType & size)
this->m_OffsetTable[i + 1] = num;
}

this->m_TempIndex.SetSize(this->GetMeasurementVectorSize());

m_NumberOfInstances = num;

// adjust the sizes of min max value containers
Expand All @@ -196,7 +194,10 @@ Histogram<TMeasurement, TFrequencyContainer>::Initialize(const SizeType & size)

// initialize auxiliary variables
this->m_TempIndex.SetSize(this->GetMeasurementVectorSize());
m_TempIndex.Fill(0);

this->m_TempMeasurementVector.SetSize(this->GetMeasurementVectorSize());
m_TempMeasurementVector.Fill(0);

// initialize the frequency container
m_FrequencyContainer->Initialize(this->m_OffsetTable[this->GetMeasurementVectorSize()]);
Expand Down Expand Up @@ -701,8 +702,7 @@ Histogram<TMeasurement, TFrequencyContainer>::PrintSelf(std::ostream & os, Inden
}
}

os << indent << "TempMeasurementVector: "
<< static_cast<typename NumericTraits<MeasurementVectorType>::PrintType>(m_TempMeasurementVector) << std::endl;
os << indent << "TempMeasurementVector: " << m_TempMeasurementVector << std::endl;
Comment thread
jhlegarreta marked this conversation as resolved.
os << indent << "TempIndex: " << static_cast<typename NumericTraits<IndexType>::PrintType>(m_TempIndex) << std::endl;
os << indent << "ClipBinsAtEnds: " << (m_ClipBinsAtEnds ? "On" : "Off") << std::endl;
}
Expand Down
14 changes: 7 additions & 7 deletions Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,16 @@ class ITK_TEMPLATE_EXPORT ParallelSparseFieldCityBlockNeighborList
Print(std::ostream & os, Indent indent) const;

private:
char m_Pad1[128];
unsigned int m_Size;
RadiusType m_Radius;
std::vector<unsigned int> m_ArrayIndex;
std::vector<OffsetType> m_NeighborhoodOffset;
char m_Pad1[128]{};
Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhlegarreta @dzenanz Please wait, before merging. I'm still considering to propose adjusting the ITK Coding Style Guide, to exclude padding bytes from {} initialization.

Update: please check: InsightSoftwareConsortium/ITKSoftwareGuide#192

Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker Mar 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could make it clearer that these are just padding bytes that do not need to be initialized by renaming m_Pad1 and m_Pad2 to something like m_UninitializedPaddingBytes1 and m_UninitializedPaddingBytes2...? We could also possibly make it clearer by declaring them as arrays of byte, instead of arrays of char. C++17 defines an std::byte type that we can easily borrow: https://en.cppreference.com/w/cpp/types/byte

Copy link
Copy Markdown
Member

@hjmjohnson hjmjohnson Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@N-Dekker I agree with our last comment here. the m_Pad1 is not really a state variable of the class. It is a solution to a problem that happens to use a char array to enforce performance gains, but does not represent the state of the class.

These pad variables should have good documentation that they are excluded from the "standard rules" applied to member variables that represent the state of the object.

Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker Mar 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @hjmjohnson By the way, it looks like C++17 offers some extra functionality to prevent false sharing. From https://en.cppreference.com/w/cpp/thread/hardware_destructive_interference_size:

Minimum offset between two objects to avoid false sharing.

struct keep_apart {
  alignas(std::hardware_destructive_interference_size) std::atomic<int> cat;
  alignas(std::hardware_destructive_interference_size) std::atomic<int> dog;
};

This would prevent false sharing between cat and dog, without having to declare those padding bytes explicitly. hardware_destructive_interference_size is typically 64 or 128, so similar to our m_Pad1 and m_Pad2 size.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When did we move to C++14? Maybe it is time to move to C++17?

Copy link
Copy Markdown
Contributor

@N-Dekker N-Dekker Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Mar 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was almost 2 years ago. I suggest making a forum topic about move to C++17, to see what others think about it. Perhaps describe implications on compiler support, and other similar considerations in the post.

Copy link
Copy Markdown
Member

@dzenanz dzenanz Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has other improvements which are not contentious. I will merge this. @N-Dekker can then make a follow-up PR which just deals with Padding bytes.

unsigned int m_Size{ 2 * Dimension };
RadiusType m_Radius{};
std::vector<unsigned int> m_ArrayIndex{};
std::vector<OffsetType> m_NeighborhoodOffset{};

/** An internal table for keeping track of stride lengths in a neighborhood,
* i.e. the memory offsets between pixels along each dimensional axis. */
unsigned int m_StrideTable[Dimension];
char m_Pad2[128];
unsigned int m_StrideTable[Dimension]{};
char m_Pad2[128]{};
};

/**
Expand Down
3 changes: 1 addition & 2 deletions Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ ParallelSparseFieldCityBlockNeighborList<TNeighborhoodType>::ParallelSparseField
NeighborhoodType it(m_Radius, dummy_image, dummy_image->GetRequestedRegion());
nCenter = it.Size() / 2;

m_Size = 2 * Dimension;
m_ArrayIndex.reserve(m_Size);
m_NeighborhoodOffset.reserve(m_Size);

Expand Down Expand Up @@ -83,7 +82,7 @@ ParallelSparseFieldCityBlockNeighborList<TNeighborhoodType>::Print(std::ostream

os << "ParallelSparseFieldCityBlockNeighborList: " << std::endl;

os << indent << "m_Pad1: " << m_Pad1 << std::endl;
os << indent << "Pad1: " << m_Pad1 << std::endl;
os << indent << "Size: " << m_Size << std::endl;
os << indent << "Radius: " << m_Radius << std::endl;
os << indent << "ArrayIndex: " << m_ArrayIndex << std::endl;
Expand Down