move new c abi abort behavior behind feature gate

[cratelyn]

Background

In #76570, new ABI strings including C-unwind were introduced. Their
behavior is specified in RFC 2945 ¹.

However, it was reported in the #ffi-unwind stream of the Rust community Zulip
that this had altered the way that extern "C" functions behaved even when the
c_unwind feature gate was not active. ²

Overview

This makes a small patch to rustc_mir_build::build::should_abort_on_panic, so
that the same behavior from before is in place when the c_unwind gate is not
active.

rustc_middle::ty::layout::fn_can_unwind is not touched, as the visible
behavior should not differ before/after #76570. ³

---

1: https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md
2: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/Is.20unwinding.20through.20extern.20C.20UB.3F/near/230112325
3: https://github.com/rust-lang/rust/pull/76570/files#diff-b0320c2b8868f325d83c027fc5d71732636e9763551e35895488f30fe057c6e9L2599-R2617

[rust-highfive]

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

[BatmanAoD]

Does rustc_middle::ty::layout::fn_can_unwind control whether the nounwind attribute is emitted? If possible, we should not emit that for "C" when the "C-unwind" feature is not enabled, because it's the source of the UB.

[cratelyn]

Does rustc_middle::ty::layout::fn_can_unwind control whether the nounwind attribute is emitted? If possible, we should not emit that for "C" when the "C-unwind" feature is not enabled, because it's the source of the UB.

Hi! I want to make sure that we're not mixed up here; I realize the distinctions between "should abort" and "can unwind" are not initially intuitive and aren't quite the direct inverses that I initially believed them to be.

First, to answer your original question, yes rustc_middle::ty::layout::fn_can_unwind controls whether the nounwind attribute is emitted. You can see that happen here specifically, in <FnAbi<'tcx, Ty<'tcx>> as FnAbiLlvmExt<'tcx>>::apply_attrs_llfn.

But, I want to highlight that this UB exists on stable today, unless I'm mistaken. If we take this file...

#![crate_type = "lib"]
pub extern "C" fn huhu() {}

and compile it like so...

$: rustc --version                                                       
rustc 1.51.0 (2fd73fabe 2021-03-23)
$: rustc --edition=2018 --emit=llvm-ir -C opt-level=0 extern-c-example.rs
$: /bin/cat extern-c-example.ll
; ModuleID = 'extern_c_example.3a1fbbbh-cgu.0'
source_filename = "extern_c_example.3a1fbbbh-cgu.0"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

; extern_c_example::huhu
; Function Attrs: nounwind nonlazybind uwtable
define void @_ZN16extern_c_example4huhu17h914345848a8b0e88E() unnamed_addr #0 {
start:
  ret void
}

attributes #0 = { nounwind nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" }

!llvm.module.flags = !{!0, !1}

!0 = !{i32 7, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}

...we'll see the same nounwind. The behavior that's changed in the wake of #76570 is whether or not an extern "C" function will abort on panic, which you can see in this particular part of that diff.

we should not emit that for "C" when the "C-unwind" feature is not enabled, because it's the source of the UB.

So to get to this second part of your question @BatmanAoD, I believe we should keep the nounwind emission the same as it is today, as it matches the current behavior of stable w.r.t extern "C" functions.

Does that make sense?

[BatmanAoD]

Yes, that makes sense. You're right that the UB exists today, but it would be better to eliminate it as soon as possible, I think; that's the main reason to land a patch rather than revert the original change.

…

On Tue, Apr 13, 2021, 7:30 PM katelyn martin ***@***.***> wrote: Does rustc_middle::ty::layout::fn_can_unwind control whether the nounwind attribute is emitted? If possible, we should *not* emit that for "C" when the "C-unwind" feature is not enabled, because it's the source of the UB. Hi! I want to make sure that we're not mixed up here; I realize the distinctions between "*should abort*" and "*can unwind*" are not initially intuitive and aren't quite the direct inverses that I initially believed them to be. First, to answer your original question, *yes* rustc_middle::ty::layout::fn_can_unwind controls whether the nounwind attribute is emitted. You can see that happen here <https://github.com/cratelyn/rust/blob/1284da34da56a17ae368e4673920ec4120562cbd/compiler/rustc_codegen_llvm/src/abi.rs#L444-L447> specifically, in <FnAbi<'tcx, Ty<'tcx>> as FnAbiLlvmExt<'tcx>>::apply_attrs_llfn. *But*, I want to highlight that this UB exists on stable today, unless I'm mistaken. If we take this file... #![crate_type = "lib"]pub extern "C" fn huhu() {} and compile it like so... $: rustc --version rustc 1.51.0 (2fd73fa 2021-03-23) $: rustc --edition=2018 --emit=llvm-ir -C opt-level=0 extern-c-example.rs $: /bin/cat extern-c-example.ll; ModuleID = 'extern_c_example.3a1fbbbh-cgu.0' source_filename = "extern_c_example.3a1fbbbh-cgu.0" target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" target triple = "x86_64-unknown-linux-gnu" ; extern_c_example::huhu; Function Attrs: nounwind nonlazybind uwtable define void @_ZN16extern_c_example4huhu17h914345848a8b0e88E() unnamed_addr #0 { start: ret void } attributes #0 = { nounwind nonlazybind uwtable "probe-stack"="__rust_probestack" "target-cpu"="x86-64" } !llvm.module.flags = !{!0, !1} !0 = !{i32 7, !"PIC Level", i32 2}!1 = !{i32 2, !"RtLibUseGOT", i32 1} ...we'll see the same nounwind. The behavior that's changed in the wake of #76570 <#76570> is whether or not an extern "C" function will abort on panic, which you can see in this <https://github.com/rust-lang/rust/pull/76570/files#diff-596bb4000aae6e6b37927544992b2bdf7e7ac5bad13ea2becadb4cc2638fa2acL561-L564> particular part of that diff. we should *not* emit that for "C" when the "C-unwind" feature is not enabled, because it's the source of the UB. So to get to this second part of your question @BatmanAoD <https://github.com/BatmanAoD>, I believe we should keep the nounwind emission the same as it is today, as it matches the current behavior of stable w.r.t extern "C" functions. Does that make sense? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#84158 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARU4T7JOYBHH4R2GNMOKLLTITV2BANCNFSM423OCVZQ> .

[nikomatsakis]

@bors r+

[bors]

📌 Commit 3e16d23 has been approved by nikomatsakis

[bors]

⌛ Testing commit 3e16d23 with merge 07ef259...

[bors]

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 07ef259 to master...

[pnkfelix]

beta-nominating, because I believe our intention was for this to get into 1.52 in order to avoid bugs like #83541 hitting stable.

[cratelyn]

Seconding, that was exactly the intention 🙂

[wesleywiser]

We discussed this in the compiler team triage meeting this morning and the consensus was that we would like to try reverting #76570 on beta first. If that proves complicated, then we will backport this PR instead.