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 #209

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarijnS95
Copy link
Member

@MarijnS95 MarijnS95 commented Jan 7, 2025

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 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.

@MarijnS95 MarijnS95 requested review from dvc94ch and rib January 7, 2025 15:56
@MarijnS95
Copy link
Member Author

CC @jsclary for pretty much reverting your 32efed6, I don't expect it to cause any issues though as users are very unlikely to build from an Android host for an Android target (it doesn't even work until #138 is finished and merged).

@MarijnS95
Copy link
Member Author

Note that this change might cause an error like the following, if you use the rpmalloc crate (or any other code like it):

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

This will also affect cargo-apk if using a newer NDK (since r26b).

The change, issue and solution are:

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
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.
@MarijnS95 MarijnS95 force-pushed the android-set-sdk-level branch from c4d4f6e to 42648ec Compare January 9, 2025 13:11
MarijnS95 added a commit to MarijnS95/findshlibs that referenced this pull request Jan 9, 2025
Since Rust 1.82 the minimum Android API level is 21.  In
`backtrace`, this warranted the removal of the API version check via
`__ANDROID_API__` in rust-lang/backtrace-rs#656
since it is now always known to be >=21.  When `findshlibs` bumps its
MSRV the same could be done.

This change is sparked by a long search for why backtraces always had
`<unknown>` symbols on Android - and when solving that by upgrading Rust
(for `std::backtrace`) or our `backtrace` dependency, why Sentry reports
did not record what images/libraries were loaded at which offsets to
resolve stack addresses back to function symbols.  It turned out that
the `xbuild` build tool never set `__ANDROID_API__` (by not telling
`clang` about the target API level via the triple) which would cause
this code to never set `feature = "dl_iterate_phdr"` to search for
libraries on Android: rust-mobile/xbuild#209
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.

1 participant