-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Don't special-case llvm.* as nounwind #144225
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
base: master
Are you sure you want to change the base?
Don't special-case llvm.* as nounwind #144225
Conversation
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs |
I'm not 100% sure there's no subtle issue here, even though |
r? @workingjubilee :) |
|
This looks reasonable to me.
LLVM intrinsics should generally use the |
☔ The latest upstream changes (presumably #144249) made this pull request unmergeable. Please resolve the merge conflicts. |
Certain LLVM intrinsics, such as `llvm.wasm.throw`, can unwind. Marking them as nounwind causes us to skip cleanup of locals and optimize out `catch_unwind` under inlining or when `llvm.wasm.throw` is used directly by user code. The motivation for forcibly marking llvm.* as nounwind is no longer present: most intrinsics are linked as `extern "C"` or other non-unwinding ABIs, so we won't codegen `invoke` for them anyway.
1233757
to
ed11a39
Compare
@bors r+ |
…, r=nikic Don't special-case llvm.* as nounwind Certain LLVM intrinsics, such as `llvm.wasm.throw`, can unwind. Marking them as nounwind causes us to skip cleanup of locals and optimize out `catch_unwind` under inlining or when `llvm.wasm.throw` is used directly by user code. The motivation for forcibly marking llvm.* as nounwind is no longer present: most intrinsics are linked as `extern "C"` or other non-unwinding ABIs, so we won't codegen `invoke` for them anyway. Closes rust-lang#132416. `@rustbot` label +T-compiler +A-panic
Rollup of 16 pull requests Successful merges: - #142569 (Suggest clone in user-write-code instead of inside macro) - #143401 (tests: Don't check for self-printed output in std-backtrace.rs test) - #143424 (clippy fix: rely on autoderef) - #143970 (Update core::mem::copy documentation) - #143979 (Test fixes for Arm64EC Windows) - #144160 (tests: debuginfo: Work around or disable broken tests on powerpc) - #144200 (Tweak output for non-`Clone` values moved into closures) - #144209 (Don't emit two `assume`s in transmutes when one is a subset of the other) - #144225 (Don't special-case llvm.* as nounwind) - #144314 (Hint that choose_pivot returns index in bounds) - #144316 (bootstrap: Move musl-root fallback out of sanity check) - #144364 (Update `dlmalloc` dependency of libstd) - #144368 (resolve: Remove `Scope::CrateRoot`) - #144373 (remove deprecated Error::description in impls) - #144390 (Remove dead code and extend test coverage and diagnostics around it) - #144392 (rustc_public: Remove movability from `RigidTy/AggregateKind::Coroutine`) r? `@ghost` `@rustbot` modify labels: rollup
…, r=nikic Don't special-case llvm.* as nounwind Certain LLVM intrinsics, such as `llvm.wasm.throw`, can unwind. Marking them as nounwind causes us to skip cleanup of locals and optimize out `catch_unwind` under inlining or when `llvm.wasm.throw` is used directly by user code. The motivation for forcibly marking llvm.* as nounwind is no longer present: most intrinsics are linked as `extern "C"` or other non-unwinding ABIs, so we won't codegen `invoke` for them anyway. Closes rust-lang#132416. ``@rustbot`` label +T-compiler +A-panic
Rollup of 17 pull requests Successful merges: - #142569 (Suggest clone in user-write-code instead of inside macro) - #143401 (tests: Don't check for self-printed output in std-backtrace.rs test) - #143424 (clippy fix: rely on autoderef) - #143970 (Update core::mem::copy documentation) - #143979 (Test fixes for Arm64EC Windows) - #144160 (tests: debuginfo: Work around or disable broken tests on powerpc) - #144200 (Tweak output for non-`Clone` values moved into closures) - #144209 (Don't emit two `assume`s in transmutes when one is a subset of the other) - #144225 (Don't special-case llvm.* as nounwind) - #144314 (Hint that choose_pivot returns index in bounds) - #144316 (bootstrap: Move musl-root fallback out of sanity check) - #144340 (UI test suite clarity changes: Rename `tests/ui/SUMMARY.md` and update rustc dev guide on `error-pattern`) - #144364 (Update `dlmalloc` dependency of libstd) - #144368 (resolve: Remove `Scope::CrateRoot`) - #144390 (Remove dead code and extend test coverage and diagnostics around it) - #144392 (rustc_public: Remove movability from `RigidTy/AggregateKind::Coroutine`) - #144424 (Allow setting `release-blog-post` label with rustbot) r? `@ghost` `@rustbot` modify labels: rollup
@bors r- |
cc @Amanieu, @folkertdev, @sayantn |
That's fun... looks like the lint was always meant to trigger on that code but only started doing it now due to this bug. @rustbot ready |
I'm not sure I fully understand the implications of suppressing the lint. The docs say:
|
I agree that it's strange and suppressing the lint doesn't strike me as right either. I had assumed that since @rustbot author |
Reminder, once the PR becomes ready for a review, use |
#118168 has this unresolved question:
And there's this snippet in rust/library/unwind/src/wasm.rs Lines 48 to 63 in a955f1c
So I think the idea here is that if I've added a FIXME comment, but otherwise this PR can be merged, I think. I've verified that it doesn't break @rustbot ready |
eafcb72
to
17519ae
Compare
Certain LLVM intrinsics, such as
llvm.wasm.throw
, can unwind. Marking them as nounwind causes us to skip cleanup of locals and optimize outcatch_unwind
under inlining or whenllvm.wasm.throw
is used directly by user code.The motivation for forcibly marking llvm.* as nounwind is no longer present: most intrinsics are linked as
extern "C"
or other non-unwinding ABIs, so we won't codegeninvoke
for them anyway.Closes #132416.
@rustbot label +T-compiler +A-panic