COMP: Fix for gcc13.2 compiler test failures #4636

hjmjohnson · 2024-05-02T15:21:48Z

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 · 2024-05-02T15:27:03Z

@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 · 2024-05-02T15:44:50Z

@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 · 2024-05-02T17:25:49Z

@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 · 2024-05-02T20:11:58Z

malaterre/GDCM#173

hjmjohnson · 2024-05-03T12:48:25Z

@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 · 2024-05-03T17:08:04Z

Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmDataSet.h

-      return true;
-      }
-    return false;
+    const auto it = GetDataElement(t);


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

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

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.

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

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.

@issakomi Thanks for this little research! 👍

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 · 2024-05-03T18:55:13Z

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

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.

thewtex · 2024-05-06T18:00:10Z

Integrated in #4647

The original code requested `gdcm::DataSet` to search for one and the same data element three times in a row, by calling both FindDataElement and GetDataElement for the very same tag. With this commit, the search is only performed once. Issue found by Mihail Isakov: InsightSoftwareConsortium#4636 (comment)

hjmjohnson added the type:Compiler Compiler support or related warnings label May 2, 2024

hjmjohnson added this to the ITK 5.4.0 milestone May 2, 2024

hjmjohnson requested review from thewtex and malaterre May 2, 2024 15:21

hjmjohnson self-assigned this May 2, 2024

github-actions bot added the area:ThirdParty Issues affecting the ThirdParty module label May 2, 2024

thewtex approved these changes May 2, 2024

View reviewed changes

hjmjohnson force-pushed the gdcm-gcc13.2-workarounds branch from a98ed44 to 0ea2f6d Compare May 2, 2024 15:47

github-actions bot added area:Core Issues affecting the Core module area:Segmentation Issues affecting the Segmentation module labels May 2, 2024

hjmjohnson force-pushed the gdcm-gcc13.2-workarounds branch from 95f966e to 0ea2f6d Compare May 2, 2024 20:39

github-actions bot removed area:Core Issues affecting the Core module area:Segmentation Issues affecting the Segmentation module labels May 2, 2024

hjmjohnson force-pushed the gdcm-gcc13.2-workarounds branch from 0ea2f6d to b0fc983 Compare May 3, 2024 11:56

issakomi reviewed May 3, 2024

View reviewed changes

hjmjohnson force-pushed the gdcm-gcc13.2-workarounds branch from b0fc983 to 8eeba79 Compare May 5, 2024 12:48

thewtex closed this May 6, 2024

N-Dekker mentioned this pull request May 8, 2024

PERF: Avoid redundant searches for a data element in a gdcm::DataSet #4650

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COMP: Fix for gcc13.2 compiler test failures #4636

COMP: Fix for gcc13.2 compiler test failures #4636

hjmjohnson commented May 2, 2024

hjmjohnson commented May 2, 2024

thewtex left a comment

hjmjohnson commented May 2, 2024

thewtex commented May 2, 2024

hjmjohnson commented May 2, 2024

hjmjohnson commented May 3, 2024

issakomi May 3, 2024 •

edited

Loading

N-Dekker May 7, 2024

issakomi May 7, 2024

issakomi May 7, 2024 •

edited

Loading

issakomi May 7, 2024 •

edited

Loading

N-Dekker May 7, 2024

issakomi May 8, 2024

thewtex commented May 3, 2024

thewtex commented May 6, 2024

COMP: Fix for gcc13.2 compiler test failures #4636

COMP: Fix for gcc13.2 compiler test failures #4636

Conversation

hjmjohnson commented May 2, 2024

PR Checklist

hjmjohnson commented May 2, 2024

thewtex left a comment

Choose a reason for hiding this comment

hjmjohnson commented May 2, 2024

thewtex commented May 2, 2024

hjmjohnson commented May 2, 2024

hjmjohnson commented May 3, 2024

issakomi May 3, 2024 • edited Loading

Choose a reason for hiding this comment

N-Dekker May 7, 2024

Choose a reason for hiding this comment

issakomi May 7, 2024

Choose a reason for hiding this comment

issakomi May 7, 2024 • edited Loading

Choose a reason for hiding this comment

issakomi May 7, 2024 • edited Loading

Choose a reason for hiding this comment

N-Dekker May 7, 2024

Choose a reason for hiding this comment

issakomi May 8, 2024

Choose a reason for hiding this comment

thewtex commented May 3, 2024

thewtex commented May 6, 2024

issakomi May 3, 2024 •

edited

Loading

issakomi May 7, 2024 •

edited

Loading

issakomi May 7, 2024 •

edited

Loading