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

fix: missing backtrace when using std::backtrace::Backtrace on Android #129305

Closed
wants to merge 2 commits into from

Conversation

fengys1996
Copy link

@fengys1996 fengys1996 commented Aug 20, 2024

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 the dl_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 enable dl_iterate_phdr feature for Android without checking the Android API level.

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2024

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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot
Copy link
Collaborator

rustbot commented Aug 20, 2024

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2024
library/std/build.rs Outdated Show resolved Hide resolved
Comment on lines 191 to 200
// 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");
Copy link
Member

@jieyouxu jieyouxu Aug 21, 2024

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.

@Amanieu
Copy link
Member

Amanieu commented Aug 21, 2024

r? @jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned Amanieu Aug 21, 2024
@jieyouxu
Copy link
Member

@Amanieu I just left some drive-by reviews, I can't actually approve this, I'm not a libs reviewer^^

r? @Amanieu

@rustbot rustbot assigned Amanieu and unassigned jieyouxu Aug 22, 2024
@fengys1996
Copy link
Author

It seems to cause issues for projects building the standard library with non-cargo build systems. See #116705

@ollie27
Copy link
Member

ollie27 commented Aug 22, 2024

I believe that since #120593, 21 is the minimum supported API level so we should be able to unconditionally enable the dl_iterate_phdr feature.

@Amanieu
Copy link
Member

Amanieu commented Aug 23, 2024

I assume that with rust-lang/backtrace-rs#656, this PR is no longer needed?

@fengys1996
Copy link
Author

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 backtrace rs library in std?

workingjubilee added a commit to rust-lang/backtrace-rs that referenced this pull request Aug 23, 2024
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.
@fengys1996
Copy link
Author

We can first set dl_iterate_phdr feature unconditionally. Once bactrace-rs in std is upgraded, we can remove this logic.

@Amanieu
Copy link
Member

Amanieu commented Aug 24, 2024

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.

@Amanieu
Copy link
Member

Amanieu commented Sep 5, 2024

Closing since this is solved by updating backtrace-rs.

@Amanieu Amanieu closed this Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stdlib: Missing backtrace on panic when using std::backtrace::Backtrace vs backtrace::Backtrace on Android
5 participants