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

Print a helpful message if unwinding aborts when it reaches a nounwind function #92828

Merged
merged 3 commits into from
Jan 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,28 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
helper.do_call(self, &mut bx, fn_abi, llfn, &args, None, cleanup);
}

fn codegen_abort_terminator(
&mut self,
helper: TerminatorCodegenHelper<'tcx>,
mut bx: Bx,
terminator: &mir::Terminator<'tcx>,
) {
let span = terminator.source_info.span;
self.set_debug_loc(&mut bx, terminator.source_info);

// Get the location information.
let location = self.get_caller_location(&mut bx, terminator.source_info).immediate();

// Obtain the panic entry point.
let def_id = common::langcall(bx.tcx(), Some(span), "", LangItem::PanicNoUnwind);
let instance = ty::Instance::mono(bx.tcx(), def_id);
let fn_abi = bx.fn_abi_of_instance(instance, ty::List::empty());
let llfn = bx.get_fn_addr(instance);

// Codegen the actual panic invoke/call.
helper.do_call(self, &mut bx, fn_abi, llfn, &[location], None, None);
}

/// Returns `true` if this is indeed a panic intrinsic and codegen is done.
fn codegen_panic_intrinsic(
&mut self,
Expand Down Expand Up @@ -1014,10 +1036,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::TerminatorKind::Resume => self.codegen_resume_terminator(helper, bx),

mir::TerminatorKind::Abort => {
bx.abort();
// `abort` does not terminate the block, so we still need to generate
// an `unreachable` terminator after it.
bx.unreachable();
self.codegen_abort_terminator(helper, bx, terminator);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update cg_clif too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cg_clif doesn't implement TerminatorKind::Abort (it's only used in landing pads).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it does implement it, it seems it is indeed unreachable until unwinding is implemented. Could you add a fixme so I don't forget to implement it once I implement unwinding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

            TerminatorKind::Resume | TerminatorKind::Abort => {
                trap_unreachable(fx, "[corruption] Unwinding bb reached.");
            }

I think it's already pretty clear that both are needed for unwinding support.

}

mir::TerminatorKind::Goto { target } => {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_hir/src/lang_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ language_item_table! {
PanicInfo, sym::panic_info, panic_info, Target::Struct, GenericRequirement::None;
PanicLocation, sym::panic_location, panic_location, Target::Struct, GenericRequirement::None;
PanicImpl, sym::panic_impl, panic_impl, Target::Fn, GenericRequirement::None;
PanicNoUnwind, sym::panic_no_unwind, panic_no_unwind, Target::Fn, GenericRequirement::Exact(0);
/// libstd panic entry point. Necessary for const eval to be able to catch it
BeginPanic, sym::begin_panic, begin_panic_fn, Target::Fn, GenericRequirement::None;

Expand Down
10 changes: 9 additions & 1 deletion compiler/rustc_monomorphize/src/collector.rs
Original file line number Diff line number Diff line change
Expand Up @@ -807,10 +807,18 @@ impl<'a, 'tcx> MirVisitor<'tcx> for MirNeighborCollector<'a, 'tcx> {
self.output.push(create_fn_mono_item(tcx, instance, source));
}
}
mir::TerminatorKind::Abort { .. } => {
let instance = Instance::mono(
tcx,
tcx.require_lang_item(LangItem::PanicNoUnwind, Some(source)),
);
if should_codegen_locally(tcx, &instance) {
self.output.push(create_fn_mono_item(tcx, instance, source));
}
}
mir::TerminatorKind::Goto { .. }
| mir::TerminatorKind::SwitchInt { .. }
| mir::TerminatorKind::Resume
| mir::TerminatorKind::Abort
| mir::TerminatorKind::Return
| mir::TerminatorKind::Unreachable => {}
mir::TerminatorKind::GeneratorDrop
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,7 @@ symbols! {
panic_implementation,
panic_info,
panic_location,
panic_no_unwind,
panic_runtime,
panic_str,
panic_unwind,
Expand Down
16 changes: 15 additions & 1 deletion library/core/src/panic/panic_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ pub struct PanicInfo<'a> {
payload: &'a (dyn Any + Send),
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
can_unwind: bool,
}

impl<'a> PanicInfo<'a> {
Expand All @@ -44,9 +45,10 @@ impl<'a> PanicInfo<'a> {
pub fn internal_constructor(
message: Option<&'a fmt::Arguments<'a>>,
location: &'a Location<'a>,
can_unwind: bool,
) -> Self {
struct NoPayload;
PanicInfo { location, message, payload: &NoPayload }
PanicInfo { location, message, payload: &NoPayload, can_unwind }
}

#[unstable(
Expand Down Expand Up @@ -127,6 +129,18 @@ impl<'a> PanicInfo<'a> {
// deal with that case in std::panicking::default_hook and core::panicking::panic_fmt.
Some(&self.location)
}

/// Returns whether the panic handler is allowed to unwind the stack from
/// the point where the panic occurred.
///
/// This is true for most kinds of panics with the exception of panics
/// caused by trying to unwind out of a `Drop` implementation or a function
/// whose ABI does not support unwinding.
#[must_use]
#[unstable(feature = "panic_can_unwind", issue = "92988")]
pub fn can_unwind(&self) -> bool {
self.can_unwind
}
}

#[stable(feature = "panic_hook_display", since = "1.26.0")]
Expand Down
27 changes: 26 additions & 1 deletion library/core/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,31 @@ fn panic_bounds_check(index: usize, len: usize) -> ! {
panic!("index out of bounds: the len is {} but the index is {}", len, index)
}

#[cfg(not(bootstrap))]
#[cold]
#[cfg_attr(not(feature = "panic_immediate_abort"), inline(never))]
#[track_caller]
#[lang = "panic_no_unwind"] // needed by codegen for panic in nounwind function
fn panic_no_unwind() -> ! {
if cfg!(feature = "panic_immediate_abort") {
super::intrinsics::abort()
}

// NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call
// that gets resolved to the `#[panic_handler]` function.
extern "Rust" {
#[lang = "panic_impl"]
fn panic_impl(pi: &PanicInfo<'_>) -> !;
}

// PanicInfo with the `can_unwind` flag set to false forces an abort.
let fmt = format_args!("panic in a function that cannot unwind");
let pi = PanicInfo::internal_constructor(Some(&fmt), Location::caller(), false);

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
}

/// The entry point for panicking with a formatted message.
///
/// This is designed to reduce the amount of code required at the call
Expand Down Expand Up @@ -104,7 +129,7 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
fn panic_impl(pi: &PanicInfo<'_>) -> !;
}

let pi = PanicInfo::internal_constructor(Some(&fmt), Location::caller());
let pi = PanicInfo::internal_constructor(Some(&fmt), Location::caller(), true);

// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
unsafe { panic_impl(&pi) }
Expand Down
1 change: 1 addition & 0 deletions library/std/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@
#![feature(once_cell)]
#![feature(panic_info_message)]
#![feature(panic_internals)]
#![feature(panic_can_unwind)]
#![feature(panic_unwind)]
#![feature(pin_static_ref)]
#![feature(portable_simd)]
Expand Down
22 changes: 14 additions & 8 deletions library/std/src/panicking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -576,9 +576,14 @@ pub fn begin_panic_handler(info: &PanicInfo<'_>) -> ! {
let msg = info.message().unwrap(); // The current implementation always returns Some
crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
if let Some(msg) = msg.as_str() {
rust_panic_with_hook(&mut StrPanicPayload(msg), info.message(), loc);
rust_panic_with_hook(&mut StrPanicPayload(msg), info.message(), loc, info.can_unwind());
} else {
rust_panic_with_hook(&mut PanicPayload::new(msg), info.message(), loc);
rust_panic_with_hook(
&mut PanicPayload::new(msg),
info.message(),
loc,
info.can_unwind(),
);
}
})
}
Expand All @@ -602,7 +607,7 @@ pub const fn begin_panic<M: Any + Send>(msg: M) -> ! {

let loc = Location::caller();
return crate::sys_common::backtrace::__rust_end_short_backtrace(move || {
rust_panic_with_hook(&mut PanicPayload::new(msg), None, loc)
rust_panic_with_hook(&mut PanicPayload::new(msg), None, loc, true)
});

struct PanicPayload<A> {
Expand Down Expand Up @@ -647,6 +652,7 @@ fn rust_panic_with_hook(
payload: &mut dyn BoxMeUp,
message: Option<&fmt::Arguments<'_>>,
location: &Location<'_>,
can_unwind: bool,
) -> ! {
let (must_abort, panics) = panic_count::increase();

Expand All @@ -663,14 +669,14 @@ fn rust_panic_with_hook(
} else {
// Unfortunately, this does not print a backtrace, because creating
// a `Backtrace` will allocate, which we must to avoid here.
let panicinfo = PanicInfo::internal_constructor(message, location);
let panicinfo = PanicInfo::internal_constructor(message, location, can_unwind);
rtprintpanic!("{}\npanicked after panic::always_abort(), aborting.\n", panicinfo);
}
intrinsics::abort()
crate::sys::abort_internal();
}

unsafe {
let mut info = PanicInfo::internal_constructor(message, location);
let mut info = PanicInfo::internal_constructor(message, location, can_unwind);
let _guard = HOOK_LOCK.read();
match HOOK {
// Some platforms (like wasm) know that printing to stderr won't ever actually
Expand All @@ -691,13 +697,13 @@ fn rust_panic_with_hook(
};
}

if panics > 1 {
if panics > 1 || !can_unwind {
// If a thread panics while it's already unwinding then we
// have limited options. Currently our preference is to
// just abort. In the future we may consider resuming
// unwinding or otherwise exiting the thread cleanly.
rtprintpanic!("thread panicked while panicking. aborting.\n");
intrinsics::abort()
crate::sys::abort_internal();
}

rust_panic(payload)
Expand Down
7 changes: 6 additions & 1 deletion library/std/src/sys/unix/process/process_unix/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,10 @@ fn test_command_fork_no_unwind() {
let status = got.expect("panic unexpectedly propagated");
dbg!(status);
let signal = status.signal().expect("expected child process to die of signal");
assert!(signal == libc::SIGABRT || signal == libc::SIGILL || signal == libc::SIGTRAP);
assert!(
signal == libc::SIGABRT
|| signal == libc::SIGILL
|| signal == libc::SIGTRAP
|| signal == libc::SIGSEGV
);
}
2 changes: 1 addition & 1 deletion src/test/codegen/unwind-and-panic-abort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ extern "C-unwind" {

// CHECK: Function Attrs:{{.*}}nounwind
// CHECK-NEXT: define{{.*}}void @foo
// CHECK: call void @llvm.trap()
// CHECK: call void @_ZN4core9panicking15panic_no_unwind
#[no_mangle]
pub unsafe extern "C" fn foo() {
bar();
Expand Down