android: Enable tls_model("local_dynamic") for NDK-less builds on LLVM >= 17
#349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #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-android29may be invoked without ever including an NDK header that sets__NDK_MAJOR__ 26or higher (noting that LLVM 17 is used since r26): https://togithub.com/mjansson/rpmalloc/pull/347#discussion_r1912096361Instead, check if the
__clang_major__define is17or higher, but also keep the original comparison in place just in case even if it's superfluous. Also remove unnecessarydefined(XXX)checks, since the C specification clarifies that undefined tokens are replaced with0which aptly turn the>=intofalse.