Use `call` instead of `invoke` for functions that cannot unwind by wesleywiser · Pull Request #70467 · rust-lang/rust

wesleywiser · 2020-03-27T11:54:50Z

The FnAbi now knows if the function is allowed to unwind. If a
function isn't allowed to unwind, we can use a call instead of an
invoke.

This resolves an issue when calling LLVM intrinsics which cannot unwind
LLVM will generate an error if you attempt to invoke them so we need to
ignore cleanup blocks in codegen and generate a call instead.

Fixes#69911

r? @eddyb
cc @rust-lang/wg-ffi-unwind

nikic · 2020-03-27T12:55:42Z

There are some LLVM intrinsics that do support invokes, you can find the list at https://github.com/llvm/llvm-project/blob/30a8b77080b98ffdd1837d2ea2e74f91e40cc867/llvm/lib/IR/Verifier.cpp#L4164-L4175. Apart from maybe the wasm one, probably nothing that's relevant to Rust.

As a side-note, LLVM intrinsics should always use the "unadjusted" ABI, something that is not happening right now.

eddyb · 2020-03-27T13:14:34Z

I think this PR is branded as fixing a bug, but it's changing a lot about how nounwind is computed and works, so it should be rebranded or split into two PRs.

@nikic I think those intrinsics (if ever used directly from a library, which we should've never allowed, but that's another discussion) should be marked explicitly with #[unwind].

wesleywiser · 2020-03-27T16:29:07Z

@nikic I think those intrinsics (if ever used directly from a library, which we should've never allowed, but that's another discussion) should be marked explicitly with #[unwind].

I agree. I wouldn't want to maintain a list of unwinding intrinsics in rustc and using #[unwind] on those would work.

bors · 2020-03-30T08:33:24Z

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

RalfJung · 2020-03-30T16:01:22Z

If this logic now lives in librustc, that might help with #65303.

@eddyb

…ddyb Add `can_unwind` field to `FnAbi` This is a pure refactoring with no behavior changes. Extracted out of rust-lang#70467 r? @eddyb

@eddyb

…ddyb Add `can_unwind` field to `FnAbi` This is a pure refactoring with no behavior changes. Extracted out of rust-lang#70467 r? @eddyb

The `FnAbi` now knows if the function is allowed to unwind. If a function isn't allowed to unwind, we can use a `call` instead of an `invoke`. This resolves an issue when calling LLVM intrinsics which cannot unwind LLVM will generate an error if you attempt to invoke them so we need to ignore cleanup blocks in codegen and generate a call instead.

wesleywiser · 2020-04-15T12:58:52Z

I believe this is ready for review.

eddyb · 2020-04-16T00:05:22Z

r? @nagisa (LGTM, but I'm not sure if this is going to affect some ongoing ffi-unwind stuff)

nagisa

r=me after at least an issue is filled about not generating cleanup blocks in the first place.

nagisa · 2020-04-16T02:38:05Z

- if let Some(cleanup) = cleanup {
+ // If there is a cleanup block and the function we're calling can unwind, then
+ // do an invoke, otherwise do a call.
+ if let Some(cleanup) = cleanup.filter(|_| fn_abi.can_unwind) {


IMO in most instances this should happen way earlier. That is, the cleanup blocks should not be reach codegen in the first place for calls of functions that cannot unwind. This can happen either during MIR construction or afterwards in some clean-up pass.
This piece of code still needs to exist, of course, because fn_abi information in some cases may only be available after monomorphization, but the test case that changes in this PR does not involve generics...
I’m fine with work to do this happening in some follow-up PR (possibly implemented by somebody else), but in that case at least an issue needs to exist to that effect.

BatmanAoD · 2020-04-16T03:12:30Z

@eddyb @Amanieu will this cause us to use call for any pthread-cancelable functions? Otherwise, AFAIK it should not be a problem from the perspective of FFI-unwind.

Amanieu · 2020-04-16T08:30:25Z

@BatmanAoD It doesn't matter since unwinding from extern "C" is UB. pthread cancellation uses unwinding at first and then switches to longjmp if it can't find unwind info for a frame. The only valid way to use these functions is to assume they longjmp and avoid any destructors in frame that they unwind.

wesleywiser · 2020-04-17T00:03:26Z

@bors r=nagisa

bors · 2020-04-17T00:03:28Z

📌 Commit 8da26e0 has been approved by nagisa

@eddyb

Use `call` instead of `invoke` for functions that cannot unwind The `FnAbi` now knows if the function is allowed to unwind. If a function isn't allowed to unwind, we can use a `call` instead of an `invoke`. This resolves an issue when calling LLVM intrinsics which cannot unwind LLVM will generate an error if you attempt to invoke them so we need to ignore cleanup blocks in codegen and generate a call instead. Fixesrust-lang#69911 r? @eddyb cc @rust-lang/wg-ffi-unwind

@ghost

Rollup of 6 pull requests Successful merges: - rust-lang#70467 (Use `call` instead of `invoke` for functions that cannot unwind ) - rust-lang#71070 (rustbuild: Remove stage 0 LLD flavor workaround for MSVC) - rust-lang#71167 (big-O notation: parenthesis for function calls, explicit multiplication) - rust-lang#71238 (Miri: fix typo) - rust-lang#71242 (Format Mailmap To Work With GitHub) - rust-lang#71243 (Account for use of `try!()` in 2018 edition and guide users in the right direction) Failed merges: r? @ghost

rust-highfive assigned eddyb Mar 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 27, 2020

eddyb reviewed Mar 27, 2020
View reviewed changes

Comment threadsrc/librustc/ty/layout.rs Outdated

eddyb reviewed Mar 27, 2020
View reviewed changes

Comment threadsrc/librustc/ty/layout.rs Outdated

eddyb reviewed Mar 27, 2020
View reviewed changes

Comment threadsrc/librustc/ty/layout.rs Outdated

This comment has been minimized.
Sign in to view

wesleywiser force-pushed the invoke-vs-call branch from c0d2691 to 5461dffCompare March 27, 2020 14:43

wesleywiser mentioned this pull request Mar 30, 2020
Add can_unwind field to FnAbi#70547
Merged

RalfJung reviewed Mar 30, 2020
View reviewed changes

Comment threadsrc/librustc/ty/layout.rs Outdated
RalfJungMar 30, 2020
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this logic now lives in librustc, that might help with #65303.

joelpalmer added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 9, 2020

Dylan-DPC-zz 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 Apr 9, 2020

wesleywiser force-pushed the invoke-vs-call branch from 5461dff to 098c46dCompare April 15, 2020 01:09

wesleywiser force-pushed the invoke-vs-call branch from 098c46d to 8da26e0Compare April 15, 2020 10:00

wesleywiser changed the title ~~Use call instead of invoke on LLVM intrinsics~~Use call instead of invoke for functions that cannot unwind Apr 15, 2020

rust-highfive assigned nagisa and unassigned eddyb Apr 16, 2020

nagisa approved these changes Apr 16, 2020
View reviewed changes

wesleywiser mentioned this pull request Apr 17, 2020
Don't generate unnecessary cleanup blocks for calls that cannot unwind #71229
Open

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 Apr 17, 2020

Dylan-DPC-zz mentioned this pull request Apr 17, 2020
Rollup of 4 pull requests #71245
Closed

Dylan-DPC-zz mentioned this pull request Apr 17, 2020
Rollup of 6 pull requests #71264
Merged

bors merged commit 43cf9d7 into rust-lang:masterApr 18, 2020

oxalica mentioned this pull request Aug 18, 2025
Missed-optimization: extern "C" fn type calls are not nounwind #64090
Closed

Uh oh!

Uh oh!

Conversation

wesleywiser commented Mar 27, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nikic commented Mar 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eddyb commented Mar 27, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

wesleywiser commented Mar 27, 2020

Uh oh!

bors commented Mar 30, 2020

Uh oh!

RalfJungMar 30, 2020

Choose a reason for hiding this comment

Uh oh!

wesleywiser commented Apr 15, 2020

Uh oh!

eddyb commented Apr 16, 2020

Uh oh!

nagisa left a comment

Choose a reason for hiding this comment

Uh oh!

nagisaApr 16, 2020

Choose a reason for hiding this comment

Uh oh!

BatmanAoD commented Apr 16, 2020

Uh oh!

Amanieu commented Apr 16, 2020

Uh oh!

wesleywiser commented Apr 17, 2020

Uh oh!

bors commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

wesleywiser commented Mar 27, 2020•
edited
Loading

eddyb commented Mar 27, 2020•
edited
Loading