Closed seanm closed 1 month ago
itkStatisticsUniqueLabelMapFilterTest
sounds familiar, because I think it is flaky. So this might be a genuine bug discovery. We shouldn't sweep it under the carpet. Does anyone have time to tackle this, now or in the future?
The test makes reference to JIRA issue number 3370: https://github.com/InsightSoftwareConsortium/ITK/blob/ba49784a99d36308b2101d24cda541e1f6a2e4c4/Modules/Filtering/LabelMap/test/itkStatisticsUniqueLabelMapFilterTest1.cxx#L35
Which is referenced in this commit: https://github.com/InsightSoftwareConsortium/ITK/commit/a18b3cf00e3812c3831efecd0703e73f0134ec0b
There is this issue: #3031.
There is this issue: #3031.
Ha! Totally forgot that I had apparently discovered this 2 years ago too!
We shouldn't sweep it under the carpet.
Agreed.
Does anyone have time to tackle this, now or in the future?
I don't actually use this part of ITK, so I'm afraid I won't tackle it, with so many other things to do.
But I would like to get Mac14.x-AppleClang-dbg-TSan
on cdash green, even if it requires suppressing likely bugs, because only if it's green will anyone notice/care if future TSan issues are found.
I am suspicious of the algorithm implemented here: https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/Filtering/LabelMap/include/itkAttributeUniqueLabelMapFilter.hxx#L79-L255
I am not sure how only the previous label line can be looked at to ensure they are not over lapping.
This solves the itkStatisticsUniqueLabelMapFilterTest1
test failure under TSan! Amazing! We'll see on cdash if it solves other similar failures too...
Several ITK tests report errors with thread sanitizer. For several of them, the issue is the same.
itkImage::SetPixel()
is being used by different threads to write to the same memory location simultaneously. Even if each thread is writing the same value (I didn't check), this is still undefined behaviour (without an atomic write).If each thread is writing the same value, I think this could be fixed, on gcc/clang at least, by using
__atomic_store_n
and__atomic_load_n
inGetPixel
/SetPixel
. I tried, but couldn't figure out all the C++ templates to get down to the raw buffer.Long term, C++20
std::atomic_ref
could help too. (It's a wrapper of the aforemenioned compiler intrinsics.)For example, the test
itkStatisticsUniqueLabelMapFilterTest1
results in the following report: