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

Prevent aborting guard from aborting the process in a forced unwind #104070

Merged
merged 7 commits into from
May 8, 2023
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
5 changes: 5 additions & 0 deletions compiler/rustc_codegen_gcc/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1227,6 +1227,11 @@ impl<'a, 'gcc, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'gcc, 'tcx> {
(value1, value2)
}

fn filter_landing_pad(&mut self, pers_fn: RValue<'gcc>) -> (RValue<'gcc>, RValue<'gcc>) {
// TODO(antoyo): generate the correct landing pad
self.cleanup_landing_pad(pers_fn)
Copy link
Member

Choose a reason for hiding this comment

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

This seems incorrect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The proper way to encode this in GCC would be <<<eh_filter (NULL)>>> (or even better, <<<eh_must_not_throw (terminate)>>>, which is something we hoped for for LLVM!). But I don't think there is support in libgccjit yet, @antoyo to confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'll take care of this. I'm not done with supporting unwinding yet.

But, please add a // TODO(antoyo): generate the correct landing pad.

For my personal information, what are <<<eh_filter (NULL)>>> and <<<eh_must_not_throw (terminate)>>>? I've never seen this syntax. Are they C attributes?

I don't remember exactly what filter is doing, but the proper implementation of this method might be very similar as cleanup_landing_pad, but without adding to self.cleanup_blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are part of generic/gimple: https://godbolt.org/z/5adjbahnd

}

#[cfg(feature="master")]
fn resume(&mut self, exn0: RValue<'gcc>, _exn1: RValue<'gcc>) {
let exn_type = exn0.get_type();
Expand Down
9 changes: 8 additions & 1 deletion compiler/rustc_codegen_llvm/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,13 +985,20 @@ impl<'a, 'll, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {

fn cleanup_landing_pad(&mut self, pers_fn: &'ll Value) -> (&'ll Value, &'ll Value) {
let ty = self.type_struct(&[self.type_i8p(), self.type_i32()], false);
let landing_pad = self.landing_pad(ty, pers_fn, 1 /* FIXME should this be 0? */);
let landing_pad = self.landing_pad(ty, pers_fn, 0);
unsafe {
llvm::LLVMSetCleanup(landing_pad, llvm::True);
}
(self.extract_value(landing_pad, 0), self.extract_value(landing_pad, 1))
}

fn filter_landing_pad(&mut self, pers_fn: &'ll Value) -> (&'ll Value, &'ll Value) {
let ty = self.type_struct(&[self.type_i8p(), self.type_i32()], false);
let landing_pad = self.landing_pad(ty, pers_fn, 1);
self.add_clause(landing_pad, self.const_array(self.type_i8p(), &[]));
(self.extract_value(landing_pad, 0), self.extract_value(landing_pad, 1))
}

fn resume(&mut self, exn0: &'ll Value, exn1: &'ll Value) {
let ty = self.type_struct(&[self.type_i8p(), self.type_i32()], false);
let mut exn = self.const_poison(ty);
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1600,7 +1600,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
bx = Bx::build(self.cx, llbb);

let llpersonality = self.cx.eh_personality();
bx.cleanup_landing_pad(llpersonality);
bx.filter_landing_pad(llpersonality);

funclet = None;
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_codegen_ssa/src/traits/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ pub trait BuilderMethods<'a, 'tcx>:

// These are used by everyone except msvc
fn cleanup_landing_pad(&mut self, pers_fn: Self::Value) -> (Self::Value, Self::Value);
fn filter_landing_pad(&mut self, pers_fn: Self::Value) -> (Self::Value, Self::Value);
Copy link
Member

Choose a reason for hiding this comment

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

What is the difference between cleanup and filter? How should a backend implement it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's purely metadata in LSDA. https://itanium-cxx-abi.github.io/cxx-abi/exceptions.pdf

A negative value in action record means filter, a positive value means catch, and zero (or absence of action record) means cleanup.

Copy link
Member

Choose a reason for hiding this comment

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

This corresponds to section 7.5 "Exception Specification" of that document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I call it filter because that's how LLVM calls it.

The code here doesn't actually specify any exceptions (corresponds to C++ throw()), so it's essentially catch-all. But it provides a way for the personality function to distinguish terminate vs cleanup.

fn resume(&mut self, exn0: Self::Value, exn1: Self::Value);

// These are used only by msvc
Expand Down
5 changes: 4 additions & 1 deletion library/std/src/personality/dwarf/eh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ pub enum EHAction {
None,
Cleanup(usize),
Catch(usize),
Filter(usize),
Terminate,
}

Expand Down Expand Up @@ -142,9 +143,11 @@ unsafe fn interpret_cs_action(
let ttype_index = action_reader.read_sleb128();
if ttype_index == 0 {
EHAction::Cleanup(lpad)
} else {
} else if ttype_index > 0 {
// Stop unwinding Rust panics at catch_unwind.
EHAction::Catch(lpad)
} else {
EHAction::Filter(lpad)
}
}
}
Expand Down
11 changes: 7 additions & 4 deletions library/std/src/personality/gcc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ cfg_if::cfg_if! {
EHAction::None | EHAction::Cleanup(_) => {
return continue_unwind(exception_object, context);
}
EHAction::Catch(_) => {
EHAction::Catch(_) | EHAction::Filter(_) => {
// EHABI requires the personality routine to update the
// SP value in the barrier cache of the exception object.
(*exception_object).private[5] =
Expand All @@ -147,7 +147,8 @@ cfg_if::cfg_if! {
} else {
match eh_action {
EHAction::None => return continue_unwind(exception_object, context),
EHAction::Cleanup(lpad) | EHAction::Catch(lpad) => {
EHAction::Filter(_) if state & uw::_US_FORCE_UNWIND as c_int != 0 => return continue_unwind(exception_object, context),
EHAction::Cleanup(lpad) | EHAction::Catch(lpad) | EHAction::Filter(lpad) => {
uw::_Unwind_SetGR(
context,
UNWIND_DATA_REG.0,
Expand Down Expand Up @@ -201,13 +202,15 @@ cfg_if::cfg_if! {
if actions as i32 & uw::_UA_SEARCH_PHASE as i32 != 0 {
match eh_action {
EHAction::None | EHAction::Cleanup(_) => uw::_URC_CONTINUE_UNWIND,
EHAction::Catch(_) => uw::_URC_HANDLER_FOUND,
EHAction::Catch(_) | EHAction::Filter(_) => uw::_URC_HANDLER_FOUND,
EHAction::Terminate => uw::_URC_FATAL_PHASE1_ERROR,
}
} else {
match eh_action {
EHAction::None => uw::_URC_CONTINUE_UNWIND,
EHAction::Cleanup(lpad) | EHAction::Catch(lpad) => {
// Forced unwinding hits a terminate action.
EHAction::Filter(_) if actions as i32 & uw::_UA_FORCE_UNWIND as i32 != 0 => uw::_URC_CONTINUE_UNWIND,
EHAction::Cleanup(lpad) | EHAction::Catch(lpad) | EHAction::Filter(lpad) => {
uw::_Unwind_SetGR(
context,
UNWIND_DATA_REG.0,
Expand Down
4 changes: 2 additions & 2 deletions tests/codegen/vec-shrink-panik.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub fn issue71861(vec: Vec<u32>) -> Box<[u32]> {

// Call to panic_cannot_unwind in case of double-panic is expected
// on LLVM 16 and older, but other panics are not.
// CHECK: cleanup
// CHECK: filter
// old-NEXT: ; call core::panicking::panic_cannot_unwind
// old-NEXT: panic_cannot_unwind

Expand All @@ -40,7 +40,7 @@ pub fn issue75636<'a>(iter: &[&'a str]) -> Box<[&'a str]> {

// Call to panic_cannot_unwind in case of double-panic is expected,
// on LLVM 16 and older, but other panics are not.
// CHECK: cleanup
// CHECK: filter
// old-NEXT: ; call core::panicking::panic_cannot_unwind
// old-NEXT: panic_cannot_unwind

Expand Down
9 changes: 9 additions & 0 deletions tests/run-make/forced-unwind-terminate-pof/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# ignore-cross-compile
# only-linux
include ../tools.mk

all: foo
$(call RUN,foo) | $(CGREP) -v "cannot unwind"

foo: foo.rs
$(RUSTC) $<
17 changes: 17 additions & 0 deletions tests/run-make/forced-unwind-terminate-pof/foo.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Tests that forced unwind through POF Rust frames wouldn't trigger our terminating guards.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have a Windows version of this test that uses longjmp. On Windows longjmp is implemented using SEH exceptions and effectively acts like a forced unwind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have a test longjmp-across-rust added by #48572 to fix #48251


#![feature(c_unwind)]
#![no_main]

extern "C-unwind" {
fn pthread_exit(v: *mut core::ffi::c_void) -> !;
}

unsafe extern "C" fn call_pthread_exit() {
pthread_exit(core::ptr::null_mut());
}

#[no_mangle]
unsafe extern "C-unwind" fn main(_argc: core::ffi::c_int, _argv: *mut *mut core::ffi::c_char) {
call_pthread_exit();
}