We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4683

Summary: SDL_atomic infinite recursion on armv6/armv5 w/ thumb
Product: SDL Reporter: Braden Obrzut <admin>
Component: atomicAssignee: 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
Spent a decent amount of time trying to figure out why my application was crashing after upgrading beyond 2.0.5.  Turns out that for some reason I was compiling armv5 and reaching this branch:

https://hg.libsdl.org/SDL/file/9d3f245739f8/include/SDL_atomic.h#l171

As we see in this case it's supposed to call a "real function" which is turn is just a call to SDL_MemoryBarrierRelease which of course gets preprocessed to itself here:

https://hg.libsdl.org/SDL/file/9d3f245739f8/src/atomic/SDL_atomic.c#l295

End result is stack overflow and silent crash.  In the changeset that introduced this I see a reference to an SDL_atomic.c.arm in the Android project, but I don't know what that is.  I'm using CMake with some changes to the build scripts allow me to uni-build my whole project.
Comment 1 Sylvain 2019-06-22 06:43:02 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) ?
Comment 2 Braden Obrzut 2019-06-22 12:17:49 UTC
I'm using ndk r14b (which does still work even though I see reports of 1
Comment 3 Braden Obrzut 2019-06-22 12:23:34 UTC
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.
Comment 4 Sylvain 2019-06-22 12:46:25 UTC
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 ?
Comment 5 Braden Obrzut 2019-06-22 13:22:44 UTC
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.
Comment 6 Sylvain 2019-06-24 09:02:42 UTC
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
Comment 8 Sam Lantinga 2019-07-01 06:00:46 UTC
Thanks Sylvain, I'll take this for 2.0.10.
Comment 9 Sam Lantinga 2019-07-01 06:27:14 UTC
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.