Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCC 13: EQ/Filters/Waveforms broken #11483

Closed
uklotzde opened this issue Apr 16, 2023 · 18 comments
Closed

GCC 13: EQ/Filters/Waveforms broken #11483

uklotzde opened this issue Apr 16, 2023 · 18 comments

Comments

@uklotzde
Copy link
Contributor

uklotzde commented Apr 16, 2023

Bug Description

image

Tested with both the old and new fidlib code from 2.3 (#11475 + #11478). Probably undefined behavior.

Built with -DOPTIMIZE=native and -DOPTIMIZE=portable on Intel Skylake.

All tests pass.

Updates

@uklotzde uklotzde added the bug label Apr 16, 2023
@uklotzde uklotzde changed the title GCC13: EQ/Filters/Waveforms broken GCC13 (OPTIMIZE=native): EQ/Filters/Waveforms broken Apr 16, 2023
@daschuer
Copy link
Member

Did you post two issues in GitHub issue? This is hard to handle. Can you spit them?
It this the EQ part a duplicate of #11409

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 16, 2023

It's a single issue. Probably a duplicate of #11409, which only mentions the audible errors. But the errors are also visible in waveforms. Same root cause.

The preconditions for reproducing it are described here, i.e. GCC 13 + -DOPTIMIZE=native or -DOPTIMIZE=portable.

@daschuer
Copy link
Member

Ah OK, so the visual issue in the waveforms happens due to the broken filter.

In #11409 the command line of fildlib.c has some extra flags. It would be nice to see the original compiler invocation in your case as well, to check if there are any differences.

Even though @hapst3r has compiled with OPTIMIZE=portable he experienced the filter issues. So there must be a subtle difference.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 16, 2023

-DOPTIMIZE=native (doesn't work):
/usr/lib64/ccache/cc -DMIXXX_USE_QOPENGL -DT_LINUX -g -fvisibility=hidden -pipe -O3 -ffast-math -funroll-loops -fomit-frame-pointer -march=native -o CMakeFiles/fidlib.dir/lib/fidlib/fidlib.c.o -c /home/uk/volumes/Build/cpp/mixxx/lib/fidlib/fidlib.c

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 16, 2023

-DOPTIMIZE=portable (doesn't work, too):
/usr/lib64/ccache/cc -DMIXXX_USE_QOPENGL -DT_LINUX -g -fvisibility=hidden -pipe -O3 -ffast-math -funroll-loops -fomit-frame-pointer -mtune=generic -o CMakeFiles/fidlib.dir/lib/fidlib/fidlib.c.o -c /home/uk/volumes/Build/cpp/mixxx/lib/fidlib/fidlib.c

@uklotzde
Copy link
Contributor Author

Let's burn the C/C++ toolchain. After multiple rebuilds with previously wiping the ccache cache it turns out that this issue affects all GCC 13 builds, independent of the optimization settings.

With Clang it still works as expected.

@uklotzde uklotzde changed the title GCC13 (OPTIMIZE=native): EQ/Filters/Waveforms broken GCC13: EQ/Filters/Waveforms broken Apr 17, 2023
@uklotzde uklotzde changed the title GCC13: EQ/Filters/Waveforms broken GCC 13: EQ/Filters/Waveforms broken Apr 17, 2023
@hapst3r
Copy link

hapst3r commented Apr 17, 2023

I have suggested to close #11409 cause your issue is more comprehensive and you seem to know your way around debugging much better than I do. Thus, I will try to contribute to this issue to the extent that I can and it makes sense.

For now, the only thing I can meaningfully contribute is to echo that when using clang instead of gcc, the problems mentioned in both issues don't appear.

I did git checkout 2.3.4 beforehand, because as mentioned before both main and 2.4 prompt me for qt5qml which is not available on my distro (openSUSE Tumbleweed).

The steps I took

cd /path/to/mixxx/repo
rm -rf ./build
export CC=/path/to/clang
export CXX=/path/to/clang++
cmake -DCMAKE_INSTALL_PREFIX=/usr/local -S /path/to/mixxx/repo
cmake --build /path/to/mixxx/repo --parallel `nproc`

Afterwards, running /path/to/mixxx/repo/build/mixxx works including the problematic functionalities in question (waveform, eq).

If there is anything I can contribute to the resolution even given my non-existent c++ skills, feel free to tell me what to do :)

kwizart pushed a commit to rpmfusion/mixxx that referenced this issue Apr 17, 2023
The build with GCC 13 is broken.

See also: <mixxxdj/mixxx#11483>
@daschuer
Copy link
Member

@uklotzde @hapst3r
I have just added a test with that check the waveform analyzer with a simple rectangular jump wave.
Please check if you are able to fail this test. If not, I need to use a real track for reference.
https://github.com/daschuer/mixxx/tree/analyzerwavefromtest_2

git checkout -b analyzerwavefromtest_2 2.3
git pull https://github.com/daschuer/mixxx.git analyzerwavefromtest_2

@hapst3r
Copy link

hapst3r commented Apr 18, 2023

@uklotzde @hapst3r I have just added a test with that check the waveform analyzer with a simple rectangular jump wave. Please check if you are able to fail this test. If not, I need to use a real track for reference. https://github.com/daschuer/mixxx/tree/analyzerwavefromtest_2

git checkout -b analyzerwavefromtest_2 2.3
git pull https://github.com/daschuer/mixxx.git analyzerwavefromtest_2

The build process terminates prematurely: 230418-analyzerwavefromtest_2-build.log.

Nonetheless, here's the ctest: 230418-analyzerwavefromtest_2-ctest.log

After the git checkout and git pull, I had to manually merge files, which I find surprising.

@uklotzde
Copy link
Contributor Author

It is pointless trying to run the tests if the build already failed. The huge test log only contains messages that the test executable was not found.

@uklotzde
Copy link
Contributor Author

I am not planning to spend more time and resources on analyzing these issues. Switching to Clang works for me for the time being.

Keeping this ancient C code working is a huge technical risk for the future of Mixxx.

@daschuer
Copy link
Member

@hapst3r I guess your 2.3 branch was not clean before
you can reset the branch to the original state by:

git reset 0a85171 --hard

@Swiftb0y
Copy link
Member

good news I guess, https://github.com/daschuer/mixxx/tree/analyzerwavefromtest_2 fails

Note: Google Test filter = AnalyzerWaveformTest.canary
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AnalyzerWaveformTest
[ RUN      ] AnalyzerWaveformTest.canary
Warning []: Buffer does not match "AnalyzerWaveformsTest" , actual buffer written to  "reference_buffers/AnalyzerWaveformsTest.actual"
/home/swiftb0y/Sourcerepositories/mixxx/src/test/analyserwaveformtest.cpp:84: Failure
Value of: false
  Actual: false
Expected: true
Warning []: Buffer does not match "AnalyzerWaveformSummaryTest" , actual buffer written to  "reference_buffers/AnalyzerWaveformSummaryTest.actual"
/home/swiftb0y/Sourcerepositories/mixxx/src/test/analyserwaveformtest.cpp:84: Failure
Value of: false
  Actual: false
Expected: true
[  FAILED  ] AnalyzerWaveformTest.canary (40 ms)

@Swiftb0y
Copy link
Member

I assume you'll want to conduct some tests on the results?
AnalyzerWaveform.zip

@daschuer
Copy link
Member

It looks like I have found a GCC 13 bug. The -HUGE_VAL is optimized away which makes fidlib go wild.
This demonstrates the issue:
https://godbolt.org/z/7bnb77hv4

@daschuer
Copy link
Member

I have reported it upstream:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109581

@apinski-cavium
Copy link

It looks like I have found a GCC 13 bug. The -HUGE_VAL is optimized away which makes fidlib go wild. This demonstrates the issue: https://godbolt.org/z/7bnb77hv4

And it is not a GCC 13 bug, especially when you use -ffast-math/-ffinite-math-only .

@Swiftb0y
Copy link
Member

Swiftb0y commented Aug 7, 2023

This can be closed, right?

@uklotzde uklotzde closed this as completed Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants