Skip to content

Stop reading CV_8U masks through bool, which clang miscompiles - #347

Merged
atsju merged 5 commits into
githubdoe:masterfrom
Lucassifoni:fix-bool-mask-aliasing
Aug 3, 2026
Merged

Stop reading CV_8U masks through bool, which clang miscompiles#347
atsju merged 5 commits into
githubdoe:masterfrom
Lucassifoni:fix-bool-mask-aliasing

Conversation

@Lucassifoni

Copy link
Copy Markdown
Contributor

Note : this problem was diagnosed and fixed by Opus 5, but it allowed me to obtain wavefront generation on Mac OS with the latest codebase state.

There is a change in behaviour, newPhase is now initialized with zeros, and wasn't before.

It's currently building for all platforms : https://github.com/Lucassifoni/DFTFringe/actions?query=branch%3Afix-bool-mask-aliasing

---- BEGIN AI-generated summary ----

makeMask returns CV_8UC1 holding 0 or 255, and wavefront::workMask is Mat_<uint8_t>. Three places read those bytes as bool. That is undefined behaviour, and while gcc happens to treat any non zero byte as true, clang at -O2 evaluates every element as false. A release build made with clang is therefore silently wrong:

  • dftarea.cpp, in vortex(): count stays 0, so m2 becomes 0.0/0 and the subtraction that follows fills the whole image with NaN. Every wavefront computed from an interferogram ends up NaN, and so does every Zernike term.
  • dftarea.cpp, in subtractPlane(): nothing is accumulated, cv::solve runs on an uninitialised X and Z, and newPhase is returned without being written.
  • profileplot.cpp: no point passes the mask test, so the profile plot is empty.

Read the masks as uint8_t instead, and zero newPhase, whose unmasked pixels were never written.

Reproduced with clang 17 on arm64: reading a 255 byte through bool* yields false at -O2 and true at -O0.

makeMask returns CV_8UC1 holding 0 or 255, and wavefront::workMask is
Mat_<uint8_t>. Three places read those bytes as bool. That is undefined
behaviour, and while gcc happens to treat any non zero byte as true, clang at
-O2 evaluates every element as false. A release build made with clang is
therefore silently wrong:

- dftarea.cpp, in vortex(): count stays 0, so m2 becomes 0.0/0 and the
  subtraction that follows fills the whole image with NaN. Every wavefront
  computed from an interferogram ends up NaN, and so does every Zernike term.
- dftarea.cpp, in subtractPlane(): nothing is accumulated, cv::solve runs on an
  uninitialised X and Z, and newPhase is returned without being written.
- profileplot.cpp: no point passes the mask test, so the profile plot is empty.

Read the masks as uint8_t instead, and zero newPhase, whose unmasked pixels
were never written.

Reproduced with clang 17 on arm64: reading a 255 byte through bool* yields
false at -O2 and true at -O0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gr5

gr5 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

At first glance this looks like a good PR. Only 5 lines of code are changed.

Looking more carefully everything looks like an improvement (potential bug fix). I don't love the comments but they are helpful for the code review. Maybe after everyone has looked it over we can get rid of the comments. The comments are only to explain the code fixes and have no bearing on how the code works and don't belong in the code.

I built this with QT5 and it works fine. I didn't bother trying QT6 (yet).

I recommend we approve this merge.

@Lucassifoni

Copy link
Copy Markdown
Contributor Author

Hi George,
Yes, I think the comments are noise. I left them so you had the full context since you are the codebase experts, but I’ll clean up before anything moves forward.

@atsju

atsju commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

I fully agree with @gr5.
@Lucassifoni Please update the comments and we should merge. Good catch, thank you !

Additional note about Windows build failing locally: the build did pass, but the automatic PR commenting part failed because Lucas is working from a fork. This is expected due to write rights.

Comment thread dftarea.cpp Outdated
Comment thread dftarea.cpp Outdated
Comment thread profileplot.cpp Outdated
Comment thread dftarea.cpp Outdated
@gr5

gr5 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

I removed the comments. Will merge (or anyone else can) after it rebuilds.

@Lucassifoni

Copy link
Copy Markdown
Contributor Author

Thanks to both of you, this was the only code change since 6.2 to get the most used features of the app working in a clang build. I did not test all features yet (I certainly use only a small subset while making mirrors).

@atsju
atsju merged commit 2d136ee into githubdoe:master Aug 3, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants