-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
fix: missing backtrace when using std::backtrace::Backtrace on Android #129305
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
library/std/build.rs
Outdated
// Used to detect the value of the `__ANDROID_API__` | ||
// builtin #define | ||
const MARKER: &str = "BACKTRACE_RS_ANDROID_APIVERSION"; | ||
const ANDROID_API_C: &str = " | ||
BACKTRACE_RS_ANDROID_APIVERSION __ANDROID_API__ | ||
"; | ||
|
||
// Create `android-api.c` on demand. | ||
let out_dir = env::var_os("OUT_DIR").unwrap(); | ||
let android_api_c = Path::new(&out_dir).join("android-api.c"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: could we leave a remark for why this is needed, and if possible link to some docs or references for this? Even if it's just what backtrace-rs does I think it'd still be useful. I'm guessing we're trying to detect minimum Android API version, but this isn't super obvious as to why this dance is needed.
r? @jieyouxu |
It seems to cause issues for projects building the standard library with non-cargo build systems. See #116705 |
I believe that since #120593, 21 is the minimum supported API level so we should be able to unconditionally enable the |
I assume that with rust-lang/backtrace-rs#656, this PR is no longer needed? |
It seems that it is not need. After this PR is merged, do we need to upgrade the |
Since rust-lang/rust#120593 the minimum android API level is 21. Related: https://github.com/android/ndk/wiki/Changelog-r26 rust-lang/rust#129305 1. stop setting `dl_iterate_phdr` based on whether the android API level is greater than or equal to 21, but retain it for back compat. 2. remove `cc` build dependency. 3. upgrade ndk r25b to r26d in CI.
5f9413f
to
1deb904
Compare
We can first set |
Since this is fixed upsteam, I'd prefer if we just upgraded directly to the latest version of backtrace-rs. Otherwise this hack is likely to get forgotten and stay for a long time. |
Closing since this is solved by updating backtrace-rs. |
This pr will fix #121033.
Cause of the problem
backtrace-rs
is introduced into the std lib in this way, which will cause the build.rs of backtrace-rs to not be executed, and thedl_iterate_phdr
feature will not be set. Ultimately, the stack information on android cannot be obtained.Solution
Since #120593, the minimum Android API level change from 19 to 21. And Android supports
dl_iterate_phdr
since API level 21. Please see https://android.googlesource.com/platform/bionic/+/HEAD/docs/status.md for details. So we can enabledl_iterate_phdr
feature for Android without checking the Android API level.