COMP: Fix uninitialized memory Valgrind defects

[jhlegarreta]

• COMP: Fix itkParallelSparseFieldLevelSetImageFilter uninitialized memory
• COMP: Fix itk::Histogram uninitialized memory
• STYLE: Remove an unnecessary static_cast from Histogram::PrintSelf
• STYLE: Prefer in-class {} member initializers
• STYLE: Make PrintSelf implementation style consistent
• STYLE: Remove duplicate statement in itk::Histogram::Initialize

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)

[jhlegarreta]

Not sure the second commit actually fixes the relevant issues.

[N-Dekker]

The first commit "COMP: Fix itkParallelSparseFieldLevelSetImageFilter uninitialized memory" certainly looks fine to me 👍

Apparently my previous bulk commits (PR #3851, #3913, #3917) don't address all unitialized data member problems. That is because they only address classes that have virtual member functions or a itkNewMacro, itkSimpleNewMacro, or itkCreateAnotherMacro macro call. ParallelSparseFieldCityBlockNeighborList appears to have neither of them!

[issakomi]

A size of the array m_TempMeasurementVector is set in Initialize function at the line 199 in itkHistogram.hxx, this line is the origin of the uninitialized value(s). Probably m_TempMeasurementVector is sometimes not used after, i guess, don't know exactly.

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

The {} in .h file seems to have no effect. Values could be initialized with e.g. (after the line mentioned above).

  for (size_t x = 0; x < m_TempMeasurementVector.size(); ++x)
  {
    m_TempMeasurementVector[x] = 0;
  }

It seems to fix the defect, but, honestly, i don't know is it a good idea or not. Just FYI. Not a suggestion.

BTW, another thing, s. Histogram.hxx
this->m_TempIndex.SetSize(this->GetMeasurementVectorSize());
is done two times in the same function, s. line 179 and 198.

[issakomi]

Private variables m_Pad1 and m_Pad2 (char[128]) seem to be unused. Other {}-d variables are initialized in the constructor to different values.

[N-Dekker]

for (size_t x = 0; x < m_TempMeasurementVector.size(); ++x)
{
m_TempMeasurementVector[x] = 0;
}

FWIW, m_TempMeasurementVector seems to be an itk::Array, so m_TempMeasurementVector.Fill(0) could also work. Anyway, good catch @issakomi !

[N-Dekker]

Private variables m_Pad1 and m_Pad2 (char[128]) seem to be unused.

If they are indeed unnecessary, please remove those m_Pad... members!

---

Update: I guess now that those "pad" members are there for some clever optimization purposes. To align the other data members efficiently, to avoid "false sharing". Honestly I do not know, as it's not my code. But does not seem useful to me to print them anyway. @jhlegarreta Do you really need the bits of m_Pad1 and m_Pad2 to be printed? Looking at PR #3872 commit c47ed1c

ITK/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx

Lines 86 to 93 in c47ed1c

	os << indent << "m_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;
	os << indent << "NeighborhoodOffset: " << m_NeighborhoodOffset << std::endl;
	os << indent << "StrideTable: " << m_StrideTable << std::endl;
	os << indent << "Pad2: " << m_Pad2 << std::endl;

[jhlegarreta]

Thanks for investigating the valgrind defects with itk::Histogram @issakomi 💯. I will try to review your comments and see if I can come up with a patch set as time permits.

Private variables m_Pad1 and m_Pad2 (char[128]) seem to be unused.

Thanks. Will have a look at them and remove if appropriate.

Other {}-d variables are initialized in the constructor to different values.

Will see if I have the time to look if the initialization can be done in the header file.

[dzenanz]

to avoid "false sharing"

This is the more likely reason. Not printing the pad members sounds like the right thing. And then they might not need to be initialized?

I did only a glancing review initially. It is good that other people review as well.

[jhlegarreta]

. @jhlegarreta Do you really need the bits of m_Pad1 and m_Pad2 to be printed? Looking at PR #3872 commit c47ed1c

I'd say that it is not a matter of preference: if the person that designed the class thought/required a given variable to be a member, then the ITK standard is to have it printed as any other member (unless static). Note that I did not read the body of the methods to determine in which ways the variable is used when addressing the defect/when printing the variable.

The standard could be improved if we come up with a rationale as to why a given variable should not be printed, and a way for automatically marking such variables for the purpose you describe Niels (a no-op macro if that makes sense/serves the purpose).

In this case, and until we propose something, I can comment both lines to serve as a reminder, or else I can remove them.

And then they might not need to be initialized?

I would keep the initialization to avoid issues. But as you wish.

[dzenanz]

Initializing them seems safer, even if not necessary.

[jhlegarreta]

A few notes:

• Have been thinking about the false sharing hypothesis: why use 2 variables instead of only 1?
• Have found that itk::SparseFieldLevelSetImageFilter has a similar class named itk::SparseFieldCityBlockNeighborList tha does not have the m_Pad1, m_Pad2 variables:

  ITK/Modules/Segmentation/LevelSets/include/itkSparseFieldLevelSetImageFilter.h

  Line 118 in c44f9c1

  unsigned int m_Size;

So maybe the variables were actually intended to be used. So for now, until we come up with more complete idea, I'll have them printed.

@issakomi @N-Dekker @dzenanz thanks for all the pointers.

[issakomi]

FYI, this padding is there since the first 'parallel' version of the filter (2003). S. here

  struct ThreadData
  {
    char pad1 [128];
...
    char pad2 [128];
  };
  
  /** An array storing the individual structures for each thread. */
  ThreadData *m_Data;

And here

[N-Dekker]

@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

[N-Dekker]

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

[hjmjohnson]

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

[N-Dekker]

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.

[dzenanz]

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

[N-Dekker]

• @dzenanz Check pull request ENH: Upgrade ITK from C++11 to C++14 #2563
  C++17 has got some really great improvements. 😃

[dzenanz]

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.

[dzenanz]

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.

[dzenanz]

@jhlegarreta If this PR is ready for merging, please merge it. If not, please turn it into a draft.

[jhlegarreta]

@dzenanz This is ready to be merged on my side; @N-Dekker was reluctant to initialize the m_Pad1 and m_Pad2 ivars, and @hjmjohnson also shared that. Niels assumed that those variables intended to avoid false sharing; we do not have any better hypothesis. I ignore if a steady decision has been made; if yes, it should be put in the ITK SW Guide. I am mostly away from keyboard these days, but I would very much like to have this merged as it will solve the Valgrind defects that are present since a month; changes can then be made in a separate PR (also to avoid needing to wait for the C++17 transition if that will affect how these variables are declared and printing the variables in the PrintSelf mehod), still ensuring that no new Valgrind defects are present.

[N-Dekker]

I would very much like to have this merged as it will solve the Valgrind defects that are present since a month

Thanks for you analysis, @jhlegarreta Just to recall (for those who did not remember), the undefined behavior appears caused by printing those padding bytes (while they were uninitialized):

ITK/Modules/Segmentation/LevelSets/include/itkParallelSparseFieldLevelSetImageFilter.hxx

Lines 86 to 93 in ef40e98

	os << indent << "m_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;
	os << indent << "NeighborhoodOffset: " << m_NeighborhoodOffset << std::endl;
	os << indent << "StrideTable: " << m_StrideTable << std::endl;
	os << indent << "Pad2: " << m_Pad2 << std::endl;

I'm still in favor of simply removing those print statements. Your PR appears to assume that m_Pad1 and m_Pad2 are zero-terminated character strings. But they are not. They are just raw bytes. Even after adding {} initializers, printing them does not yield any useful information. I mean, os << m_Pad1 always just prints an empty string, when m_Pad1 is {}-initialized.

[jhlegarreta]

Your PR appears to assume that m_Pad1 and m_Pad2 are zero-terminated character strings

My PR does no assumption on their utility and follows the current ITK SW Guide guidelines to print all non-static ivars.

I mean, os << m_Pad1 always just prints an empty string,

Simply because they are not used elsewhere, and we are only guessing about their utility.

[N-Dekker]

Just to explain what I mean by saying that PR (and specifically the print statements) appears to assume that m_Pad1 and m_Pad2 are always zero-terminated. If these arrays were filled with (only) non-zero values, printing them by os << m_Pad1 and os << m_Pad2 would still cause undefined behavior.

Example:

char arr[3] = { '1', '2', '3' };
std::cout << arr;  // Undefined behavior, arr is not zero-terminated!

[dzenanz]

Padding bytes to be reassessed in a new PR. @N-Dekker if you do not plan to do that soon (maybe as part of C++17 PR which allows use of alignas), it would be good to create an issue so we don't forget about it. I think there are some other instances of padding bytes.