Refactor atomic operations for Android compatibility

[Duron27]

Updated atomic operations to use std::atomic for Android platform.

This removes a good chunk of one of our patches HERE

Even though some of these look like we should fall under the "else" with the same return as in the android statement we don't. I've build for android not using these changes and the game is very unstable.

[AnyOldName3]

I don't think this is quite the right approach if we're upstreaming this. Just like there are a bunch of existing options like _OPENTHREADS_ATOMIC_USE_GCC_BUILTINS and _OPENTHREADS_ATOMIC_USE_MUTEX, it would be sensible to have a new one called something like _OPENTHREADS_ATOMIC_USE_STD_ATOMIC, and then have something in the CMake that sets it for Android (or potentially any platform if the source is being compiled as at least C++11 - it might be faster than the existing Win32 interlocked implementation as under the hood it should be the same, but it'll be inlinable if it's all done in the header).

However, I'm not convinced that any C++ changes should be required here at all. The _OPENTHREADS_ATOMIC_USE_GCC_BUILTINS should work fine on Android as all the GCC builtins are also available with Clang. It just looks like the existing configuration machinery is dumb. Instead of checking that various test snippets in CheckAtomicOps.cmake compile, which would be enough, it checks if they compile and run, which doesn't support cross-compilation unless you've set up CMAKE_CROSSCOMPILING_EMULATOR, which didn't exist yet in the version of CMake that upstream OSG uses. Because of that, it's got to avoid running that script on Android, and so no atomic support is detected, and then instead of sensibly falling back to mutexes, it falls back to just pretending a volatile unsigned is going to be atomic.

The minimal work to get something mergable would be to:

• Change the checks in CheckAtomicOps.cmake from CHECK_CXX_SOURCE_RUNS to check_cxx_source_compiles.
• Get rid of the IF(NOT ANDROID) in src/OpenThreads/CMakeLists.txt before the INCLUDE(CheckAtomicOps) line so it's run for all platforms including Android.
• Check whether it works.

For bonus points, you could also:

• Add an extra check to CheckAtomicOps.cmake that detects whether std::atomic stuff works, and sets _OPENTHREADS_ATOMIC_USE_STD_ATOMIC.
• Make sure that it doesn't enable two variants on any platform like the existing bit at the end of that script that deals with GCC on Windows having both GCC builtins and Win32 functions available.
• Make sure that everywhere that propagates these variables, like src/OpenThreads/common/Config.in is also made aware of the new variable.
• Put the C++ changes that the current version of this PR has behind a _OPENTHREADS_ATOMIC_USE_STD_ATOMIC check rather than an Andoird check.

[AnyOldName3]

You're not defining _OPENTHREADS_ATOMIC_USE_LIBRARY_ROUTINES when _OPENTHREADS_ATOMIC_USE_STD_ATOMIC is active, so none of this file is seen by the compiler when you're using the new mode, and you therefore don't need to have any implementation here for the _OPENTHREADS_ATOMIC_USE_STD_ATOMIC approach. You only need the parts of the implementation in include/OpenThreads/Atomic and not the parts on src/OpenThreads/Atomic.cpp.

[Duron27]

I reversed the changes to Atomic.cpp, this closes the other MR I submitted also.

according to our patches there's another small change in two files shown in this patch.
RemainingAtomic.patch

I'm still trying to figure out a suitable way to do this. Maybe something like?

#if defined(_OPENTHREADS_ATOMIC_USE_STD_ATOMIC) std::atomic<unsigned> _numClients; #else unsigned int _numClients; #endif

[AnyOldName3]

If those values need to be atomic, then it would stand to reason that they'd need to be atomic on all platforms. That might be a bit of a pain to test, though, as on x86, aligned reads and writes under eight bytes are always atomic (although with relaxed ordering), so it might not be a symptomatic problem on desktop machines.

If we can confirm it's a real cross-platform issue, then the solution is to use an OpenTheads atomic instead of an STL one.

[Duron27]

The only thing I can think of is to build for android without atomic changes and hope to catch a dump from the crash. Would this be a helpful test?

[AnyOldName3]

Maybe, although it might be more helpful to build with tsan and see if it yells at us about those variables.

[Duron27]

So I'll build everything with -fsanitize=thread in the build and linker flags and see what comes up. Would I be applying this MR patch or building osg without?