Skip to content

Remove libstd's calls to C-unwind foreign functions #97033

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

Merged
merged 2 commits into from
May 19, 2022

Conversation

nbdd0121
Copy link
Contributor

Remove all libstd and its dependencies' usage of extern "C-unwind".

This is a prerequiste of a WIP PR which will forbid libraries calling extern "C-unwind" functions to be compiled in -Cpanic=unwind and linked against panic_abort (this restriction is necessary to address soundness bug #96926).
Cargo will ensure all crates are compiled with the same -Cpanic but the std is only compiled -Cpanic=unwind but needs the ability to be linked into -Cpanic=abort.

Currently there are two places where C-unwind is used in libstd:

  • __rust_start_panic is used for interfacing to the panic runtime. This could be extern "Rust"
  • _{rdl,rg}_oom: a shim __rust_alloc_error_handler will be generated by codegen to call into one of these; they can also be extern "Rust" (in fact, the generated shim is used as extern "Rust", so I am not even sure why these are not, probably because they used to extern "C" and was changed to extern "C-unwind" when we allow alloc error hooks to unwind, but they really should just be using Rust ABI).

For dependencies, there is only one extern "C-unwind" function call, in unwind crate. This can be expressed as a re-export.

More dicussions can be seen in the Zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/210922-project-ffi-unwind/topic/soundness.20in.20mixed.20panic.20mode

@rustbot label: T-libs F-c_unwind

nbdd0121 added 2 commits May 14, 2022 02:53
This ensures that there are no calls to `C-unwind` function in libunwind.
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 14, 2022
@rust-highfive
Copy link
Contributor

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the F-c_unwind `#![feature(c_unwind)]` label May 14, 2022
@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 14, 2022
@bjorn3
Copy link
Member

bjorn3 commented May 14, 2022

These all depend on the rust abi happening to match up with the abi rustc expects for the respective functions. Rustc generates shim functions calling or defining them which don't go through the standard abi handling code.

@nbdd0121
Copy link
Contributor Author

These all depend on the rust abi happening to match up with the abi rustc expects for the respective functions. Rustc generates shim functions calling or defining them which don't go through the standard abi handling code.

Well, given rustc_codegen_llvm/allocator.rs actually uses the same LLVM type for the function definition and the call, we already require and expect the ABI to match up anyway.

@Mark-Simulacrum
Copy link
Member

r? @Amanieu perhaps?

This seems OK, but I'd like another set of eyes on it at least. In general the restrictions / unsoundness from mixing panic={abort,unwind} and extern "C" and "C-unwind" feel pretty worrisome to me, but it may be non-avoidable unfortunately. It sounds like in practice this comes down to std being wrongly compiled in panic=unwind mode when used by panic=abort crates, so it may be that we should be investing in making that not the case -- it seems like it shouldn't be impossible for us to distribute two copies of std, though it would come at unfortunate increase in Rust install sizes probably.

So maybe this approach is best. I do think we need the lints referenced in the description to make it viable, though.

@Amanieu
Copy link
Member

Amanieu commented May 16, 2022

What about _Unwind_RaiseException? It's still defined as "C-unwind" and is called by the standard library via the panic-unwind runtime.

@nbdd0121
Copy link
Contributor Author

The panic-unwind crate is not a dependency of std but instead injected by the compiler, so it's fine. We already have protection against linking both panic-unwind and panic-abort together.

@Amanieu
Copy link
Member

Amanieu commented May 16, 2022

On its own this change is fine. Whether it is sufficient will be clearer once the followup PR is ready.

@bors r+

@bors
Copy link
Collaborator

bors commented May 16, 2022

📌 Commit fbb3c19 has been approved by Amanieu

@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 May 16, 2022
@bors
Copy link
Collaborator

bors commented May 19, 2022

⌛ Testing commit fbb3c19 with merge 50872bd...

@bors
Copy link
Collaborator

bors commented May 19, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 50872bd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2022
@bors bors merged commit 50872bd into rust-lang:master May 19, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 19, 2022
@rust-timer
Copy link
Collaborator

rust-timer commented May 19, 2022

Finished benchmarking commit (50872bd): comparison url.

Summary:

Instruction count

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

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 🎉 relevant improvements found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 0 0 6 0
mean2 N/A N/A N/A -2.3% N/A
max N/A N/A N/A -4.0% N/A

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 2 2 1 0 3
mean2 2.4% 3.1% -2.3% N/A 0.8%
max 2.6% 4.5% -2.3% N/A 2.6%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes 2

  2. the arithmetic mean of the percent change 2

@RalfJung
Copy link
Member

The follow-up is ready at #97235

@nbdd0121 nbdd0121 deleted the unwind3 branch June 8, 2022 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-c_unwind `#![feature(c_unwind)]` 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-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants