Skip to content

Reject unsupported, unstable ABIs during AST lowering #141877

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Jun 2, 2025

Currently, we check whether ABIs have support on the target we are compiling for during HIR analysis. This unfortunately is a slightly "whack-a-mole" proposition, because there is no inherent relationship between ABI strings and their usages in the HIR1. We can instead check for support in rustc_ast_lowering, where we first materialize ExternAbi from strings. This lets us always error on ABIs we wish to error on.

As a first step, we throw the switch for unstable ABIs. Despite being nightly features, we actually have emitted future compatibility warnings for all unsupported ABIs for some time:

  • unsupported_calling_conventions became a hard error.
  • unsupported_fn_ptr_calling_conventions is warn-in-deps since Rust 1.87, a stable Rust version.

With this we effectively make the latter lint a hard error on unstable ABIs. Stable ABIs still only receive warnings.

As a bonus, this addresses two ICEs that the compiler could reach. It prevents many more LLVM errors from being seen by programmers as well, as most unstable ABIs cannot have correct code generated for them by targets that do not support the ABI.

Fixes #132430
Fixes #138738

Footnotes

  1. For example, we already missed some use-sites of ABI strings because FnSigs are not BareFnTys, requiring us to implement a new future-compat-warning to handle the function pointer case.

We elaborate rustc_ast_lowering to prevent unstable, 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, unstable ABI strings will now hard-error instead of
triggering the FCW `unsupported_fn_ptr_calling_conventions`.
This FCW was upgraded to warn in dependencies in Rust 1.87 so it has
become active within a stable Rust version and it was within the
compiler's nightly-feature contract to break this code without warning,
so this breakage has likely had a reasonable amount of foreshadowing.
We already gated unstable, unsupported ABIs during AST lowering.
If we check all ABIs for target support during HIR lowering, then we
risk duplicate errors in code that uses unstable extern "{abi}"s.

Limiting the ABI support checks to stable ABIs reduces this, and
remains correct because of gating all unstable ABIs already.
This code complication can be reduced when we switch the FCW to error.
Explicitly test it for relevant targets and check it errors on hosts.
@rustbot
Copy link
Collaborator

rustbot commented Jun 2, 2025

r? @Nadrieril

rustbot has assigned @Nadrieril.
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 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 Jun 2, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 2, 2025

HIR ty lowering was modified

cc @fmease

This PR changes a file inside tests/crashes. If a crash was fixed, please move into the corresponding ui subdir and add 'Fixes #' to the PR description to autoclose the issue upon merge.

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 2, 2025

@aDotInTheVoid Mm. I... am just gonna remove the extern "vectorcall" instances from these tests if that's alright? I don't see what useful coverage you're getting by testing for them if you already have e.g. extern "system" covered?

@rustbot
Copy link
Collaborator

rustbot commented Jun 2, 2025

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

  • It's intentional.
  • The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

@rustbot rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Jun 2, 2025
@aDotInTheVoid
Copy link
Member

I don't see what useful coverage you're getting by testing for them if you already have e.g. extern "system" covered?

It's checking that we produces the rustdoc_json_types::Abi::Other variant for ABI's that don't have a dedicated variant in rustdoc-json.

fn convert_abi(a: ExternAbi) -> Abi {
match a {
ExternAbi::Rust => Abi::Rust,
ExternAbi::C { unwind } => Abi::C { unwind },
ExternAbi::Cdecl { unwind } => Abi::Cdecl { unwind },
ExternAbi::Stdcall { unwind } => Abi::Stdcall { unwind },
ExternAbi::Fastcall { unwind } => Abi::Fastcall { unwind },
ExternAbi::Aapcs { unwind } => Abi::Aapcs { unwind },
ExternAbi::Win64 { unwind } => Abi::Win64 { unwind },
ExternAbi::SysV64 { unwind } => Abi::SysV64 { unwind },
ExternAbi::System { unwind } => Abi::System { unwind },
_ => Abi::Other(a.to_string()),
}
}

pub enum Abi {
// We only have a concrete listing here for stable ABI's because there are so many
// See rustc_ast_passes::feature_gate::PostExpansionVisitor::check_abi for the list
/// The default ABI, but that can also be written explicitly with `extern "Rust"`.
Rust,
/// Can be specified as `extern "C"` or, as a shorthand, just `extern`.
C { unwind: bool },
/// Can be specified as `extern "cdecl"`.
Cdecl { unwind: bool },
/// Can be specified as `extern "stdcall"`.
Stdcall { unwind: bool },
/// Can be specified as `extern "fastcall"`.
Fastcall { unwind: bool },
/// Can be specified as `extern "aapcs"`.
Aapcs { unwind: bool },
/// Can be specified as `extern "win64"`.
Win64 { unwind: bool },
/// Can be specified as `extern "sysv64"`.
SysV64 { unwind: bool },
/// Can be specified as `extern "system"`.
System { unwind: bool },
/// Any other ABI, including unstable ones.
Other(String),
}

Could you instead change the tests to use some other unstable ABI here? But it's fine to only test this once (with both the -unwind and not cases), the current thing of testing every kind of abi everywhere an abi can appear is kind of excessive and provides very little value.

@workingjubilee
Copy link
Member Author

@aDotInTheVoid Well. Most other unstable ABIs don't have "unwind" cases.

We move the vectorcall ABI tests into their own file which is now
only run on x86-64, while replacing them with rust-cold ABI tests
so that aarch64 hosts continue to test an unstable ABI.

A better solution might be cross-compiling or something but
I really don't have time for that right now.
@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 2, 2025

I have pushed an alternative which does some rearrangement so that we don't have complications due to aarch64 hosts, still test an unstable ABI, and retain the vectorcall-unwind coverage when we do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-json Area: Rustdoc JSON backend 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.
Projects
None yet
5 participants