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: Set SDK/API level via version-suffxed --target triple #24

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Jan 7, 2025

Fixes https://github.com/Traverse-Research/evolve/issues/1058. Our version of rust-mobile#209, see that PR for all details.

* Initial support for x86_64-linux-android targets.

* Improve target selection for clang link phase.

* Only add clang target if it differs from the host.
@MarijnS95 MarijnS95 requested a review from maxded January 7, 2025 16:01
@MarijnS95
Copy link
Member Author

This seems to work on small test-apps, but fails on our main app with:

01-07 17:39:59.432  1141  1141 E AndroidRuntime: java.lang.UnsatisfiedLinkError: dlopen failed: TLS symbol "(null)" in dlopened "/data/app/~~cHL8LBKb7eumhNxrtXw63g==/nl.traverse_research.evolve-u9FL5VS6IhAJMmQ9_7K5kg==/lib/arm64/libevolve_android.so" referenced from "/data/app/~~cHL8LBKb7eumhNxrtXw63g==/nl.traverse_research.evolve-u9FL5VS6IhAJMmQ9_7K5kg==/lib/arm64/libevolve_android.so" using IE access model

I did not run into that before testing, perhaps because I upgraded the backtrace crate before? Testing ongoing.

@MarijnS95
Copy link
Member Author

A lot of chasing later, I got this figured out. Wasn't going to settle for a -D__ANDROID_API__={min_sdk_ver} hack.

I couldn't initially reproduce this with cargo-apk because I had a too-old NDK installed (26.3.11579264). After upgrading to 27.2, the issue reproduces there as well.

Searching for it, it's supposedly an intended change where ELF TLS is automatically enabled for SDK level >=29 in upstream LLVM and also in NDK r26b (I was supposedly on r26a): android/ndk#1679. In xbuild, we weren't telling clang what min API version to compile for.

Via golang/go#69109 (comment) we also see that the STATIC_TLS flag is added to our binary. Turns out that's not because of a compiler flag (or lack thereof), but because the rpmalloc crate sets a static TLS model which results in this error: mjansson/rpmalloc#332 / mjansson/rpmalloc#347.

I don't know if we can opt-out of this with a compiler flag, but there are a few things we can do:

  1. git reference the linked rpmalloc PR to fix this;
  2. Disable rpmalloc for Android (we also disabled it for MacOS due to similar strange issues);
  3. Keep not telling clang about our API version, and create a bigger gap with the Android ecosystem by hacking it with -D__ANDROID_API__={min_sdk_ver} instead 🤮

@maxded
Copy link

maxded commented Jan 8, 2025

I would be fine with the git reference fix so that we can move on and actually get further into our certification process. Would be good to also share this information with the xbuild issue to get some outside eyes on it (if anyone even does that 👀 ).

Disable rpmalloc for Android (we also disabled it for MacOS due to similar strange issues);

I'm unsure what kind of impact this would have but could be an option as well, I'll leave the decision up to you.

@MarijnS95
Copy link
Member Author

@maxded thanks, that's the way I'm leaning too. Using that commit is quite a divergence so I might just cherry-pick it to an rpmalloc fork.

@MarijnS95
Copy link
Member Author

@maxded note that this repository is public (it is our fork of xbuild) and I can easily link or copy-paste the issue may anyone else observe it.

I'm only opening the PR here and on the upstream side because we have quite a divergent branch where none of the new features have been written/PRd against upstream. I would have to go through everything that was pushed and PR it upstream so that we can get rid of it, or at the very least rebase when all these fixes land upstream.

We haven't set the SDK/API level via the `__ANDROID_API__` define for
a very long time and so far got away with it.  However, while debugging
why `backtrace` (and by extension Rust `std` which reuses that crate)
wasn't generating symbolicated stacktraces in `panic_log`, and why
`findshlibs` wasn't providing the list of loaded libraries to `sentry`,
we found that both rely on expanding the `__ANDROID_API__` define
via compiling a small C file via `cc` to make the code conditional on
SDK/API >= 21:

gimli-rs/findshlibs#65
rust-lang/backtrace-rs#415

(It would have been lovely if these crates emitted a `cargo:warning`
when the define wasn't set at all, indicating an "incomplete"
cross-compiler setup, and/or looked at the runtime Android API version
via something like rust-mobile/ndk#479.)

Note that `backtrace 0.3.74` / Rust 1.82
(rust-lang/rust@0763a3a) no longer rely on
this because Rust 1.82 bumped the minimum SDK/API level to 21:
rust-lang/backtrace-rs#656

We could set this define directly, or rely on `clang` to set it
for us by appending the SDK/API level to the target triple, of
the form `<arch>-linux-android<sdk level>`.  The latter is more
common. Keep in mind that the `cc` crate adds an unversioned
`--target=<arch>-linux-android` to the command line arguments as well,
but clang seems to deduplicate them (or look at the latter `--target`
which contains our version).

Note that this effectively reverts
rust-mobile@32efed6 because
we must now always pass the SDK level via the triple again, even if the
host also happens to be Android with the same architecture.
We don't use the template and its `wry` feature in this fork.
Remove it now that `ubuntu-latest` no longer has the necessary
packages which requires a tedious and complicated upgrade
of the `wry` "nonsense".  This is attempted upstream at:
https://togithub.com/rust-mobile/xbuild/pull/210
@MarijnS95 MarijnS95 force-pushed the for-tr-android-sdk-level branch from bb87df9 to 2df97d7 Compare January 9, 2025 13:11
@MarijnS95 MarijnS95 merged commit c6dd267 into for-traverse Jan 9, 2025
3 of 4 checks passed
@MarijnS95 MarijnS95 deleted the for-tr-android-sdk-level branch January 9, 2025 13:28
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.

3 participants