Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Android: Switch to native ELF TLS & latest LTS NDK, and enable shared druntime/Phobos #4618

Merged
merged 2 commits into from
Apr 19, 2024

Conversation

kinke
Copy link
Member

@kinke kinke commented Apr 13, 2024

Fixes #3918.

@kinke
Copy link
Member Author

kinke commented Apr 13, 2024

@s-ludwig @PetarKirov: Would you happen to be able and willing to check out the CI artifacts from https://github.com/ldc-developers/ldc/actions/runs/8676824079?pr=4618? E.g., if the bundled dub executable works fine to build some little app natively on Android, I'd expect things to be good (enough TLS coverage in dub and compiler itself).

If this works, we could most likely simply enable shared druntime+Phobos support for Android too, i.e., including the shared .so libs and allowing multiple D binaries in a single process, just like on vanilla Linux.

arch='${{ inputs.arch }}'
apiLevel='${{ inputs.android_api_level }}'
cmakeFlags="-DTARGET_SYSTEM='Android;Linux;UNIX'"
if [[ "$arch" == armv7a ]]; then
triple="$arch-linux-androideabi$apiLevel"
cmakeFlags+=' -DANDROID_ABI=armeabi-v7a'
elif [[ "$arch" == aarch64 ]]; then
# FIXME: as of NDK rc26d, libc.a has __tls_get_addr, but libc.so only since API level 30 (Android v11)
apiLevel=30
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird that the AArch64 libc.so got the required __tls_get_addr symbol in level 30, not level 29 as for armv7a; the NDK changelog doesn't mention this. Not sure whether this is an oversight in the NDK (it's like this for r26c and r26d), or whether there's a good reason why __tls_get_addr requires Android v11+ on AArch64 (unless linking libc statically).

@kinke kinke force-pushed the android_tls branch 4 times, most recently from 5815927 to 30d0447 Compare April 14, 2024 02:26
@s-ludwig
Copy link
Contributor

@s-ludwig @PetarKirov: Would you happen to be able and willing to check out the CI artifacts from https://github.com/ldc-developers/ldc/actions/runs/8676824079?pr=4618? E.g., if the bundled dub executable works fine to build some little app natively on Android, I'd expect things to be good (enough TLS coverage in dub and compiler itself).

If this works, we could most likely simply enable shared druntime+Phobos support for Android too, i.e., including the shared .so libs and allowing multiple D binaries in a single process, just like on vanilla Linux.

We've performed a quick test with a newly built runtime for NDK 25 and while it built successfully, for some reason it now hangs in a pthread_once call inside libcrypto:

Screenshot from 2024-04-14 12-52-09

@kinke
Copy link
Member Author

kinke commented Apr 14, 2024

THx for checking. So you haven't tested the prebuilt libs, but built them yourself? Using a prebuilt compiler here, or some other existing compiler? The former is required, as the main change is in the used LLVM (not visible in this diff here).

@kinke kinke changed the title Android: Switch to native ELF TLS & latest LTS NDK Android: Switch to native ELF TLS & latest LTS NDK, and enable shared druntime/Phobos Apr 14, 2024
@s-ludwig
Copy link
Contributor

I've used the compiler binary from the linux-x86_64 CI build, but generating an android runtime since none is included there. Should I try again using the libraries from the android-aarch64 build?

@kinke
Copy link
Member Author

kinke commented Apr 17, 2024

Should I try again using the libraries from the android-aarch64 build?

Might be worth a try, but I wouldn't expect wonders. Ideally with the same NDK r26d? - No idea how you built the libs exactly; I've dropped the -fvisibility=hidden here (for the shared libs) and use the level 29/30 clang for cross-compilation & -linking (and the same level for LDC's -mtriple).

Some TLS malfunction resulting in a hanging libcrypto init sounds a bit weird/unlucky though IMO...

@kinke
Copy link
Member Author

kinke commented Apr 19, 2024

Shall we get this in for beta1 to gather more feedback, reverting it for the final if problematic?

@s-ludwig
Copy link
Contributor

I've just tested again, this time with the latest CI artifacts, using the compiler binary from the linux-x86_64 artifact and the libraries from the android-aarch64 artifact, using NDK 26.3.11579264. Unfortunately, this results in exactly the same hang. I can try to generate a more minimal JNI library and specifically test TLS to hopefully narrow this down a bit.

@kinke
Copy link
Member Author

kinke commented Apr 19, 2024

Okay thx for testing & trying. How's that libcrypto built? I've had a extremely quick look into the repo, grepping for thread_local, but didn't find anything interesting - it looked like it uses manual libpthread-based TLS only, no native TLS. So that seemed fine, unlikely needing to be compiled against level 29 and possibly having to disable clang's emutls. Especially if this library works fine with the old manual TLS emulation.

@s-ludwig
Copy link
Contributor

Okay, basic TLS seems to work fine (setting an integer TLS variable, starting a setting/manipulating the same variable concurrently).

@s-ludwig
Copy link
Contributor

Turned out the libcrypto was still built against NDK 25 due to a CI misconfiguration. Rebuilt against NDK 26 and now works flawlessly!

@kinke
Copy link
Member Author

kinke commented Apr 19, 2024

Oh wow, awesome news, thx!

@kinke kinke merged commit 11908ed into ldc-developers:master Apr 19, 2024
22 of 23 checks passed
@kinke kinke deleted the android_tls branch April 19, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Android] Support the lld linker
2 participants