Link time asserts

[daschuer]

Finding a way to improve debug asserts in const expressions I have developed this PR as a drive by.

This extends our debug asserts to fail linking if the likely good branch is optimized away and not reachable.

This utilizes the __builtin_constant_p() build in function available on gcc and clang.

Unfortunately it does not work in case the DEBUG_ASSERT() statement is duplicated into two branches during optimization.
For these case I have added the old version as X_DEBUG_ASSERT()

Things like DEBUG_ASSERT(false) do no longer work, because they have no good branch and will fail.
For this purpose I have added DEBUG_ASSERT_UNREACHABLE(false)

The good thing is that this works without all constexpr keywords and uses the feature the compiler uses anyway in O3.
The bad thing is that it relies on the compiler optimization.

What do you think about it?

[daschuer]

A link error will look like this:

/usr/bin/ld: libmixxx-lib.a(mixxxapplication.cpp.o): in function `MixxxApplication::MixxxApplication(int&, char**)':
/home/sperry/workspace/2.3/src/mixxxapplication.cpp:63: undefined reference to `link_assert_failed()'
/usr/bin/ld: libmixxx-lib.a(mixxxapplication.cpp.o): in function `makei':
/home/sperry/workspace/2.3/src/mixxxapplication.cpp:65: undefined reference to `link_assert_failed()'
/usr/bin/ld: libmixxx-lib.a(jsonwebtask.cpp.o)(.debug_ranges+0x400000003ef8): reloc against `.text': error 4

[coveralls]

Pull Request Test Coverage Report for Build 1062803258

• 5 of 69 (7.25%) changed or added relevant lines in 34 files are covered.
• 523 unchanged lines in 10 files lost coverage.
• Overall coverage decreased (-0.02%) to 28.611%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/analyzer/analyzerbeats.cpp	0	1	0.0%
src/analyzer/analyzerkey.cpp	0	1	0.0%
src/analyzer/trackanalysisscheduler.cpp	0	1	0.0%
src/audio/types.cpp	0	1	0.0%
src/controllers/scripting/colormapper.cpp	0	1	0.0%
src/database/mixxxdb.cpp	0	1	0.0%
src/encoder/encodersndfileflac.cpp	0	1	0.0%
src/engine/controls/cuecontrol.cpp	1	2	50.0%
src/engine/enginebuffer.cpp	0	1	0.0%
src/library/library.cpp	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/engine/enginevumeter.cpp	1	90.24%
src/library/basesqltablemodel.cpp	1	31.82%
src/library/basetracktablemodel.cpp	3	21.84%
src/engine/sync/internalclock.cpp	9	80.51%
src/track/beatgrid.cpp	19	73.56%
src/track/cue.cpp	20	65.56%
src/track/beatmap.cpp	32	76.97%
src/sources/soundsourceflac.cpp	34	51.33%
src/track/track.cpp	146	51.12%
src/engine/controls/cuecontrol.cpp	258	69.43%

Totals
Change from base Build 1062435932:	-0.02%
Covered Lines:	20085
Relevant Lines:	70201

---

💛 - Coveralls

[uklotzde]

Do we need to split all such debug assertions now? In this case using & in the condition was intended.

[daschuer]

Without the split, the link assertion kicks in. I have not yet an explanation why.
I assume that the compiler optimizes it in a way that it creates a two separate branched where one has no longer a good path.

That the original code no longer works is really a caveat of this solution.

I think I need to examine the ASM code to find out what is going on.

[daschuer]

Comments addressed. I am not sure how continue here. This link time assert finds issues early, but has in rare cases false positives due to optimization. I was not able to fix all of them. If this hits a new contributor it will put extra work on us.
What do you think?

We can not merge this and test it from time to time ...
Make it an extra CI step ..

[Be-ing]

Could we run a CI job without optimization?

[daschuer]

Could we run a CI job without optimization?

This woks only with optimization. The link assert strikes in, if the good branch is optimized out and only the bad branch remains.

[daschuer]

The code above enables the feature only on optimized builds.
We can add an extra CMake flag to control it.
Off for local builds.
Than the contributor is surprised when CI fails.

Maybe we may merge it and collect experience ...

[uklotzde]

No, the builds should behave consistently and the outcome should always be predictable no matter where and how the code is built.

[uklotzde]

What do others think? I am open to try this out.

[Be-ing]

Any new automated checks should be reliable. If they are not reliable they are worse than useless; they are misleading and will end up getting ignored.

[daschuer]

Any new automated checks should be reliable.

The check itself is reliable once it is in place.

The issue is that the DEBUG_ASSERT does fail at link time when the compiler optimizes it ,by splitting up the good and the bad path. In this case the bad path is the only path and it fails.

If this happens, the contributor has two options, rearrange the code or use RUNTIME_DEBUG_ASSERT instead.
From our 2,187 asserts this was required in 5 cases (0,2%).

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.