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

Disable all unwinding on -Z no-landing-pads LTO #10916

Merged
merged 1 commit into from
Dec 13, 2013

Conversation

alexcrichton
Copy link
Member

When performing LTO, the rust compiler has an opportunity to completely strip
all landing pads in all dependent libraries. I've modified the LTO pass to
recognize the -Z no-landing-pads option when also running an LTO pass to flag
everything in LLVM as nothrow. I've verified that this prevents any and all
invoke instructions from being emitted.

I believe that this is one of our best options for moving forward with
accomodating use-cases where unwinding doesn't really make sense. This will
allow libraries to be built with landing pads by default but allow usage of them
in contexts where landing pads aren't necessary.

@alexcrichton
Copy link
Member Author

I think there's a few things that should be reviewed about this:

  • whether the code is good (as usual)
  • whether this should be a crate attribute or a compile-time flag (I don't think this should be allowed as a crate attribute on rlib or dylib outputs)
  • whether this should actually close the issue in question

@huonw
Copy link
Member

huonw commented Dec 11, 2013

How will this interact with --test (especially as a crate attribute, since there is no way to conditionally add/remove individual attributes from items (and, in fact, no way at all for crate attributes))?

@thestinger
Copy link
Contributor

@alexcrichton: I'm not quite sure it should close it, because it would be nice to be able to omit them without optimizations enabled or without link-time optimization. This should also probably not work if unwinding is used because it's going to be undefined behaviour. One way to do that would be having an abort language item in addition to fail, and enforcing that only one or the other it undefined. It's still up to the user to get it right, but it will prevent problems with the default standard library configuration.

@alexcrichton
Copy link
Member Author

I don't think that we should encourage distribution of libraries with -Z no-landing-pads because most rust programs cannot safely link to them. I am very much in favor of this solution because the landing-pad-presence is a choice of the final-downstream user. It's a little unfortunate that this can only be done at LTO time, but that's also the only time at which we can alter upstream code.

I agree that the only way to truly safely do this is to statically ensure that fail! does not unwind and rather aborts. Right now we have no compile-time checking that the failure function triggers an abort rather than failing. I am a firm believer in that crates should be compiled once, so I should never have to make a decision of calling fail!() vs abort!().

The best way that I can think of doing something like this:

  • If landing pads are generated, then the compiler will link dylibs/binaries to librustunwind which defines two symbols: rust_try and rust_begin_unwind. The compiler will also manually specify -lstdc++ on the link line. Note that this is different than what happens today in that libstd currently specifies the libstdc++ dependency via std::rtdeps. Additionally, librustunwind is currently an object file inside librustrt
  • If landing pads are not generated, then the compiler will instead link to librustnounwind. This library would also define two symbols: rust_try and rust_begin_unwind but the difference would be that rust_begin_unwind just aborts and rust_try just calls the function. The compiler would then have no need to link against libstdc++

If we did something like this, then we would have a static guarantee that code compiled with -Z no-landing-pads would not unwind through the fail!() macro (because no support for it was ever linked in). Additionally, this means that even libstd doesn't have to get recompiled with and without unwinding.

As another consequence of possibly going down this route, if we start making no-landing-pads a first-class option, I would want to make it a crate attribute and have it be a compiler error if an rlib or dylib is being generated. We could explore a method of allowing no-landing-pads for dylib outputs, but this has the same problems of mixing static/dynamic linking in that all upstream dependencies must be tracked.

@pcwalton
Copy link
Contributor

I like this approach because it handles the kernel case quite nicely (as kernels definitely want LTO and probably don't want unwinding). Although if your kernel supports loadable kernel modules then, well, we'll have to come up with something.

When performing LTO, the rust compiler has an opportunity to completely strip
all landing pads in all dependent libraries. I've modified the LTO pass to
recognize the -Z no-landing-pads option when also running an LTO pass to flag
everything in LLVM as nothrow. I've verified that this prevents any and all
invoke instructions from being emitted.

I believe that this is one of our best options for moving forward with
accomodating use-cases where unwinding doesn't really make sense. This will
allow libraries to be built with landing pads by default but allow usage of them
in contexts where landing pads aren't necessary.

cc rust-lang#10780
@alexcrichton
Copy link
Member Author

(I force pushed to remove the 'Closes' part of the commit message)

bors added a commit that referenced this pull request Dec 13, 2013
When performing LTO, the rust compiler has an opportunity to completely strip
all landing pads in all dependent libraries. I've modified the LTO pass to
recognize the -Z no-landing-pads option when also running an LTO pass to flag
everything in LLVM as nothrow. I've verified that this prevents any and all
invoke instructions from being emitted.

I believe that this is one of our best options for moving forward with
accomodating use-cases where unwinding doesn't really make sense. This will
allow libraries to be built with landing pads by default but allow usage of them
in contexts where landing pads aren't necessary.
@bors bors closed this Dec 13, 2013
@bors bors merged commit 667d114 into rust-lang:master Dec 13, 2013
@alexcrichton alexcrichton deleted the nounwind branch December 16, 2013 05:54
Amanieu added a commit to Amanieu/rust that referenced this pull request Jan 14, 2022
This was originally introduced in rust-lang#10916 as a way to remove all landing
pads when performing LTO. However this is no longer necessary today
since rustc properly marks all functions and call-sites as nounwind
where appropriate.

In fact this is incorrect in the presence of `extern "C-unwind"` which
must create a landing pad when compiled with `-C panic=abort` so that
foreign exceptions are caught and properly turned into aborts.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 17, 2022
…k-Simulacrum

Remove LLVMRustMarkAllFunctionsNounwind

This was originally introduced in rust-lang#10916 as a way to remove all landing
pads when performing LTO. However this is no longer necessary today
since rustc properly marks all functions and call-sites as nounwind
where appropriate.

In fact this is incorrect in the presence of `extern "C-unwind"` which
must create a landing pad when compiled with `-C panic=abort` so that
foreign exceptions are caught and properly turned into aborts.
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 30, 2023
…as,xFrednet

New lint [`min_ident_chars`]

Closes rust-lang#10915

This also implements `WithSearchPat` for `Ident`, as I was going to rewrite this as a late lint to optionally ignore fields and/or generics but this was more complex than anticipated

changelog: New lint [`min_ident_chars`]
[rust-lang#10916](rust-lang/rust-clippy#10916)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants