Skip to content

Reject unsupported extern "{abi}"s consistently in all positions #142134

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 13 commits into
base: master
Choose a base branch
from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Jun 6, 2025

Modify the handling of extern "{abi}" in the compiler so that it has consistent errors without regard to the position in the grammar.

What

Implement the following breakages:

  • Promote unsupported_fn_ptr_calling_conventions from a warning to a hard error
  • Guarantee future edge-cases like trait declarations will not escape so that ABIs that have already been unusable in most positions will now be unusable in all positions. See "How" and "Why or Why Not" for more details.

In particular, these architecture-specific ABIs now only compile on their architectures1:

  • amdgpu: "gpu-kernel"
  • arm: "aapcs", "C-cmse-nonsecure-entry", "C-cmse-nonsecure-call"
  • avr: "avr-interrupt", "avr-non-blocking-interrupt"
  • msp430: "msp430-interrupt"
  • nvptx64: "gpu-kernel", "ptx-kernel"
  • riscv32 and riscv64: "riscv-interrupt-m", "riscv-interrupt-s"
  • x86: "thiscall"
  • x86 and x86_64: "x86-interrupt"
  • x86_64: "sysv64", "win64"

ABIs that are logically x86-specific but actually permitted on all Windows targets remain permitted on Windows, as before. For non-Windows targets, they error if we had previously done so in other positions.

How

We modify rustc_ast_lowering to prevent unsupported ABIs from leaking through the HIR without being checked for target support to emit hard errors for every case where we would return Invalid from AbiMap::canonize_abi. 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. The code is also simpler compared to some cases which split on unstable versus stable, only suffering some unavoidable complication to support the newly-revived unsupported_calling_conventions lint.2

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:

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, silently3. However, this obviously exposes Rust to errors in codegen. We cannot lower directly to the "obvious", target-incorrect ABI and then trust code generators like LLVM to reliably error on these cases, either.

Other considerations include:

  • We could refactor the compiler to defer ABI computations, but that seems like it would suffer the "whack-a-mole" problem close to linking instead of after parsing and expansion.
  • We could run a deprecation cycle for the edge cases, but we would be warning highly marginal cases, like this trait declaration without a definition that cannot be implemented without error4.
pub trait UsedToSneakBy {
    pub extern "gpu-kernel" fn sneaky();
}
  • The crater run on this PR's draft form suggests the primary issue is with implementations on function pointers, which has already been warned on, so it does not seem like we would be benefiting any real code.
  • Converting this to a hard error now, in the same cycle that we ship the reanimation of the unsupported_calling_conventions lint, means people who would otherwise have to deal with two lints only have to update their code in one batch. Of course, one of them is as breakage.

r? lang

Fixes #86232
Fixes #132430
Fixes #138738
Fixes #142107

Footnotes

  1. Some already will not compile, due to reaching ICEs or LLVM errors.

  2. That lint cannot be moved in a similar way yet because lints operate on HIR, so you cannot emit lints when the HIR has not been completely formed.

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

  4. 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 for anything with function pointers. Thus we would be discussing deprecation cycles for code that is impotent or forewarned5.

  5. 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 are being excised in 643a9d2

@rustbot rustbot added 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 6, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 6, 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 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

@workingjubilee
Copy link
Member Author

@bors2 try

@rust-bors
Copy link

rust-bors bot commented Jun 6, 2025

⌛ Trying commit c1db989 with merge 8b8eff5

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

rust-bors bot added a commit that referenced this pull request Jun 6, 2025
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.
@rust-bors
Copy link

rust-bors bot commented Jun 7, 2025

☀️ Try build successful (CI)
Build commit: 8b8eff5 (8b8eff55bd72abbb57167bc42222a7f91d41cb0d)

@workingjubilee
Copy link
Member Author

@craterbot run mode=build-only name=pr-142134-abi-ast-error cap-lints=warn

@craterbot
Copy link
Collaborator

🚧 Experiment pr-142134-abi-ast-error is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

👌 Experiment pr-142134-abi-ast-error created and queued.
🤖 Automatically detected try build 8b8eff5
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2025
@craterbot
Copy link
Collaborator

🎉 Experiment pr-142134-abi-ast-error is completed!
📊 57 regressed and 16 fixed (643383 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 8, 2025
@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 8, 2025

I'll send the necessary PRs to size-of and retour.

The primary culprit seems to be impls on function pointers, which are already warned on: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=e1e8ca91878cbbcba5c6dc3b064cea2b

on retour

I should note that while impls on function pointers are convenient, it's not clear that code that works at the low level that retour does can be correct when interacting with these function pointers unless it is written with full awareness that the relevant ABIs are translated to extern "C". Some simple versions, maybe, but often detouring libraries have to make assumptions about codegen, and consistently getting that correct seems unlikely. So I believe enforcing this lint in a now-absolutist fashion will, in fact, make the world's code more correct.

on size_of

For size_of the upgrade to warn-on-deps seems to have attracted notice: Kixiron/size-of#6

I was able to contact the maintainer who is happy to run a release once I draft a PR that fixes this.

on the rest

The "regressions" for libc, serde_derive, etc. all appear to be spurious failures due to build space running out.

@bors
Copy link
Collaborator

bors commented Jun 9, 2025

☔ The latest upstream changes (presumably #141435) made this pull request unmergeable. Please resolve the merge conflicts.

@workingjubilee workingjubilee force-pushed the reject-unsupported-abi branch from c1db989 to 9981fee Compare June 9, 2025 17:17
@workingjubilee workingjubilee changed the title Reject extern "{abi}" when the target does not support it Reject extern "{abi}" consistently in all positions Jun 9, 2025
@workingjubilee workingjubilee changed the title Reject extern "{abi}" consistently in all positions Reject unsupported extern "{abi}"s consistently in all positions Jun 9, 2025
@rust-log-analyzer

This comment has been minimized.

@workingjubilee workingjubilee force-pushed the reject-unsupported-abi branch 2 times, most recently from 1595699 to 4306efa Compare June 9, 2025 23:09
@RalfJung
Copy link
Member

Fixes #86232

So what does this do with invalid ABIs in traits? I can't see a new lint being introduced here -- but making this a hard error immediately would be a breaking change, no?


impl Test {
pub extern "gpu-kernel" fn test(val: &str) {}
//~^ ERROR [E0570]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error codes are pretty meaningless, making the test file hard to read. IMO it'd be better to have a snippet of the error message in the test file here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these tests seem to assume that "gpu-kernel" is not a valid ABI. But that's target-specific, so this will fail when the test suite is run on a GPU target, right?

Like the other unsupported ABI tests, I think this should use --target and #![no_core] (ideally with minicore) so that the test does not depend on what the host target happens to be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I mean technically GPUs shouldn't ever be hosts so I wouldn't think we would build tests for them, but then Aaron Hsu's codfns compiler exists so maybe I shouldn't think that.

@workingjubilee
Copy link
Member Author

So what does this do with invalid ABIs in traits? I can't see a new lint being introduced here -- but making this a hard error immediately would be a breaking change, no?

Correct.

My proposal to lang is that they should consider it a functional non-event, because it truly seems to be. All the crater regressions are from doing things with function pointers, not unimplementable associated functions of traits. We have been writing these lints and missing cases every time, even though we thought we covered our bases last time. I think we should cut out the problem at the root. If they think we should write another lint, that's... probably fine, actually, but how many more times are we going to do this? I'd like to have a commitment to break; when the loop counter reaches some limit, even if we haven't done everything perfectly, before we go further.

Obviously, I think 0 would be a reasonable number of additional repetitions, but there's other integers I could see myself happy with.

@rustbot label: I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jun 10, 2025
@RalfJung
Copy link
Member

We have been writing these lints and missing cases every time, even though we thought we covered our bases last time.

Did we? #86232 was very clear about affecting traits and fn ptrs, and the fn ptr lint is very clear about only checking fn ptrs. I don't think anyone was confused about the trait case, it's just that so far nobody felt like implementing the lint for that.

err.emit();
}
// already erred in rustc_ast_lowering
AbiMapping::Invalid => (),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the history of leaky checks, maybe add a delay_span_bug here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some trait declarations here? That can replace at least one of your new tests, too.

));
}
}

pub fn check_abi(tcx: TyCtxt<'_>, hir_id: hir::HirId, span: Span, abi: ExternAbi) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand this PR correctly, we still do not call this function for trait declarations. This means that UNSUPPORTED_CALLING_CONVENTIONS is not emitted for them, and so moving cdecl and friends to Invalid will be another sudden breaking change with no prior lint. Is that right?

How hard would it be to find a place in hir_analysis (or wherever) to call this on trait declarations, too?

@workingjubilee
Copy link
Member Author

Did we? #86232 was very clear about affecting traits and fn ptrs, and the fn ptr lint is very clear about only checking fn ptrs. I don't think anyone was confused about the trait case, it's just that so far nobody felt like implementing the lint for that.

"Nobody felt like it" is kind of the problem.

I have been triaging issues up and down the issue tracker for years, spending large amounts of time tagging and sorting issues, and increasingly trying to focus on understanding and collecting all the ones specifically related to ABI. Talking with you, even!... and ~last week was the first time I saw it. Even the people who know about it have bigger fish to fry. There is exactly zero code on crater that is affected by the edge-case breakage in practice. Usually, even the most obscure breakage at least hits some random unfinished project on GitHub, and then we decide whether to write off some abandoned code from 2017 or not, but the breakage I saw was purely about function pointers.

If people insist on a performance then I will be their Pierrot, but not before the ringmaster directs me. They know well I have more interesting acts I could be rehearsing.

@RalfJung
Copy link
Member

RalfJung commented Jun 10, 2025

0 crater regressions is a good sign, I agree. Crater doesn't see all code though, and it only builds for Linux (which is quite relevant when talking about target-specific behavior such as this).

@bors
Copy link
Collaborator

bors commented Jun 10, 2025

☔ The latest upstream changes (presumably #142299) made this pull request unmergeable. Please resolve the merge conflicts.

workingjubilee and others added 13 commits June 10, 2025 14:54
We modify rustc_ast_lowering to prevent all 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`.
This FCW was upgraded to warn in dependencies in Rust 1.87 which was
released on 2025 May 17, and it is now 2025 June, so it has become
active within a stable Rust version.

As we already had errored on these ABIs in most other positions, and
have warned for fn ptrs, this breakage has had reasonable foreshadowing.

However, this does cause errors for usages of `extern "{abi}"` that were
theoretically writeable within source but could not actually be applied
in any useful way by Rust programmers without either warning or error.
For instance, trait declarations without impls were never checked.
These are the exact kinds of leakages that this new approach prevents.

A deprecation cycle is not useful for these marginal cases as upon impl,
even default impls within traits, different HIR objects would be used.
Details of our HIR analysis meant that those objects did get checked.
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 workingjubilee force-pushed the reject-unsupported-abi branch from 6289fce to fa0169e Compare June 10, 2025 22:09
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 I-lang-nominated Nominated for discussion during a lang team meeting. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
7 participants