android: set abort message

[tweksteen]

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

[rust-highfive]

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.

[m-ou-se]

android_set_abort_message is only available in Android API level 21. In our tests we're testing against 14 as well. If we want to use this function, we should first raise our minimum supported Android API level.

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.

[tweksteen]

android_set_abort_message is only available in Android API level 21. In our tests we're testing against 14 as well. If we want to use this function, we should first raise our minimum supported Android API level.

Sounds good, I can send a change for that as well. Would something similar to #45580 be sufficient?

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.

I agree, although I don't think I can avoid the allocation here. I can mark the dependency on alloc as specific to target_os = "android" so there is no impact on other platforms?

[m-ou-se]

Sounds good, I can send a change for that as well. Would something similar to #45580 be sufficient?

A PR to raise it to level 21 was submitted a few months ago, but not merged: #78601

I agree, although I don't think I can avoid the allocation here. I can mark the dependency on alloc as specific to target_os = "android" so there is no impact on other platforms?

That wouldn't make much of a difference. If alloc is not referred to from the code, having it listed as dependency doesn't do much. Regardless, it's probably best to keep this panic handler a trivial abort on all platforms including Android.

Is it common to use panic = "abort" on Android? And if so, what are the most common reasons for doing so?

[tweksteen]

A PR to raise it to level 21 was submitted a few months ago, but not merged: #78601

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.

That wouldn't make much of a difference. If alloc is not referred to from the code, having it listed as dependency doesn't do much. Regardless, it's probably best to keep this panic handler a trivial abort on all platforms including Android.

The issue with the current definition of __rust_panic_start is that the payload argument cannot be used, unless one has some definition from alloc (for instance, to see if the payload can be downcasted to a String). Another option could be to update the definition of __rust_panic_start to pass the current payload as well as a &str of it (which can be used by low-level routine without trying to inspect/parse payload).

Is it common to use panic = "abort" on Android? And if so, what are the most common reasons for doing so?

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 FATAL level, will call abort (see https://developer.android.com/ndk/reference/group/logging). It seems natural to have a similar behaviour for panic in Rust.

[tweksteen]

Some more thoughts on the approach:

It is possible to move some of the pre-processing in std. For instance, panic_output could be modified for Android (similarly to SgxPanicOutput) to keep track of the formatted panic message. This would work for std (not no_std) and only if the default_hook is being used. It is not possible to call android_set_abort_message directly in std though because only the first call to this function will be used by libc. Since this panic could be caught (if the panic_runtime is unwind), this could be misleading (if a later abort do occur).

There is also an issue for panic_abort to actually consume that message: it cannot depend on std. The only option at hand seems to be to modify the arguments of __rust_panic_start to include a value that is somewhat usable by panic_abort. The main reasoning here is that payload is not useful at all for panic_abort currently.

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

[m-ou-se]

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 BoxMeUp::get() instead of BoxMeUp::take_box() to get to the payload, and not copy a &'static str into a new String. Unfortunately, that'll not get a zero terminated string like the android_set_abort_message wants. A failure to allocate here will result in another panic, repeatedly panicking until the stack overflows.

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 android_set_abort_message such that it's only used when available. The current state of weak linkage in Rust is not great though, so you'll have to do some experimentation to see what works.

[tweksteen]

Done. Please take a look. Thanks.

[m-ou-se]

Looks good. Did you test it?

Two small comments:

[tweksteen]

Thanks for the review. I tested it in-tree using Android build system (Soong). I just tested using x.py. I had to rearrange the feature attribute to build successfully here.

[m-ou-se]

Thanks!

@bors r+

[bors]

📌 Commit f41d0a4 has been approved by m-ou-se

[tweksteen]

The CI error does not contain much details. I managed to reproduce it locally and here is the error I got from llvm:

rustc: /checkout/src/llvm-project/llvm/include/llvm/Support/Casting.h:269: typename llvm::cast_retty<X, Y*>::ret_type llvm::cast(Y*) [with X = llvm::Function; Y = llvm::Value; typename llvm::cast_retty<X, Y*>::ret_type = llvm::Function*]: Assertion `isa<X>(Val) && "cast<Ty>() argument of incompatible type!"' failed.
[RUSTC-TIMING] panic_abort test:false 0.094
rustc exited with signal: 6
error: could not compile `panic_abort`

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 try_reserve. This can be reproduced with the function body:

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;
    }
}

[tweksteen]

The use of try_reserve requires the correct setup of rust_eh_personality, which is not the case for the panic_abort crate. Document this limitation and use malloc() instead, as recommended by @bjorn3.

[bjorn3]

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.

[Urgau]

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 alloc crate is only pull when building for android but @m-ou-se has already said that this might not be a good idea.

[m-ou-se]

Thanks! I have one small comment, but otherwise r=me:

[m-ou-se]

I am not sure if the new alloc dependency of panic_abort isn't going to cause issues in the future.

panic_abort is only used in combination with std, so it's safe to assume the alloc crate is available.

[m-ou-se]

Thanks!

@bors r+

[bors]

📌 Commit 52ee9fb has been approved by m-ou-se

[bors]

⌛ Testing commit 52ee9fb with merge 7953910...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 7953910 to master...