COMP: Fix for gcc13.2 compiler test failures

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.

https://itk.org

Apache License 2.0

1.37k stars 660 forks source link

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

Closed hjmjohnson closed 1 month ago

hjmjohnson commented 2 months ago

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

[x] No API changes were made (or the changes have been approved)
[x] No major design changes were made (or the changes have been approved)
[x] Added test (or behavior not changed)

hjmjohnson commented 2 months ago

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

hjmjohnson commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

https://github.com/malaterre/GDCM/pull/173

hjmjohnson commented 2 months ago

@thewtex @malaterre merged upstream. https://github.com/malaterre/GDCM/pull/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.

thewtex commented 2 months ago

@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: #4629 (there may more needed)

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

thewtex commented 1 month ago

Integrated in https://github.com/InsightSoftwareConsortium/ITK/pull/4647