| Summary: | SDL_atomic infinite recursion on armv6/armv5 w/ thumb | ||
|---|---|---|---|
| Product: | SDL | Reporter: | Braden Obrzut <admin> |
| Component: | atomic | Assignee: | Sam Lantinga <slouken> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | blocker | ||
| Priority: | P2 | Keywords: | target-2.0.10 |
| Version: | 2.0.9 | ||
| Hardware: | ARM | ||
| OS: | Android (All) | ||
|
Description
Braden Obrzut
2019-06-22 00:54:07 UTC
This is the commit: https://hg.libsdl.org/SDL/rev/773cbb61ad27 armv5 is quite old to build for Android, isn't it ? Maybe this depends on NDK version (latest are not even supporting this) ? I'm using ndk r14b (which does still work even though I see reports of 1 Not sure what I bumped to make it submit my last comment while I was typing. Anyway, I think I've heard that r14b isn't supposed to work, but it does for me. In any case though I was compiling for armv5 by accident (thought I set it to armv8, but apparently I neglected a 64). What I can say is, although I don't know the consequences, that the final path (https://hg.libsdl.org/SDL/file/9d3f245739f8/include/SDL_atomic.h#l178) works for me. I wouldn't personally be bothered if SDL only supported armv7 and v8, although I imagine there are still some people that care about armv6. But as it is right now it is possible to build a version of SDL that crashes whenever atomics are used. I think for Android, nobody will use armv5, and for a while. because we bumped the minSdkTarget, and because there are not more armv5 device in that range. Moreover, GooglePlay is now requiring 64 bits apps. I mean support 64 bits arch, and probably in the future, only to be only 64 bits. Not even sure RPY are still building with armv5. Do you have a testcase to reproduce the crash ? I don't have a concise test case right now, but in theory all you need to do is call SDL_LockMutex or even SDL_Init while building with thumb enabled ARMv5 or ARMv6. I do agree that probably no one is using ARMv5 Android devices at this point. The oldest one I have is ARMv7 Samsung Galaxy Player (hence my comment about not personally caring for anything less than v7). But I do know the Raspberry Pi still has ARMv6 devices which should also be affected since they share a branch here? My distribution model is an APK on my website so Google Play requirements don't really affect me. Only limiting factor is what SDL can support. If you're curious this is the project that I'm working on: https://bitbucket.org/ecwolf/ecwolf/src/default/ I still need to fix the docker script but you can see how my normal build environment is setup here (add SDK 26 per new requirements of course): https://bitbucket.org/ecwolf/ecwolf/src/69309ebf5b7cb8356f8207a40c2b460556636f5e/docker.sh#lines-396 As I said in my last comment, I discovered this since when I ran CMake manually I specified the wrong arch and didn't realize it. Just to re-answer: SDL_atomic.c.arm means the file SDL_atomic.c is compiled in ARM (and not thumb). (when it makes sense, eg. not x86,). As you notice in the first message: https://hg.libsdl.org/SDL/file/9d3f245739f8/include/SDL_atomic.h#l171 https://hg.libsdl.org/SDL/file/9d3f245739f8/src/atomic/SDL_atomic.c#l295 we have the define: #define SDL_MemoryBarrierRelease() SDL_MemoryBarrierReleaseFunction() Then the function void SDL_MemoryBarrierReleaseFunction(void) { SDL_MemoryBarrierRelease(); } So it's defined as itself and do an infinite recursion call. Maybe, it's a mistake in the commit https://hg.libsdl.org/SDL/rev/773cbb61ad27 Thanks Sylvain, I'll take this for 2.0.10. Patch added here: https://hg.libsdl.org/SDL/rev/12a6ba543f44 The real problem is that SDL_atomic.c was built in thumb mode instead of ARM mode, which is required to use the mcr instruction on ARM platforms. Added a compiler error to catch this case so we don't generate code that does infinite recursion. I also added a potentially better way to handle things on Linux ARM platforms, based on comments in the Chromium headers, which we can try out after 2.0.10 ships. |