Permit unwinding through FFI by default #62603

cuviper · 2019-07-11T21:42:10Z

This repeats #62505 for master (Rust 1.38+), as #58794 is not yet resolved. This is a stopgap until a stable alternative is available, like RFC 2699, as long as progress is being made to that end.

r? @joshtriplett

RalfJung · 2019-07-13T11:16:09Z

To be clear, this allows extern C functions to unwind and it does not set the nounwind attribute for them? So there's lots of platform-specific concerns, but at least not outright UB?

cuviper · 2019-07-18T18:32:09Z

@RalfJung no, this has no effect on the nounwind attribute, maintaining the (questionable) status quo that we've been releasing with so far. The only effect here is whether we add a cleanup block to non-Rust-ABI functions, so it forces an abort on panic/unwinding.

I believe nounwind is controlled here:

rust/src/librustc_codegen_llvm/attributes.rs

Lines 278 to 286 in 2eb0bc5

    
           if cx.tcx.is_foreign_item(id) { 
        
               // Foreign items like `extern "C" { fn foo(); }` are assumed not to 
        
               // unwind 
        
               false 
        
           } else if sig.abi != Abi::Rust && sig.abi != Abi::RustCall { 
        
               // Any items defined in Rust that *don't* have the `extern` ABI are 
        
               // defined to not unwind. We insert shims to abort if an unwind 
        
               // happens to enforce this. 
        
               false

bors · 2019-07-27T23:11:30Z

☔ The latest upstream changes (presumably #63029) made this pull request unmergeable. Please resolve the merge conflicts.

cuviper · 2019-07-31T00:26:53Z

Rebased.

Alexendoo · 2019-08-07T12:06:50Z

Ping from triage: Any updates? @joshtriplett

cuviper · 2019-08-13T16:14:38Z

A new beta 1.38 has branched -- @joshtriplett what's your preference here? Are we ready to land this on master and then nominate for beta? Or should I do another beta-targeted PR?

bors · 2019-08-16T10:44:00Z

☔ The latest upstream changes (presumably #63627) made this pull request unmergeable. Please resolve the merge conflicts.

wirelessringo · 2019-08-23T16:25:11Z

Ping from triage. @cuviper please resolve the merge conflicts. Thanks.

@rustbot modify labels to +S-waiting-on-author, -S-waiting-on-review

RalfJung · 2019-08-25T13:08:58Z

I believe nounwind is controlled here:

Could you expand this PR to also remove the nounwind then? We keep applying this on beta, which is silly -- but IMO applying this patch while still emitting nounwind is also a bad idea.

With that change, I will nominate this for lang-team discussion.

RalfJung · 2019-08-25T13:11:38Z

Ideally, that code (deciding about the LLVM unwind attribute) would call should_abort_on_panic. Not sure if that is possible.

EDIT: Hm, this is odd -- the code tests for CodegenFnAttrFlags::UNWIND but I am not sure how that relates to Rust's UnwindAttr.

EDIT2: Looks like the code here permits a function to unwind if any unwind attribute is present, including unwind(aborts)?!?

RalfJung · 2019-08-25T13:26:31Z

Looks like #48380 repurposed the attribute, but some old users still treat is as basically a boolean. I think CodegenFnAttrFlags::UNWIND should be removed, and from_fn_attrs should just call should_abort_on_panic to make sure that it only ever adds nounwind when we really do the abort-on-panic. I can do this in a separate cleanup PR, maybe?

RalfJung · 2019-08-25T15:00:30Z

See #63884 for fixing how we control the nounwind attribute. If both that PR and this one land, that will correctly stop adding nounwind to extern "C" fn.

See rust-lang#58794 for context.

This reverts commit b4a6f59.

cuviper · 2019-08-25T17:14:47Z

@RalfJung - Thanks for tackling the other side of this!

I've rebased this PR, and I got a verbal r+ to go ahead at RustConf. I'll also nominate for beta since we missed the branch.

@bors r=joshtriplett

bors · 2019-08-25T17:14:48Z

📌 Commit 367b793 has been approved by joshtriplett

bors · 2019-08-26T00:27:26Z

⌛ Testing commit 367b793 with merge 4c58535...

@joshtriplett

Permit unwinding through FFI by default This repeats #62505 for master (Rust 1.38+), as #58794 is not yet resolved. This is a stopgap until a stable alternative is available, like [RFC 2699](rust-lang/rfcs#2699), as long as progress is being made to that end. r? @joshtriplett

bors · 2019-08-26T04:10:47Z

☀️ Test successful - checks-azure
Approved by: joshtriplett
Pushing 4c58535 to master...

fix nounwind attribute logic With #62603 landed, we no longer add abort-on-panic shims to `extern "C"` functions. However, that means we should also not emit `nounwind` for `extern fn` because we are not actually catching those panics. The comment justifying that `nounwind` in the source is just factually wrong. I also noticed that we annotate `extern` *declarations* (of any ABI) with `nounwind`, which seems wrong? I removed this. Code like mozjpeg (the code that prompted the removal of the abort-on-panic shim) throws a panic from Rust code that enters C code (meaning the `extern "C"` *definition* must not have `nounwind`, or else we got UB), but then the panic bubbles through the C code and re-enters Rust code (meaning the `extern "C"` *declaration* that gets called from the Rust side must not have `nounwind` either). This should be beta-backported if #62603 gets backported.

pnkfelix · 2019-09-13T09:49:59Z

based on my interpretation of the recent Lang Team meetings (including the one last night), I am going to unilaterally accept this for a beta backport, due to time pressure with a release in two weeks. @joshtriplett shares this interpretation of the lang team meetings.

(The other motivating detail is that, the way things are now, if we do not beta-backport this, then we will have breakage that is solely on the current beta, not in stable nor master, which means we'd be scheduling one cycle of breakage.)

(I'm pretty sure there was some sort of process failure along the way here that led to a breakdown in our normal procedure, probably due to a conflation of how this PR is semi-coupled with PR #63909, the question of which teams should have been tagged to each PR, etc...)

cuviper · 2019-09-13T22:51:02Z

Thanks @pnkfelix -- the backport is underway in #64438.

@Mark-Simulacrum

[beta] Rollup backports Cherry-picked: - Permit unwinding through FFI by default #62603 - pprust: Do not print spaces before some tokens #63897 - Account for doc comments coming from proc macros without spans #63930 - Support "soft" feature-gating using a lint #64066 - Update xLTO compatibility table in rustc book. #64092 - Include compiler-rt in the source tarball #64240 - Update LLVM submodule #64317 r? @Mark-Simulacrum

rust-highfive assigned joshtriplett Jul 11, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 11, 2019

cuviper mentioned this pull request Jul 11, 2019

[beta] Permit unwinding through FFI by default #62505

Merged

cuviper force-pushed the no-unwind-abort branch from 8320809 to 699b1c2 Compare July 31, 2019 00:26

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2019

RalfJung added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Aug 25, 2019

This was referenced Aug 25, 2019

update UB rust-lang/reference#659

Merged

sync adding nounwind attribute with generating abort-on-panic shim #63884

Closed

Mark-Simulacrum and others added 3 commits August 25, 2019 09:47

Permit unwinding through FFI by default

39412ca

See rust-lang#58794 for context.

Revert "Allow a dirty MirBuilt for make_extern and make_method_extern"

d810e77

This reverts commit b4a6f59.

Force #[unwind(aborts)] in test/codegen/c-variadic.rs

367b793

cuviper force-pushed the no-unwind-abort branch from 699b1c2 to 367b793 Compare August 25, 2019 17:11

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 25, 2019

cuviper added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 25, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 26, 2019

bors merged commit 367b793 into rust-lang:master Aug 26, 2019

This was referenced Aug 26, 2019

Validation: check raw wide pointer metadata #63880

Merged

fix nounwind attribute logic #63909

Closed

gnzlbg mentioned this pull request Aug 27, 2019

Undefined Behavior in safe code that unwinds out of extern "C" function #63943

Closed

RalfJung mentioned this pull request Sep 10, 2019

Fixes soundness bug 18510 by aborting on unwind from safe extern "C" functions only #64315

Closed

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 13, 2019

cuviper mentioned this pull request Sep 13, 2019

[beta] Rollup backports #64438

Merged

cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 14, 2019

cuviper deleted the no-unwind-abort branch April 3, 2020 18:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit unwinding through FFI by default #62603

Permit unwinding through FFI by default #62603

cuviper commented Jul 11, 2019

RalfJung commented Jul 13, 2019 •

edited

Loading

cuviper commented Jul 18, 2019

bors commented Jul 27, 2019

cuviper commented Jul 31, 2019

Alexendoo commented Aug 7, 2019

cuviper commented Aug 13, 2019

bors commented Aug 16, 2019

wirelessringo commented Aug 23, 2019

RalfJung commented Aug 25, 2019 •

edited

Loading

RalfJung commented Aug 25, 2019 •

edited

Loading

RalfJung commented Aug 25, 2019

RalfJung commented Aug 25, 2019

cuviper commented Aug 25, 2019

bors commented Aug 25, 2019

bors commented Aug 26, 2019

bors commented Aug 26, 2019

pnkfelix commented Sep 13, 2019 •

edited

Loading

cuviper commented Sep 13, 2019

Permit unwinding through FFI by default #62603

Permit unwinding through FFI by default #62603

Conversation

cuviper commented Jul 11, 2019

RalfJung commented Jul 13, 2019 • edited Loading

cuviper commented Jul 18, 2019

bors commented Jul 27, 2019

cuviper commented Jul 31, 2019

Alexendoo commented Aug 7, 2019

cuviper commented Aug 13, 2019

bors commented Aug 16, 2019

wirelessringo commented Aug 23, 2019

RalfJung commented Aug 25, 2019 • edited Loading

RalfJung commented Aug 25, 2019 • edited Loading

RalfJung commented Aug 25, 2019

RalfJung commented Aug 25, 2019

cuviper commented Aug 25, 2019

bors commented Aug 25, 2019

bors commented Aug 26, 2019

bors commented Aug 26, 2019

pnkfelix commented Sep 13, 2019 • edited Loading

cuviper commented Sep 13, 2019

RalfJung commented Jul 13, 2019 •

edited

Loading

RalfJung commented Aug 25, 2019 •

edited

Loading

RalfJung commented Aug 25, 2019 •

edited

Loading

pnkfelix commented Sep 13, 2019 •

edited

Loading