Skip to content

Error on invalid signatures for interrupt ABIs #142633

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 1 commit into from
Jun 25, 2025

Conversation

folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Jun 17, 2025

We recently added extern "custom", which must have type fn(). The various extern "interrupt" ABIs impose similar constraints on the signature of functions with that ABI: x86-interrupt should not have a return type (linting on the exact argument types is left as future work), and the other interrupt ABIs cannot have any parameters or a return type.

r? @workingjubilee

Closes #132841

@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 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 17, 2025

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.

@folkertdev folkertdev force-pushed the interrupt-abi-restrict-signature branch from 6c041ae to 6086486 Compare June 17, 2025 17:49
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

pass of style nits, will doublecheck more closely a bit later, you can apply them now or wait until second pass

@folkertdev folkertdev force-pushed the interrupt-abi-restrict-signature branch from 6086486 to 411fce5 Compare June 18, 2025 08:47
@bors
Copy link
Collaborator

bors commented Jun 21, 2025

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

@folkertdev folkertdev force-pushed the interrupt-abi-restrict-signature branch from 411fce5 to aec1f72 Compare June 21, 2025 15:57
@folkertdev folkertdev force-pushed the interrupt-abi-restrict-signature branch from aec1f72 to 32fbbc2 Compare June 21, 2025 19:59

ast_passes_abi_must_not_have_return_type=
invalid signature for `extern {$abi}` function
.note = functions with the `"custom"` ABI cannot have a return type
Copy link
Member

Choose a reason for hiding this comment

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

leftover

Suggested change
.note = functions with the `"custom"` ABI cannot have a return type
.note = functions with the "custom" ABI cannot have a return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, I left those unchanged easier but we can fold that into this PR. Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, was this not new? GH UI may have fooled me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm maybe this one was? but some others in that file were older. Anyway, it's all fixed now.

// An `extern "custom"` function cannot be `async` and/or `gen`.
self.check_abi_is_not_coroutine(abi, sig);

// An `extern "custom"` function must have type `fn()`.
Copy link
Member

Choose a reason for hiding this comment

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

pedantic note: its type is unsafe extern "custom" fn(). and yes, you can have different implementations on unsafe fn() and fn().

don't actually change anything, I just couldn't resist noting.

/// type.
fn check_custom_abi(&self, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) {
/// Check that this function does not violate the constraints of its ABI.
fn check_abi(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) {
Copy link
Member

Choose a reason for hiding this comment

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

I find these check_this names to be slightly nondescript.

  1. sometimes they warn. sometimes they error.
  2. we specifically check only the signature.
Suggested change
fn check_abi(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) {
fn reject_invalid_abi_sig(&self, abi: ExternAbi, ctxt: FnCtxt, ident: &Ident, sig: &FnSig) {

Copy link
Member

Choose a reason for hiding this comment

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

Could also be, at least, check_extern_fn_sig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some renaming. Most functions in that module do start with check_ so that is what I followed.

@bors
Copy link
Collaborator

bors commented Jun 24, 2025

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

@folkertdev folkertdev force-pushed the interrupt-abi-restrict-signature branch from 7d6e5aa to 943d379 Compare June 24, 2025 12:40
@workingjubilee
Copy link
Member

hell yeah.

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 24, 2025

📌 Commit 943d379 has been approved by workingjubilee

It is now in the queue for this repository.

@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 Jun 24, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 25, 2025
…signature, r=workingjubilee

Error on invalid signatures for interrupt ABIs

We recently added `extern "custom"`, which must have type `fn()`. The various `extern "interrupt"` ABIs impose similar constraints on the signature of functions with that ABI: `x86-interrupt` should not have a return type (linting on the exact argument types is left as future work), and the other interrupt ABIs cannot have any parameters or a return type.

r? `@workingjubilee`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 25, 2025
…signature, r=workingjubilee

Error on invalid signatures for interrupt ABIs

We recently added `extern "custom"`, which must have type `fn()`. The various `extern "interrupt"` ABIs impose similar constraints on the signature of functions with that ABI: `x86-interrupt` should not have a return type (linting on the exact argument types is left as future work), and the other interrupt ABIs cannot have any parameters or a return type.

r? ``@workingjubilee``
bors added a commit that referenced this pull request Jun 25, 2025
Rollup of 15 pull requests

Successful merges:

 - #135731 (Implement parsing of pinned borrows)
 - #138780 (Add `#[loop_match]` for improved DFA codegen)
 - #142453 (Windows: make `read_dir` stop iterating after the first error is encountered)
 - #142633 (Error on invalid signatures for interrupt ABIs)
 - #142768 (Avoid a bitcast FFI call in transmuting)
 - #142825 (Port `#[track_caller]` to the new attribute system)
 - #142844 (Enable short-ice for Windows)
 - #142934 (Tweak `-Zmacro-stats` measurement.)
 - #142955 (Couple of test suite fixes for cg_clif)
 - #142977 (rustdoc: Don't mark `#[target_feature]` functions as ⚠)
 - #142980 (Reduce mismatched-lifetime-syntaxes suggestions to MaybeIncorrect)
 - #142982 (Corrected spelling mistake in c_str.rs)
 - #142983 (Taint body on invalid call ABI)
 - #142988 (Update wasm-component-ld to 0.5.14)
 - #142993 (Update cargo)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 891dc0f into rust-lang:master Jun 25, 2025
10 checks passed
@rustbot rustbot added this to the 1.90.0 milestone Jun 25, 2025
rust-timer added a commit that referenced this pull request Jun 25, 2025
Rollup merge of #142633 - folkertdev:interrupt-abi-restrict-signature, r=workingjubilee

Error on invalid signatures for interrupt ABIs

We recently added `extern "custom"`, which must have type `fn()`. The various `extern "interrupt"` ABIs impose similar constraints on the signature of functions with that ABI: `x86-interrupt` should not have a return type (linting on the exact argument types is left as future work), and the other interrupt ABIs cannot have any parameters or a return type.

r? ```@workingjubilee```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All interrupt ABIs should enforce either () or ! as return types
6 participants