-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Add (back) unsupported_calling_conventions
lint to reject more invalid calling conventions
#141435
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?
Add (back) unsupported_calling_conventions
lint to reject more invalid calling conventions
#141435
Conversation
These commits modify compiler targets. |
f4146aa
to
b8f4367
Compare
@rust-lang/lang following positive vibes expressed here, I'd like to land this PR as a first step to implement the plan from that issue. Can we get this FCPd? :) The one potential open question here is whether this should be a regular FCW first or whether it can immediately become an FCW that shows up in dependencies. I assume stdcall and fastcall are sufficiently rare that we can go for report-in-deps immediately (since they already get rejected on non-x86-32 non-Windows targets). "cdecl" might be more widely used though so it may be worth ramping up the lint more slowly. |
Searching across GitHub, there are 506 Rust file hits on Regarding whether to fire the FCW in deps immediately, I lean toward it probably being OK. Given that this may show up in |
Oh that's the syntax for doing such a search... I tried and failed to get a similar result out of github search.^^ |
unsupported_calling_conventions
lint to reject more invalid calling conventions
Let's propose to do it. @rfcbot fcp merge |
Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
@rfcbot reviewed |
@rfcbot reviewed I think, in the case of
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Somewhat unscientifically skimming the searches, I notice:
So I agree that we should probably offer suggestions for migrating |
b73f7b4
to
cf20f7b
Compare
I added targeted help messages for "stdcall" and "cdecl". I made them also show up for the hard error since they should be helpful there too, e.g. when someone tries to use "stdcall" on x86_64-Linux. |
cf20f7b
to
0d3a7bb
Compare
r? @workingjubilee maybe, feel free to reassign |
b2c41cd
to
2065d08
Compare
This comment has been minimized.
This comment has been minimized.
deba33e
to
18ee895
Compare
This comment has been minimized.
This comment has been minimized.
18ee895
to
d12e2c3
Compare
☔ The latest upstream changes (presumably #141984) made this pull request unmergeable. Please resolve the merge conflicts. |
9961044
to
754cfba
Compare
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
754cfba
to
ef6e711
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RalfJung I have taken the liberty of rebasing the PR so that you do not have to wade through doing so. I hope this is more help than nuisance.
Thanks for rebasing. :) |
…d calling conventions
ef6e711
to
0aed876
Compare
also unify error messages that do not seem to have a good reason to be different
0aed876
to
207b14a
Compare
Reject `extern "{abi}"` when the target does not support it ## What Promote [`unsupported_fn_ptr_calling_conventions`] from a warning to a hard error, making sure edge-cases will not escape. We now emit hard errors for every case we would return `Invalid` from `AbiMap::canonize_abi` during AST to HIR lowering. In particular, these architecture-specific ABIs now only compile on their architectures[^1]: - amdgpu: "gpu-kernel" - arm: "aapcs", "C-cmse-nonsecure-entry" - avr: "avr-interrupt", "avr-non-blocking-interrupt" - msp430: "msp430-interrupt" - nvptx64: "gpu-kernel", "ptx-kernel" - riscv32 and riscv64: "riscv-interrupt-machine", "riscv-interrupt-supervisor" - x86: "thiscall" - x86 and x86_64: "x86-interrupt" - x86_64: "sysv64", "win64" The panoply of ABIs that are logically x86-specific but actually permitted on all Windows targets remain supported on Windows, as they were before. For non-Windows targets they error if the architecture does not match. Moving the check into AST lowering **is itself a breaking change in rare cases**, above and beyond the cases rustc currently warns about. See "Why or Why Not" for details. ## How We modify rustc_ast_lowering to prevent unsupported ABIs from leaking through the HIR without being checked for target support. Previously ad-hoc checking on various HIR items required making sure we check every HIR item which could contain an `extern "{abi}"` string. This is a losing proposition compared to gating the lowering itself. As a consequence, unsupported ABI strings will now hard-error instead of triggering the FCW `unsupported_fn_ptr_calling_conventions`. However, per #86232 this does cause errors for rare usages of `extern "{abi}"` that were theoretically possible to write in Rust source, without previous warning or error. For instance, trait declarations without impls were never checked. These are the exact kinds of leakages that this new approach prevents. This differs from the following PRs: - #141435 is orthogonal, as it adds a new lint for ABIs we have not warned on and are not touched by this PR - #141877 is subsumed by this, in that this simply cuts out bad functionality instead of adding epicycles for stable code ## Why or Why Not We already made the decision to issue the `unsupported_fn_ptr_calling_conventions` future compatibility warning. It has warned in dependencies since #135767, which reached stable with Rust 1.87. That was released on 2025 May 17, and it is now June. As we already had erred on these ABI strings in most other positions, and warn on stable for function pointer types, this breakage has had reasonable foreshadowing. Upgrading the warning to an error addresses a real problem. In some cases the Rust compiler can attempt to actually compute the ABI for calling a function. We could accept this case and compute unsupported ABIs according to some other ABI, silently[^0]. However, this obviously exposes Rust to errors in codegen. We cannot lower directly to the "obvious" ABI and then trust code generators like LLVM to reliably error on these cases, either. Refactoring the compiler so we could defer more ABI computations would be possible, but seems weakly motivated. Even if we succeeded, we would at minimum risk: - exposing the "whack-a-mole" problem but "approaching linking" instead of "leaving AST" - making it harder to reason about functions we *can* lower further - complicating the compiler for no clear benefit A deprecation cycle for the edge-cases could be implemented first, but it is not very useful for such marginal cases, like this trait declaration without a definition: ```rust pub trait UsedToSneakBy { pub extern "gpu-kernel" fn sneaky(); } ``` Upon any impl, even for provided fn within trait declarations, e.g. `pub extern "gpu-kernel" fn sneaky() {}`, different HIR types were used which would, in fact, get checked. Likewise with anything with function pointers. Thus we would be discussing deprecation cycles for code that is impotent or forewarned[^2]. Implementing a deprecation cycle _is_ possible, but it would likely require emitting multiple of a functionally identical warning or error on code that would not have multiple warnings or errors before. It is also not clear to me we would not find **another**, even more marginal edge-case that slipped through, as "things slip through" is the motivation for checking earlier. Additional effort spent on additional warnings should require committing to a hard limit first. r? lang Fixes #86232 Fixes #132430 Fixes #138738 Fixes #142107 [`unsupported_fn_ptr_calling_conventions`]: #130260 [^1]: Some already will not compile, due to reaching ICEs or LLVM errors. [^0]: We already do this for all `AbiStr` we cannot parse, pretending they are `ExternAbi::Rust`, but we also emit an error to prevent reaching too far into codegen. [^2]: It actually did appear in two cases in rustc's test suite because we are a collection of Rust edge-cases by the simple fact that we don't care if the code actually runs. These cases were excised in c1db989.
This adds back the
unsupported_calling_conventions
lint that was removed in #129935, in order to start the process of dealing with #137018. Specifically, we are going for the plan laid out here:The difference to the status quo is that:
Vectorcall is an unstable ABI so we can just make this a hard error immediately. The others are stable, so we emit the
unsupported_calling_conventions
forward-compat lint. I set up the lint to show up in dependencies via cargo's future-compat report immediately, but we could also make it show up just for the local crate first if that is preferred.