-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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 abort message #81469
android: set abort message #81469
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
0bd3e9c
to
d308e0c
Compare
This comment has been minimized.
This comment has been minimized.
d308e0c
to
db0dc39
Compare
This comment has been minimized.
This comment has been minimized.
Also, it might not be a great idea to have the panic_abort crate do any allocations. On all platforms, this panic handler does nothing other than just one (sys)call or instruction to abort the program. Not pulling in any other code is one of the reasons for using the panic_abort crate. |
Sounds good, I can send a change for that as well. Would something similar to #45580 be sufficient?
I agree, although I don't think I can avoid the allocation here. I can mark the dependency on alloc as specific to |
A PR to raise it to level 21 was submitted a few months ago, but not merged: #78601
That wouldn't make much of a difference. If Is it common to use |
Thanks, I understand that such API update is unlikely to happen for now. For future reference, I'd still like to discuss the other points.
The issue with the current definition of
Using abort is the standard on Android to terminate a native process on error (see https://source.android.com/devices/tech/debug/native-crash#abort). More specifically, in C/C++ a log with the |
Some more thoughts on the approach: It is possible to move some of the pre-processing in There is also an issue for @m-ou-se Would you have any feedback on this approach? I can send another PR but I want to make sure this design is likely to be accepted first. Thanks. |
I've looked a bit deeper into all the panic logic, and I think it's perfectly fine to have panic_abort depend on alloc. I would still want to avoid allocating anything if possible, so ideally the implementation should use If you can implement it in such a way that allocation failures will not cause another panic, but just silently cause it to skip setting the abort message (or set it to a default message or something), I feel comfortable moving forward with this. As for the minimum Android API level problem: we should be able to use weak linkage to |
db0dc39
to
a23976d
Compare
This comment has been minimized.
This comment has been minimized.
a23976d
to
e35ca05
Compare
Done. Please take a look. Thanks. |
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.
Looks good. Did you test it?
Two small comments:
e35ca05
to
f41d0a4
Compare
Thanks for the review. I tested it in-tree using Android build system (Soong). I just tested using |
Thanks! @bors r+ |
📌 Commit f41d0a4 has been approved by |
…ou-se android: set abort message Android has the ability to supply an abort message [1]. This message is automatically included in the debug trace, which helps debugging [2]. Modify panic_abort to populate this message before calling abort(). [1] https://android.googlesource.com/platform/bionic/+/master/libc/include/android/set_abort_message.h [2] https://source.android.com/devices/tech/debug/native-crash
…ou-se android: set abort message Android has the ability to supply an abort message [1]. This message is automatically included in the debug trace, which helps debugging [2]. Modify panic_abort to populate this message before calling abort(). [1] https://android.googlesource.com/platform/bionic/+/master/libc/include/android/set_abort_message.h [2] https://source.android.com/devices/tech/debug/native-crash
The CI error does not contain much details. I managed to reproduce it locally and here is the error I got from llvm:
I initially thought this was related to one of the casting I made, but by commenting out the code, I found out that this was caused by the call to pub(crate) unsafe fn android_set_abort_message(payload: *mut &mut dyn BoxMeUp) {
let mut v = Vec::<u8>::new();
if v.try_reserve(1).is_err() {
return;
}
} |
f41d0a4
to
fa91225
Compare
The use of |
fa91225
to
fda75e6
Compare
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.
Mostly LGTM, but not an android expert and I am not sure if the new alloc dependency of panic_abort isn't going to cause issues in the future.
Maybe using some platform specific dependencies in the Cargo.toml, like this: [target.'cfg(target_os = "android")'.dependencies]
alloc = { path = "../alloc" } could be enough, so that the |
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.
Thanks! I have one small comment, but otherwise r=me:
|
fda75e6
to
751c298
Compare
This comment has been minimized.
This comment has been minimized.
Android has the ability to supply an abort message [1]. This message is automatically included in the debug trace, which helps debugging [2]. Modify panic_abort to populate this message before calling abort(). [1] https://android.googlesource.com/platform/bionic/+/master/libc/include/android/set_abort_message.h [2] https://source.android.com/devices/tech/debug/native-crash
751c298
to
52ee9fb
Compare
Thanks! @bors r+ |
📌 Commit 52ee9fb has been approved by |
☀️ Test successful - checks-actions |
Pkgsrc changes: * Bump bootstrap requirements to 1.52.1 * Adjust patches, adapt to upstream changes, adjust cargo checksums * If using an external llvm, require >= 10.0 Upsteream changes: Version 1.53.0 (2021-06-17) ============================ Language ----------------------- - [You can now use unicode for identifiers.][83799] This allows multilingual identifiers but still doesn't allow glyphs that are not considered characters such as `#` (diamond) or `<U+1F980>` (crab). More specifically you can now use any identifier that matches the UAX #31 "Unicode Identifier and Pattern Syntax" standard. This is the same standard as languages like Python, however Rust uses NFC normalization which may be different from other languages. - [You can now specify "or patterns" inside pattern matches.][79278] Previously you could only use `|` (OR) on complete patterns. E.g. ```rust let x = Some(2u8); // Before matches!(x, Some(1) | Some(2)); // Now matches!(x, Some(1 | 2)); ``` - [Added the `:pat_param` `macro_rules!` matcher.][83386] This matcher has the same semantics as the `:pat` matcher. This is to allow `:pat` to change semantics to being a pattern fragment in a future edition. Compiler ----------------------- - [Updated the minimum external LLVM version to LLVM 10.][83387] - [Added Tier 3\* support for the `wasm64-unknown-unknown` target.][80525] - [Improved debuginfo for closures and async functions on Windows MSVC.][83941] \* Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. Libraries ----------------------- - [Abort messages will now forward to `android_set_abort_message` on Android platforms when available.][81469] - [`slice::IterMut<'_, T>` now implements `AsRef<[T]>`][82771] - [Arrays of any length now implement `IntoIterator`.][84147] Currently calling `.into_iter()` as a method on an array will return `impl Iterator<Item=&T>`, but this may change in a future edition to change `Item` to `T`. Calling `IntoIterator::into_iter` directly on arrays will provide `impl Iterator<Item=T>` as expected. - [`leading_zeros`, and `trailing_zeros` are now available on all `NonZero` integer types.][84082] - [`{f32, f64}::from_str` now parse and print special values (`NaN`, `-0`) according to IEEE RFC 754.][78618] - [You can now index into slices using `(Bound<usize>, Bound<usize>)`.][77704] - [Add the `BITS` associated constant to all numeric types.][82565] Stabilised APIs --------------- - [`AtomicBool::fetch_update`] - [`AtomicPtr::fetch_update`] - [`BTreeMap::retain`] - [`BTreeSet::retain`] - [`BufReader::seek_relative`] - [`DebugStruct::non_exhaustive`] - [`Duration::MAX`] - [`Duration::ZERO`] - [`Duration::is_zero`] - [`Duration::saturating_add`] - [`Duration::saturating_mul`] - [`Duration::saturating_sub`] - [`ErrorKind::Unsupported`] - [`Option::insert`] - [`Ordering::is_eq`] - [`Ordering::is_ge`] - [`Ordering::is_gt`] - [`Ordering::is_le`] - [`Ordering::is_lt`] - [`Ordering::is_ne`] - [`OsStr::is_ascii`] - [`OsStr::make_ascii_lowercase`] - [`OsStr::make_ascii_uppercase`] - [`OsStr::to_ascii_lowercase`] - [`OsStr::to_ascii_uppercase`] - [`Peekable::peek_mut`] - [`Rc::decrement_strong_count`] - [`Rc::increment_strong_count`] - [`Vec::extend_from_within`] - [`array::from_mut`] - [`array::from_ref`] - [`char::MAX`] - [`char::REPLACEMENT_CHARACTER`] - [`char::UNICODE_VERSION`] - [`char::decode_utf16`] - [`char::from_digit`] - [`char::from_u32_unchecked`] - [`char::from_u32`] - [`cmp::max_by_key`] - [`cmp::max_by`] - [`cmp::min_by_key`] - [`cmp::min_by`] - [`f32::is_subnormal`] - [`f64::is_subnormal`] Cargo ----------------------- - [Cargo now supports git repositories where the default `HEAD` branch is not "master".][cargo/9392] This also includes a switch to the version 3 `Cargo.lock` format which can handle default branches correctly. - [macOS targets now default to `unpacked` split-debuginfo.][cargo/9298] - [The `authors` field is no longer included in `Cargo.toml` for new projects.][cargo/9282] Rustdoc ----------------------- - [Added the `rustdoc::bare_urls` lint that warns when you have URLs without hyperlinks.][81764] Compatibility Notes ------------------- - [Implement token-based handling of attributes during expansion][82608] - [`Ipv4::from_str` will now reject octal format IP addresses in addition to rejecting hexadecimal IP addresses.][83652] The octal format can lead to confusion and potential security vulnerabilities and [is no longer recommended][ietf6943]. Internal Only ------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools. - [Rework the `std::sys::windows::alloc` implementation.][83065] - [rustdoc: Don't enter an infer_ctxt in get_blanket_impls for impls that aren't blanket impls.][82864] - [rustdoc: Only look at blanket impls in `get_blanket_impls`][83681] - [Rework rustdoc const type][82873] [83386]: rust-lang/rust#83386 [82771]: rust-lang/rust#82771 [84147]: rust-lang/rust#84147 [84082]: rust-lang/rust#84082 [83799]: rust-lang/rust#83799 [83681]: rust-lang/rust#83681 [83652]: rust-lang/rust#83652 [83387]: rust-lang/rust#83387 [82873]: rust-lang/rust#82873 [82864]: rust-lang/rust#82864 [82608]: rust-lang/rust#82608 [82565]: rust-lang/rust#82565 [80525]: rust-lang/rust#80525 [79278]: rust-lang/rust#79278 [78618]: rust-lang/rust#78618 [77704]: rust-lang/rust#77704 [83941]: rust-lang/rust#83941 [83065]: rust-lang/rust#83065 [81764]: rust-lang/rust#81764 [81469]: rust-lang/rust#81469 [cargo/9298]: rust-lang/cargo#9298 [cargo/9282]: rust-lang/cargo#9282 [cargo/9392]: rust-lang/cargo#9392 [`char::MAX`]: https://doc.rust-lang.org/std/primitive.char.html#associatedconstant.MAX [`char::REPLACEMENT_CHARACTER`]: https://doc.rust-lang.org/std/primitive.char.html#associatedconstant.REPLACEMENT_CHARACTER [`char::UNICODE_VERSION`]: https://doc.rust-lang.org/std/primitive.char.html#associatedconstant.UNICODE_VERSION [`char::decode_utf16`]: https://doc.rust-lang.org/std/primitive.char.html#method.decode_utf16 [`char::from_u32`]: https://doc.rust-lang.org/std/primitive.char.html#method.from_u32 [`char::from_u32_unchecked`]: https://doc.rust-lang.org/std/primitive.char.html#method.from_u32_unchecked [`char::from_digit`]: https://doc.rust-lang.org/std/primitive.char.html#method.from_digit [`AtomicBool::fetch_update`]: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicBool.html#method.fetch_update [`AtomicPtr::fetch_update`]: https://doc.rust-lang.org/std/sync/atomic/struct.AtomicPtr.html#method.fetch_update [`BTreeMap::retain`]: https://doc.rust-lang.org/std/collections/struct.BTreeMap.html#method.retain [`BTreeSet::retain`]: https://doc.rust-lang.org/std/collections/struct.BTreeSet.html#method.retain [`BufReader::seek_relative`]: https://doc.rust-lang.org/std/io/struct.BufReader.html#method.seek_relative [`DebugStruct::non_exhaustive`]: https://doc.rust-lang.org/std/fmt/struct.DebugStruct.html#method.finish_non_exhaustive [`Duration::MAX`]: https://doc.rust-lang.org/std/time/struct.Duration.html#associatedconstant.MAX [`Duration::ZERO`]: https://doc.rust-lang.org/std/time/struct.Duration.html#associatedconstant.ZERO [`Duration::is_zero`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.is_zero [`Duration::saturating_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.saturating_add [`Duration::saturating_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.saturating_mul [`Duration::saturating_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.saturating_sub [`ErrorKind::Unsupported`]: https://doc.rust-lang.org/std/io/enum.ErrorKind.html#variant.Unsupported [`Option::insert`]: https://doc.rust-lang.org/std/option/enum.Option.html#method.insert [`Ordering::is_eq`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_eq [`Ordering::is_ge`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_ge [`Ordering::is_gt`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_gt [`Ordering::is_le`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_le [`Ordering::is_lt`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_lt [`Ordering::is_ne`]: https://doc.rust-lang.org/std/cmp/enum.Ordering.html#method.is_ne [`OsStr::eq_ignore_ascii_case`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.eq_ignore_ascii_case [`OsStr::is_ascii`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.is_ascii [`OsStr::make_ascii_lowercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.make_ascii_lowercase [`OsStr::make_ascii_uppercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.make_ascii_uppercase [`OsStr::to_ascii_lowercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.to_ascii_lowercase [`OsStr::to_ascii_uppercase`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html#method.to_ascii_uppercase [`Peekable::peek_mut`]: https://doc.rust-lang.org/std/iter/struct.Peekable.html#method.peek_mut [`Rc::decrement_strong_count`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.increment_strong_count [`Rc::increment_strong_count`]: https://doc.rust-lang.org/std/rc/struct.Rc.html#method.increment_strong_count [`Vec::extend_from_within`]: https://doc.rust-lang.org/beta/std/vec/struct.Vec.html#method.extend_from_within [`array::from_mut`]: https://doc.rust-lang.org/beta/std/array/fn.from_mut.html [`array::from_ref`]: https://doc.rust-lang.org/beta/std/array/fn.from_ref.html [`cmp::max_by_key`]: https://doc.rust-lang.org/beta/std/cmp/fn.max_by_key.html [`cmp::max_by`]: https://doc.rust-lang.org/beta/std/cmp/fn.max_by.html [`cmp::min_by_key`]: https://doc.rust-lang.org/beta/std/cmp/fn.min_by_key.html [`cmp::min_by`]: https://doc.rust-lang.org/beta/std/cmp/fn.min_by.html [`f32::is_subnormal`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_subnormal [`f64::is_subnormal`]: https://doc.rust-lang.org/std/primitive.f64.html#method.is_subnormal [ietf6943]: https://datatracker.ietf.org/doc/html/rfc6943#section-3.1.1
Use existing declaration of rust_eh_personality If crate declares `rust_eh_personality`, re-use existing declaration as otherwise attempts to set function attributes that follow the declaration will fail (unless it happens to have exactly the same type signature as the one predefined in the compiler). Fixes rust-lang#70117. Fixes rust-lang#81469 (comment); probably.
Android has the ability to supply an abort message [1]. This message is
automatically included in the debug trace, which helps debugging [2].
Modify panic_abort to populate this message before calling abort().
[1] https://android.googlesource.com/platform/bionic/+/master/libc/include/android/set_abort_message.h
[2] https://source.android.com/devices/tech/debug/native-crash