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

Unwinding intrinsics use non-unwinding ABI #104451

Closed
tmiasko opened this issue Nov 15, 2022 · 9 comments · Fixed by #110233
Closed

Unwinding intrinsics use non-unwinding ABI #104451

tmiasko opened this issue Nov 15, 2022 · 9 comments · Fixed by #110233
Assignees
Labels
A-intrinsics Area: Intrinsics A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness

Comments

@tmiasko
Copy link
Contributor

tmiasko commented Nov 15, 2022

There are intrinsics like assert_inhabited, which can unwind, but use rust-intrinsic ABI that never unwinds:

| RustIntrinsic
| PlatformIntrinsic
| Unadjusted => false,

An example demonstrating the issue. The argument passed to function f is never dropped, when assert_inhabited unwinds:

#![feature(core_intrinsics)]
#![feature(never_type)]

struct NoisyDrop(&'static str);

impl Drop for NoisyDrop {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

pub fn f<A, B>(_: A) {
    core::intrinsics::assert_inhabited::<B>()
}

fn main() {
    let _a = NoisyDrop("1");
    f::<NoisyDrop, !>(NoisyDrop("2"));
}

$ rustc a.rs && ./a
thread 'main' panicked at 'attempted to instantiate uninhabited type `!`', a.rs:13:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
1
@tmiasko tmiasko added the C-bug Category: This is a bug. label Nov 15, 2022
@nbdd0121
Copy link
Contributor

I'll work on this after #102906 is merged.

@rustbot claim

@RalfJung
Copy link
Member

Are these intrinsics the only ones that unwind? Maybe we should just make them abort instead by calling the (fairly recent) panic_str_nounwind.

However, AFAIK try is also an intrinsic and that can unwind?

@nbdd0121
Copy link
Contributor

try catches unwind and it does not itself unwind.

@nbdd0121
Copy link
Contributor

My current idea is to change assert_inhabited to is_inhabited intrinsic, and then have a wrapper that checks the value and panic if true. But making them abort sounds like an easier alternative.

@RalfJung
Copy link
Member

It should be a nice abort via panic_str_nounwind though.

I am honestly not sure why the intrinsics are panicking rather than returning a boolean... I think I tried that and it didn't work? But yeah bool-returning intrinsics sound better (less magic) ayway.

@RalfJung
Copy link
Member

One possible reason is that in CTFE and Miri, these intrinsics will actually abort instead of panic. Getting the if ... { panic_str_nounwind } to work in CTFE will be a bit of work (extending the existing hacks that make regular panicking work in CTFE, or using const_eval_select) and the backtrace will be slightly worse in Miri.

So given that we want something different implementations for Miri, CTFE, and runtime, maybe going with 'call panic_str_nounwind instead' is the better option. (An immediate-abort-panic just didn't exist when I added these intrinsics, otherwise this is probably what I would have done from the start.)

@workingjubilee workingjubilee added A-intrinsics Area: Intrinsics A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows labels Mar 11, 2023
@RalfJung
Copy link
Member

With #105997, I think assert_inhabited and friends do now indeed never unwind.

But as determined in rust-lang/miri#2839, we still have const_eval_select, which very much can unwind.

@nbdd0121
Copy link
Contributor

Yeah, I originally thought that we can remove all unwinding intrinsics, but unfortunately that's not true. I am working on a PR to change the ABI to be unwindable and stick #[rustc_nounwind] to most intrinsics.

@RalfJung
Copy link
Member

That sounds like the cleanest approach, thanks!

@saethlin saethlin added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Apr 12, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 12, 2023
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 12, 2023
@bors bors closed this as completed in e85ecbb Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intrinsics Area: Intrinsics A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants