Reject unsupported, unstable ABIs during AST lowering #141877

workingjubilee · 2025-06-02T03:12:49Z

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 HIR¹. 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 three 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

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. ↩

rustbot · 2025-06-02T03:12:54Z

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 · 2025-06-02T03:12:56Z

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.

workingjubilee · 2025-06-02T04:12:48Z

@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 · 2025-06-02T04:56:28Z

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

aDotInTheVoid · 2025-06-02T07:58:56Z

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.

rust/src/librustdoc/json/conversions.rs

Lines 365 to 378 in 91fad92

    
           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()), 
        
               } 
        
           }

rust/src/rustdoc-json-types/lib.rs

Lines 771 to 794 in 91fad92

    
           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 · 2025-06-02T14:09:43Z

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

workingjubilee · 2025-06-02T14:33:41Z

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.

Nadrieril · 2025-06-06T08:39:53Z

I don't know enough about ABI to review this

r? compiler

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.

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.

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.

rustbot assigned Nadrieril Jun 2, 2025

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

This comment has been minimized.

Sign in to view

rustbot added the A-rustdoc-json Area: Rustdoc JSON backend label Jun 2, 2025

workingjubilee force-pushed the wipe-the-extern-ast branch from a705901 to 1fb2cf4 Compare June 2, 2025 14:25

workingjubilee mentioned this pull request Jun 6, 2025

ICE: abi_map: None #142107

Open

rustbot assigned fee1-dead and unassigned Nadrieril Jun 6, 2025

workingjubilee added 6 commits June 6, 2025 11:16

tests: Rework abi_gpu_kernel feature gate test

81ab838

Explicitly test it for relevant targets and check it errors on hosts.

tests: Upgrade unstable ABIs to errors in unsupported ABI test

38f1f24

tests: Update and bless cmse-nonsecure ABI tests

f8b963d

tests: Adopt ABI transmute tests from crashtests

38aac10

workingjubilee force-pushed the wipe-the-extern-ast branch 3 times, most recently from 9aecfc6 to 15a693d Compare June 6, 2025 18:51

workingjubilee added 2 commits June 6, 2025 13:21

tests: Adopt unsupported ABI in weird type impl test

8c89905

workingjubilee force-pushed the wipe-the-extern-ast branch from 15a693d to 8c89905 Compare June 6, 2025 20:22

workingjubilee mentioned this pull request Jun 6, 2025

Reject extern "{abi}" when the target does not support it #142134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Reject unsupported, unstable ABIs during AST lowering #141877

Reject unsupported, unstable ABIs during AST lowering #141877

workingjubilee commented Jun 2, 2025 •

edited

Loading

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

This comment has been minimized.

workingjubilee commented Jun 2, 2025 •

edited

Loading

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

aDotInTheVoid commented Jun 2, 2025

Uh oh!

workingjubilee commented Jun 2, 2025

Uh oh!

workingjubilee commented Jun 2, 2025 •

edited

Loading

Uh oh!

Nadrieril commented Jun 6, 2025

Uh oh!

Uh oh!

Reject unsupported, unstable ABIs during AST lowering #141877

Are you sure you want to change the base?

Reject unsupported, unstable ABIs during AST lowering #141877

Conversation

workingjubilee commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Footnotes

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

This comment has been minimized.

workingjubilee commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jun 2, 2025

Uh oh!

aDotInTheVoid commented Jun 2, 2025

Uh oh!

workingjubilee commented Jun 2, 2025

Uh oh!

workingjubilee commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Nadrieril commented Jun 6, 2025

Uh oh!

Uh oh!

workingjubilee commented Jun 2, 2025 •

edited

Loading

workingjubilee commented Jun 2, 2025 •

edited

Loading

workingjubilee commented Jun 2, 2025 •

edited

Loading