rustc: Tweak funclet cleanups of ffi functions

[alexcrichton]

This commit is targeted at addressing #48251 by specifically fixing a case where
a longjmp over Rust frames on MSVC runs cleanups, accidentally running the
"abort the program" cleanup as well. Added in #46833 extern ABI functions in
Rust will abort the process if Rust panics, and currently this is modeled as a
normal cleanup like all other destructors.

Unfortunately it turns out that longjmp on MSVC is implemented with SEH, the
same mechanism used to implement panics in Rust. This means that longjmp over
Rust frames will run Rust cleanups (even though we don't necessarily want it
to). Notably this means that if you longjmp over a Rust stack frame then that
probably means you'll abort the program because one of the cleanups will abort
the process.

After some discussion on IRC it turns out that longjmp doesn't run cleanups
for caught exceptions, it only runs cleanups for cleanup pads. Using this
information this commit tweaks the codegen for an extern function to
a catch-all clause for exceptions instead of a cleanup block. This catch-all is
equivalent to the C++ code:

try {
    foo();
} catch (...) {
    bar();
}

and in fact our codegen here is designed to match exactly what clang emits for
that C++ code!

With this tweak a longjmp over Rust code will no longer abort the process. A
longjmp will continue to "accidentally" run Rust cleanups (destructors) on MSVC.
Other non-MSVC platforms will not rust destructors with a longjmp, so we'll
probably still recommend "don't have destructors on the stack", but in any case
this is a more surgical fix than #48567 and should help us stick to standard
personality functions a bit longer.

[rust-highfive]

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

[majnemer]

The pointer usually points to an RTTI type descriptor, catch(...) doesn't have a type so this is null.
As for the 64, its a bit in a bitfield describing if the catch is by reference, const, etc. In this case, 64 indicates catch-all.

[alexcrichton]

Aha, thanks!

[petrochenkov]

I'm not qualified to review this.
r? @nagisa @vadimcn @eddyb

[nagisa]

Won't be able to review till friday.

…

On Feb 27, 2018 13:08, "Alex Crichton" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/librustc_trans/mir/mod.rs <#48572 (comment)>: > + // modeled after clang's for: + // + // try { + // foo(); + // } catch (...) { + // bar(); + // } + Some(&mir::TerminatorKind::Abort) => { + let cs_bx = bx.build_sibling_block(&format!("cs_funclet{:?}", bb)); + let cp_bx = bx.build_sibling_block(&format!("cp_funclet{:?}", bb)); + ret_llbb = cs_bx.llbb(); + + let cs = cs_bx.catch_switch(None, None, 1); + cs_bx.add_handler(cs, cp_bx.llbb()); + + // No idea what this null/64 are for, but it's what clang does! Aha, thanks! — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#48572 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AApc0kmkUkL0x5EJ-Cyj4rdTOnnb5-_Xks5tY_BLgaJpZM4SUUR0> .

[eddyb]

@bors r+

LGTM, although I'm wondering if putting the aborts in MIR (instead of the backend) was a mistake.

[bors]

📌 Commit e2af240 has been approved by eddyb

[diwic]

I'm trying to understand how this test works. According to earlier comment, longjmp will still run destructors on MSVC. As such, A's destructor will run, causing a panic, but since we're in unwind mode, this will be a double unwind, causing an abort for that reason, or...?

[alexcrichton]

Ah yes that was a mistake on my part, this test was originally included in a PR where it was fixed, but I changed strategies at the last minute.

[Manishearth]

Fails on appveyor

#48615 (comment)

[alexcrichton]

@bors: r=eddyb

[bors]

📌 Commit 804666f has been approved by eddyb

[bors]

🌲 The tree is currently closed for pull requests below priority 200, this pull request will be tested once the tree is reopened

[alexcrichton]

Nominating for a backport to beta as I think this turned out to be a small enough change and this'll help re-close #48251

[fweimer]

Note that this:

try {
    foo();
} catch (...) {
    bar();
}

still breaks thread cancellation on GNU/Linux if bar() aborts unconditionally. The assumptions that FFI calls cannot unwind is not true on this platform.

[alexcrichton]

@fweimer can you expand a bit on that? I wasn't aware of this problem!

[fweimer]

@alexcrichton Not sure where to start. I previously tried to ask about this over there:

• Thread cancellation on GNU/Linux

[vadimcn]

Indiscriminate unwinding through an FFI boundary is a very likely to end badly. There may be code on the other side written in a language that does not support unwinding (e.g. C). Unwinding through such code will leak all its resources (memory, open files, acquired mutexes, etc), even if the compiler has generated unwind tables.
Btw, Rust itself does not promise to execute cleanups on foreign exceptions. I know for fact that this is the case for for x86_64-pc-windows-gnu target, not sure about others. (Should we consider putting unwind guards on the caller side of FFI as well?)

There are cases when you do know that it's ok to unwind through FFI, but doing so should be opt-in. We already have the #[unwind] attribute for this purpose, though it's currently unstable.

[alexcrichton]

@fweimer it sounds like pthread_cancel has problems with C++ noexcept anyway? In that sense I'd at least by default not expect pthread_cancel to work whereas longjmp/setjmp seems more standard to be expected to work at least. That's not to say we couldn't get it working, but as @vadimcn mentioned it's pretty scary to unwind without Rust exceptions so it's in general best to avoid that.

[fweimer]

@alexcrichton pthread_cancel has problems with the current implementation of std::thread, I think. (EDIT I was wrong, this is something that was fixed back in 2012 in libstdc++.) You can still call pthread_create directly and use pthread_cancel with the thread. If I understand the current Rust changes correctly, you have just removed that possibility from the released Rust compiler, without providing a replacement.

[alexcrichton]

Er sorry I'm not really sure I understand, can you provide an example of the breakage?

[nagisa]

This specific PR only affects Windows, not POSIX platforms. Admittedly, there might be yet another case where on Windows unwinding is used and, unlike longjmp, is caught by catch(...), but we are not currently aware of any. pthread_cancel is not something supported by Windows, and such is of no concern.

---

On POSIX, meanwhile, code like one below, is and always was undefined Rust (ignore all the bad bindings and stuff, it is something I prototyped quickly to demonstrate the point):

use std::os::unix::thread::*;

extern "C" {
    pub fn pthread_cancel(h: RawPthread) -> i32;
    pub fn pthread_create(t: *mut RawPthread, a: *mut (), t: extern "C" fn(*const ()) -> *const (), b: *mut ()) -> i32;
}

extern "C" fn banana(x: *const ()) -> *const () {
    ::std::thread::sleep(::std::time::Duration::from_millis(500)); // might unwind due to pthread_cancel and is UB
    x
}

fn main() {
    unsafe {
        let mut t: RawPthread = ::std::mem::zeroed();
        pthread_create(&mut t, ::std::ptr::null_mut(), banana, ::std::ptr::null_mut());
        pthread_cancel(t);
    }
    ::std::thread::sleep(::std::time::Duration::from_millis(1000));
}

Currently, the safety implemented in #46833 is disabled, so this code will silently work (it does not work when the safety net is re-enabled), however, to really make sure this code is not undefined, you need to specify the #[unwind] attribute on top of banana, as such:

#[unwind]
extern "C" fn banana(x: *const ()) -> *const () {
    ::std::thread::sleep(::std::time::Duration::from_millis(500)); // might unwind due to pthread_cancel and is not UB anymore, because `banana` is specified to unwind.
    x
}

This is exactly the same rule, that applies to any extern "C" function that might unwind for any reason.

[nikomatsakis]

We discussed in the @rust-lang/compiler meeting. We would prefer not to backport this -- hence I'm removing the label. The main reason is that there doesn't seem to be an urgency; the aborts are currently disabled, so we can just let this ride the trains as usual.