Adding support for Android ndk 26 change #347
Conversation
| @@ -548,6 +548,12 @@ static size_t os_page_size; | |||
| #if defined(_MSC_VER) && !defined(__clang__) | |||
| #define TLS_MODEL | |||
| #define _Thread_local __declspec(thread) | |||
| #elif defined(__ANDROID__) | |||
| #if __ANDROID_API__ >= 29 && defined(__NDK_MAJOR__) && __NDK_MAJOR__ >= 26 | |||
There was a problem hiding this comment.
@TroutZhang can you explain what you're using __NDK_MAJOR__ for here, to check against a specific set of headers (unikely) or the shipped version of the toolchain?
I'm asking because xbuild for example uses a stripped down NDK while relying on the cross-compiling capabilities of the host clang/LLVM installation. You're better off checking against __clang_major__ >= 17 in that case?
For xbuild that likely means this always defines an empty TLS_MODEL, which at least works for us.
There was a problem hiding this comment.
Tested and:
xbuilddoes not set__NDK_MAJOR__;- On
__clang_major__ 18, usinglocal-dynamicalso works, besides defining this empty.
What do you think about adding or replacing a clause for __clang_major__ >= 17 as proposed above?
| #define TLS_MODEL __attribute__((tls_model("local-dynamic"))) | ||
| #else | ||
| #define TLS_MODEL | ||
| #endif | ||
| #else | ||
| // #define TLS_MODEL __attribute__((tls_model("initial-exec"))) |
There was a problem hiding this comment.
Just noticing that this was probably temporarily commented out to test Android, but it has fortunately already been restored in 2084420.
…LVM >= 17 PR mjansson#347 fixed/removed using the `"initial-exec"` TLS model for Android which is no longer compatible since LLVM 17 where ELF TLS is enabled by default for API level 29. It opted to instead enable `"local-dynamic"`, but only if the minimum API level is 29 _and_ the NDK is version 26 or higher, and otherwise doesn't define a TLS model at all. However, this did not consider simple build cases where e.g. `clang --target aarch64-linux-android29` may be invoked without ever including an NDK header that sets `__NDK_MAJOR__ 26` or higher (noting that LLVM 17 is used since r26): https://togithub.com/mjansson/rpmalloc/pull/347#discussion_r1912096361 Instead, check if the `__clang_major__` define is `17` or higher, but also keep the original comparison in place just in case even if it's superfluous. Also remove unnecessary `defined(XXX)` checks, since the C specification clarifies that undefined tokens are replaced with `0` which aptly turn the `>=` into `false`.
check here: https://github.com/android/ndk/wiki/Changelog-r26#changes
Issue 1679: Clang will now automatically enable ELF TLS for minSdkVersion 29 or higher.