-
Notifications
You must be signed in to change notification settings - Fork 164
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
teamtalk 5.8.2 crashes on older processors #1119
Comments
Can you download the archive here and see if it also crashes? https://github.com/BearWare/TeamTalk5/actions/runs/1425552219 |
Hi, |
Dumping the binary gives something like this:
pmulld is a SSE4.1 instruction and is being run unconditionally, so systems without SSE4.1 are going to crash here. libopus' source code doesn't use SSE4.1 (or AVX) here on purpose, so it seems like the compiler is doing auto-vectorization using these instructions? Building from v5.8.2 tag on my machine (Arch Linux, Ryzen 3700X) at least gives assembly like this:
I disabled every BUILD_TEAMTALK_ except CORE, disabled all FEATURE_ except opus, disabled all TOOLCHAIN_ except OPUS and ACE. I had the same result building master. Perhaps there's some build options being set on the GitHub builds? |
Here's the code from v1.8.2 that works:
No auto-vectorization at all! v1.8.2 is compiled with |
SSE4.1 can be disabled using a configure option for OPUS: 3bbb812 |
That commit seemed to break CI? Could you upload a binary for testing? |
Hm, it didn't toggle the options I wanted :( Now it compiles like this: I'm trying in this commit now: d1f7642 |
Hm, it looks more and more like a bug in OPUS's CMake file. Now it compiles without -mavx and -msse4.1 but OPUS_X86_MAY_HAVE_SSE4_1 is still there: |
The MAY_HAVE_SSE4_1 is fine I think as that's runtime detection? The problem is that the compiler needs -msse4.1 and -mavx to support building runtime SIMD for these instruction sets. I enabled SSE4_1_SUPPORTED and AVX_SUPPORTED and added this line to the opus cmakelists.txt: add_definitions("-fno-tree-vectorize") This stops GCC from adding SIMD when not asked to, at least with current versions of GCC. A longer term solution is fixing the Opus code to only use SIMD when it needs to, not say 'I have SIMD, automatically generate code for it in addition to my hand-writen code' |
Looking at upstream libopus, it's CMakeLists does in fact only add -msse and friends to individual files. Commit 927de8453c502586c03e25c169ec08f2a93ebc02 fixes this. Upstream bug for this issue is xiph/opus#154 and has been fixed. |
Some other project also hit this exact bug: vircadia/vircadia-native-core#429 |
Sorry for the avalanche of notifications. I think by upgrading to unreleased opus should allow you to fix this issue and avoid micromanaging AVX/SSE flags to it. Short term the -fno-tree-vectorize should have the same effect. Edit: I took apart the Mac binaries too, the one that works (build 5055) has this code where it crashes:
which seems to not have any SSE4.1 or AVX, while the crashing code has this:
You can see how it uses AVX ymm registers over SSE xmm registers. |
If OPUS is compiled with -mavx then the compiler will generate code with AVX instructions, also non-assembly code. I.e. "normal" C-code will contain AVX instructions and cause compatibility problems for non-AVX CPUs. |
libopus with correct cmake files (as seen upstream) will compile
properly, generate AVX instructions, and only use them on platforms that
support it. This won't cause compatibility issues with older systems.
|
So what CMake options would you provide here to make it work on Core2 Duo? |
I'd remove the '-DSSE4_1_SUPPORTED=OFF -DAVX_SUPPORTED=OFF', then in the opus git CmakeLists.txt in your other repo add: add_definitions("-fno-tree-vectorize") This will stop the compiler from automatically emitting SIMD instructions. Alternatively, I'd remove the '-DSSE4_1_SUPPORTED=OFF -DAVX_SUPPORTED=OFF', then upgrade to opus git master which has a better fix where it only uses -mavx on its AVX files and not the rest of the projects, same with -msse. See xiph/opus#154 |
@xogium If you run this Linux client application do you still experience the crash: |
Hi, unfortunately I haven't been able to get my friend to test this yet. I'll try and get him asap. Thanks ! |
With the removal of the AVX disable flag, one way to test this patch
would to be to run it on any system without AVX, including old dual
cores.
|
Reopen if it's still an issue. |
Hi,
this issue is very similar to #1067 but affects at the very least linux on x86_64, with older processors such as core 2 duo. As soon as people with one of these older processors join an opus channel, their client crashes immediately, the same way it did for mac.
I have no way to test windows with an older cpu to figure out if it also a problem over there.
The text was updated successfully, but these errors were encountered: