Skip to content

Conversation

@purplesyringa
Copy link
Contributor

@purplesyringa purplesyringa commented Jul 20, 2025

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 #132416.

@rustbot label +T-compiler +A-panic

@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2025

r? @SparrowLii

rustbot has assigned @SparrowLii.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 20, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 20, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

@rustbot rustbot added the A-panic Area: Panicking machinery label Jul 20, 2025
@purplesyringa
Copy link
Contributor Author

purplesyringa commented Jul 20, 2025

I'm not 100% sure there's no subtle issue here, even though throw seems to be the only intrinsic declared with extern "C-unwind" that I could find, so I'm obviously waiting on PR CI here.

@SparrowLii
Copy link
Member

r? @workingjubilee :)

@rustbot rustbot assigned workingjubilee and unassigned SparrowLii Jul 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 22, 2025

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

@nikic
Copy link
Contributor

nikic commented Jul 22, 2025

This looks reasonable to me.

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.

LLVM intrinsics should generally use the "unadjusted" ABI. It looks like we don't have "unadjusted-unwind", but I guess it can be added when needed...

@bors
Copy link
Collaborator

bors commented Jul 22, 2025

☔ 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.
@purplesyringa purplesyringa force-pushed the unwinding-intrinsics branch from 1233757 to ed11a39 Compare July 22, 2025 23:19
@nikic
Copy link
Contributor

nikic commented Jul 24, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 24, 2025

📌 Commit ed11a39 has been approved by nikic

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2025
fmease added a commit to fmease/rust that referenced this pull request Jul 24, 2025
…, 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
bors added a commit that referenced this pull request Jul 24, 2025
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
fmease added a commit to fmease/rust that referenced this pull request Jul 25, 2025
…, 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
bors added a commit that referenced this pull request Jul 25, 2025
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
@matthiaskrgr
Copy link
Member

@bors r-
looks like this failed here #144428 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 25, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2025

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

@purplesyringa
Copy link
Contributor Author

purplesyringa commented Jul 25, 2025

#118168 has this unresolved question:

Should exception-handling instructions (e.g. from prebuilt libstd) be stripped out if panic=abort? If so, by whom? wasm-ld? wasm-opt does currently implement a --strip-eh pass; maybe that's good enough if users want to target wasm MVP?

And there's this snippet in unwind:

cfg_if::cfg_if! {
// panic=abort is default for wasm targets. Because an unknown instruction is a load-time
// error on wasm, instead of a runtime error like on traditional architectures, we never
// want to codegen a `throw` instruction, as that would break users using runtimes that
// don't yet support exceptions. The only time this first branch would be selected is if
// the user explicitly opts in to wasm exceptions, via -Zbuild-std with -Cpanic=unwind.
if #[cfg(panic = "unwind")] {
// corresponds with llvm::WebAssembly::Tag::CPP_EXCEPTION
// in llvm-project/llvm/include/llvm/CodeGen/WasmEHFuncInfo.h
const CPP_EXCEPTION_TAG: i32 = 0;
core::arch::wasm::throw::<CPP_EXCEPTION_TAG>(exception.cast())
} else {
let _ = exception;
core::arch::wasm::unreachable()
}
}

So I think the idea here is that if std is compiled with -C panic=unwind, it cannot currently be linked to -C panic=abort crates on Wasm because we don't have infrastructure to fix that, and that's the reason why we serve pre-compile std with -C panic=abort on Wasm targets. So the lint hits at a real problem, but it's not a new problem, and we're tracking it elsewhere anyway.

I've added a FIXME comment, but otherwise this PR can be merged, I think. I've verified that it doesn't break -C panic=abort std builds.

cc @coolreader18

@rustbot ready

@purplesyringa purplesyringa force-pushed the unwinding-intrinsics branch from eafcb72 to 17519ae Compare July 25, 2025 15:29
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 25, 2025
@nikic
Copy link
Contributor

nikic commented Jul 26, 2025

@bors2 try jobs=dist-various-1

@rust-bors
Copy link

rust-bors bot commented Jul 26, 2025

⌛ Trying commit 17519ae with merge 5319385

To cancel the try build, run the command @bors try cancel.

rust-bors bot added a commit that referenced this pull request Jul 26, 2025
Don't special-case llvm.* as nounwind

try-job: dist-various-1
@rust-bors
Copy link

rust-bors bot commented Jul 26, 2025

☀️ Try build successful (CI)
Build commit: 5319385 (53193856e5a4ac7cbbe00ed3c7f71acb23e65133, parent: 8708f3cd1f96d226f6420a58ebdd61aa0bc08b0a)

@nikic
Copy link
Contributor

nikic commented Jul 27, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 27, 2025

📌 Commit 17519ae has been approved by nikic

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2025
@bors
Copy link
Collaborator

bors commented Jul 27, 2025

⌛ Testing commit 17519ae with merge 2b5e239...

@bors
Copy link
Collaborator

bors commented Jul 28, 2025

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 2b5e239 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2025
@bors bors merged commit 2b5e239 into rust-lang:master Jul 28, 2025
12 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jul 28, 2025
@github-actions
Copy link
Contributor

What is this? This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing f8e355c (parent) -> 2b5e239 (this PR)

Test differences

No test diffs found

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 2b5e239c6b86cde974b0ef0f8e23754fb08ff3c5 --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

  1. x86_64-apple-2: 4780.3s -> 3601.6s (-24.7%)
  2. dist-aarch64-linux: 8092.6s -> 6146.0s (-24.1%)
  3. x86_64-apple-1: 9031.8s -> 6913.5s (-23.5%)
  4. pr-check-2: 2532.3s -> 2142.6s (-15.4%)
  5. dist-aarch64-apple: 5493.3s -> 6107.6s (11.2%)
  6. x86_64-rust-for-linux: 2919.0s -> 2599.7s (-10.9%)
  7. pr-check-1: 1715.5s -> 1535.6s (-10.5%)
  8. x86_64-gnu-llvm-19: 2772.9s -> 2498.1s (-9.9%)
  9. aarch64-apple: 5258.1s -> 5753.1s (9.4%)
  10. i686-gnu-2: 5931.5s -> 5374.0s (-9.4%)
How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2b5e239): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 4.5%, secondary 2.8%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
4.5% [4.4%, 4.7%] 2
Regressions ❌
(secondary)
2.8% [1.1%, 5.2%] 3
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 4.5% [4.4%, 4.7%] 2

Cycles

Results (primary 2.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
2.0% [1.9%, 2.2%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 2.0% [1.9%, 2.2%] 2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 465.942s -> 467.432s (0.32%)
Artifact size: 376.74 MiB -> 376.83 MiB (0.02%)

github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Jul 30, 2025
…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
purplesyringa added a commit to purplesyringa/rust that referenced this pull request Oct 30, 2025
rustc assumes that regular `extern "Rust"` functions unwind only if the
`unwind` panic runtime is linked. `throw` was annotated as such, but
unwound unconditionally. This could cause UB when a crate built with `-C
panic=abort` called `throw` from `core` built with `-C panic=unwind`,
since no terminator was added to handle the panic arising from calling an
allegedly non-unwinding `extern "Rust"` function.

rustc was taught to recognize this condition since
rust-lang#144225 and prevented such
linkage, but this caused regressions in
rust-lang#148246, since this meant that
Emscripten projects could not be built with `-C panic=abort` without
recompiling std.

The most straightforward solution would be to move `throw` into the
`panic_unwind` crate, so that it's only compiled if the panic runtime is
guaranteed to be `unwind`, but this is messy due to our architecture.
Instead, move it into `unwind::wasm`, which is only compiled for
bare-metal targets that default to `panic = "abort"`, rendering the
issue moot.
bors added a commit that referenced this pull request Oct 30, 2025
Move wasm `throw` intrinsic back to `unwind`

Fixes #148246, less invasive than the previously proposed #148269. Removes the publicly visible unstable intrinsic tracked in #122465 since it's not clear how to export it in a sound manner.

r? `@bjorn3`

---

rustc assumes that regular `extern "Rust"` functions unwind only if the `unwind` panic runtime is linked. `throw` was annotated as such, but unwound unconditionally. This could cause UB when a crate built with `-C panic=abort` called `throw` from `core` built with `-C panic=unwind`, since no terminator was added to handle the panic arising from calling an allegedly non-unwinding `extern "Rust"` function.

rustc was taught to recognize this condition since #144225 and prevented such linkage, but this caused regressions in
#148246, since this meant that Emscripten projects could not be built with `-C panic=abort` without recompiling std.

The most straightforward solution would be to move `throw` into the `panic_unwind` crate, so that it's only compiled if the panic runtime is guaranteed to be `unwind`, but this is messy due to our architecture. Instead, move it into `unwind::wasm`, which is only compiled for bare-metal targets that default to `panic = "abort"`, rendering the issue moot.
Kobzol pushed a commit to Kobzol/stdarch that referenced this pull request Nov 2, 2025
rustc assumes that regular `extern "Rust"` functions unwind only if the
`unwind` panic runtime is linked. `throw` was annotated as such, but
unwound unconditionally. This could cause UB when a crate built with `-C
panic=abort` called `throw` from `core` built with `-C panic=unwind`,
since no terminator was added to handle the panic arising from calling an
allegedly non-unwinding `extern "Rust"` function.

rustc was taught to recognize this condition since
rust-lang/rust#144225 and prevented such
linkage, but this caused regressions in
rust-lang/rust#148246, since this meant that
Emscripten projects could not be built with `-C panic=abort` without
recompiling std.

The most straightforward solution would be to move `throw` into the
`panic_unwind` crate, so that it's only compiled if the panic runtime is
guaranteed to be `unwind`, but this is messy due to our architecture.
Instead, move it into `unwind::wasm`, which is only compiled for
bare-metal targets that default to `panic = "abort"`, rendering the
issue moot.
Kobzol pushed a commit to Kobzol/stdarch that referenced this pull request Nov 2, 2025
Move wasm `throw` intrinsic back to `unwind`

Fixes rust-lang/rust#148246, less invasive than the previously proposed rust-lang/rust#148269. Removes the publicly visible unstable intrinsic tracked in rust-lang/rust#122465 since it's not clear how to export it in a sound manner.

r? `@bjorn3`

---

rustc assumes that regular `extern "Rust"` functions unwind only if the `unwind` panic runtime is linked. `throw` was annotated as such, but unwound unconditionally. This could cause UB when a crate built with `-C panic=abort` called `throw` from `core` built with `-C panic=unwind`, since no terminator was added to handle the panic arising from calling an allegedly non-unwinding `extern "Rust"` function.

rustc was taught to recognize this condition since rust-lang/rust#144225 and prevented such linkage, but this caused regressions in
rust-lang/rust#148246, since this meant that Emscripten projects could not be built with `-C panic=abort` without recompiling std.

The most straightforward solution would be to move `throw` into the `panic_unwind` crate, so that it's only compiled if the panic runtime is guaranteed to be `unwind`, but this is messy due to our architecture. Instead, move it into `unwind::wasm`, which is only compiled for bare-metal targets that default to `panic = "abort"`, rendering the issue moot.
folkertdev pushed a commit to folkertdev/stdarch that referenced this pull request Nov 2, 2025
rustc assumes that regular `extern "Rust"` functions unwind only if the
`unwind` panic runtime is linked. `throw` was annotated as such, but
unwound unconditionally. This could cause UB when a crate built with `-C
panic=abort` called `throw` from `core` built with `-C panic=unwind`,
since no terminator was added to handle the panic arising from calling an
allegedly non-unwinding `extern "Rust"` function.

rustc was taught to recognize this condition since
rust-lang/rust#144225 and prevented such
linkage, but this caused regressions in
rust-lang/rust#148246, since this meant that
Emscripten projects could not be built with `-C panic=abort` without
recompiling std.

The most straightforward solution would be to move `throw` into the
`panic_unwind` crate, so that it's only compiled if the panic runtime is
guaranteed to be `unwind`, but this is messy due to our architecture.
Instead, move it into `unwind::wasm`, which is only compiled for
bare-metal targets that default to `panic = "abort"`, rendering the
issue moot.
makai410 pushed a commit to makai410/rustc_public that referenced this pull request Nov 4, 2025
Move wasm `throw` intrinsic back to `unwind`

Fixes rust-lang/rust#148246, less invasive than the previously proposed rust-lang/rust#148269. Removes the publicly visible unstable intrinsic tracked in rust-lang/rust#122465 since it's not clear how to export it in a sound manner.

r? `@bjorn3`

---

rustc assumes that regular `extern "Rust"` functions unwind only if the `unwind` panic runtime is linked. `throw` was annotated as such, but unwound unconditionally. This could cause UB when a crate built with `-C panic=abort` called `throw` from `core` built with `-C panic=unwind`, since no terminator was added to handle the panic arising from calling an allegedly non-unwinding `extern "Rust"` function.

rustc was taught to recognize this condition since rust-lang/rust#144225 and prevented such linkage, but this caused regressions in
rust-lang/rust#148246, since this meant that Emscripten projects could not be built with `-C panic=abort` without recompiling std.

The most straightforward solution would be to move `throw` into the `panic_unwind` crate, so that it's only compiled if the panic runtime is guaranteed to be `unwind`, but this is messy due to our architecture. Instead, move it into `unwind::wasm`, which is only compiled for bare-metal targets that default to `panic = "abort"`, rendering the issue moot.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-attributes Area: Attributes (`#[…]`, `#![…]`) A-panic Area: Panicking machinery merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WASI unwinding is broken in release

8 participants