COMP: Fix for gcc13.2 compiler test failures

[hjmjohnson]

Three tests fail in ITK for Release and RelWithDebInfo builds on Ubuntu 24.04 with GCC 13.2:
itkGDCMLegacyMultiFrameTest (Failed)
itkGDCMImageReadWriteTest_MultiFrameMRIZSpacing (Failed)
itkGDCM_ComplianceTest_singlebit (Failed)

(Note: Debug builds do not fail these tests)

The initial review of the code did not indicate a problem, but it looks like perhaps an over-optimization that removes variables and causes the incorrect behavior to occur.

The function "FindDataElement" had duplicate code from "GetDataElement" so made "FindDataElement" use the "GetDataElement" working function.

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)
• Added test (or behavior not changed)

[hjmjohnson]

@malaterre I don't have proof, but I feel that this is a compiler optimization bug where the logic of the function is incorrectly compiled away. In Release and RelWithDebInfo compilations the function always returns false and does not seem to perform the DES.find() function.

Given the commented out line that explicitly assigned an iterator, it feels like perhaps this has been a problem in the past as well.

My workaround allows the test to pass reliably in all compilation modes and seems like a reasonable approach to solving the same problem.

If you agree that this is an acceptable approach, I'll work on making and upstream GDCM patch.

[thewtex]

LGTM

[hjmjohnson]

@thewtex I don't know how to deal with the clang-format failure. It would require a lot of changes to that file that are unrelated.

[thewtex]

@hjmjohnson we should ignore the clang-format warning for this PR, editing the GDCM sources. After the v5.4.0 I will make improvements to the linting infrastructure. Ideally, we get this into upstream GDCM before merging, as you suggested, but we can merge as-is if there is insufficient time before the release.

[hjmjohnson]

malaterre/GDCM#173

[hjmjohnson]

@thewtex @malaterre merged upstream. malaterre/GDCM#173.

Matt, I'll let you determine the best approach.

1). add this patch now, and then update GDCM after 5.4.0
2) update GDCM now, close this PR without merging.

[issakomi]

BTW, this will trigger copy of the DataElement (the function returns const DataElement&, but this const auto = forces copy of the object, IMO, previous variant also created local DataElement, maybe
return (GetDataElement(t) != GetDEEnd());
is faster). It is not a request to change, the issue is very odd. cc @N-Dekker

[N-Dekker]

@issakomi I just had a few days off, sorry for my late response! I'm not very familiar with this code. I would expect that it would search in the DataElementSet for a DataElement with the specified tag. But instead, it locally creates a DataElement with the specified tag, using default values for its VL and VR. It then tries to find this local DataElement in the DataElementSet. So I think it can only find a DataElement with those default values for VL and VR. Is that OK?

Anyway, I agree with you that a "copy" might be skipped by doing return (GetDataElement(t) != GetDEEnd()).

[issakomi]

I would expect that it would search in the DataElementSet for a DataElement with the specified tag. But instead, it locally creates a DataElement with the specified tag, using default values for its VL and VR. It then tries to find this local DataElement in the DataElementSet. So I think it can only find a DataElement with those default values for VL and VR. Is that OK?

Thank you. Honestly, I don't fully understand the logic. In fact the function bool FindDataElement(const Tag &t) takes one argument, the tag. But use DataElement ==, that operator is doing much more. Not sure.

[issakomi]

I have done some test (Linux, GCC 12). These GetDataElement and FindDataElement rely on DES.find. DES is std::set<DataElement>. DES.find doesn't seem to use DataElement's operator==. The function works (both old and new variants) with GCC 12, but I still don't fully understand ...

[issakomi]

It seems that comparing for find is something like !(x < y) && !(y < x)
without operator==, only with operator<. The last one uses Tag. But what was the issue with GCC 13 is still not clear.

[N-Dekker]

@issakomi Thanks for this little research! 👍

[issakomi]

Interesting that GDCM almost always does something like

  if( !subds2.FindDataElement(tps) ) return false;
  const DataElement &de = subds2.GetDataElement( tps );

FindDataElement does in fact 1st GetDataElement and 2nd compares returned one with invalid DataElement(Tag(0xffff, 0xffff)). So far 1 find too much (and it is not cheap: "Complexity: logarithmic in the size of the container")
It could be e.g. or something

  const DataElement &de = subds2.GetDataElement( tps );
  if (de == DataElement(Tag(0xffff, 0xffff)) return false;

There are dozens or hundreds of such double find s in e.g. gdcmImageHelper. Sometimes 3 times, e.g. in itkGDCMImageIO
Just FYI. I will not start PRs for this.

[thewtex]

@hjmjohnson @malaterre awesome, thank you for getting these issues resolved!

We should update GDCM from upstream in ITK. We want to:

• Pull in these improvements.
• Have our test suite run against the updates in GDCM re: spacing support, ensuring the same experience with vendored GDCM and system GDCM.
• Pull in CMake fixes re: Detailed CMake options are not marked as advanced. #4629 (there may more needed)

@hjmjohnson if you have the cycles and interest, please go ahead. Otherwise, I will start the update Monday.

[thewtex]

Integrated in #4647