Skip to content
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

Tracking issue for future-incompatibility lint unsupported_calling_conventions #87678

Open
nagisa opened this issue Aug 1, 2021 · 6 comments · May be fixed by #129935
Open

Tracking issue for future-incompatibility lint unsupported_calling_conventions #87678

nagisa opened this issue Aug 1, 2021 · 6 comments · May be fixed by #129935
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Aug 1, 2021

This is the summary issue for the unsupported_calling_conventions future-incompatibility warning and other related errors. The goal of this page is describe why this change was made and how you can fix code that is affected by it. It also provides a place to ask questions or register a complaint if you feel the change should not be made. For more information on the policy around future-compatibility warnings, see our breaking change policy guidelines.

What is the warning for?

The unsupported_calling_conventions lint is output whenever there is an use of the stdcall, fastcall, thiscall, vectorcall calling conventions (or their unwind variants) on targets that cannot meaningfully be supported for the requested target.

For example stdcall does not make much sense for a x86_64 or, more apparently, powerpc code, because this calling convention was never specified for those targets.

Historically MSVC toolchains have fallen back to the regular C calling convention for targets other than x86, but Rust doesn't really see a similar need to introduce a similar hack across many more targets.

Example

extern "stdcall" fn stdcall() {}

This will produce:

warning: use of calling convention not supported on this target
  --> $DIR/unsupported.rs:39:1
   |
LL | extern "stdcall" fn stdcall() {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out;
              it will become a hard error in a future release!
   = note: for more information, see issue ...

On most of the targets the behaviour of stdcall and similar calling conventions is not defined at all, but was previously accepted due to a bug in the implementation of the compiler.

Recommendations

Use #[cfg(…)] annotations to ensure that the ABI identifiers are only used in combination with targets for which the requested ABI is well specified.

When will this warning become a hard error?

At the beginning of each 6-week release cycle, the Rust compiler team will review the set of outstanding future compatibility warnings and nominate some of them for Final Comment Period. Toward the end of the cycle, we will review any comments and make a final determination whether to convert the warning into a hard error or remove it entirely.

@nagisa nagisa added regression-from-stable-to-beta Performance or correctness regression from stable to beta. C-bug Category: This is a bug. labels Aug 1, 2021
@nagisa nagisa self-assigned this Aug 1, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 1, 2021
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 12, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.55.0 milestone Aug 27, 2021
@Mark-Simulacrum
Copy link
Member

@nagisa do you think you'll get a chance to fix this before the next release? We have approximately one week before ideally all backports are in.

@nagisa nagisa changed the title UNSUPPORTED_CALLING_CONVENTIONS future compat link needs a tracking issue Tracking issue for UNSUPPORTED_CALLING_CONVENTIONS future compatibility lint Aug 27, 2021
@nagisa nagisa added C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. C-bug Category: This is a bug. labels Aug 27, 2021
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 27, 2021
@nagisa nagisa removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 27, 2021
@nagisa
Copy link
Member Author

nagisa commented Aug 27, 2021

Yeah, thanks for the reminder. #88397 is the PR.

@Mark-Simulacrum Mark-Simulacrum removed the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label Sep 3, 2021
@Mark-Simulacrum Mark-Simulacrum removed this from the 1.55.0 milestone Sep 3, 2021
@nagisa nagisa removed their assignment Sep 13, 2021
@dpaoliello
Copy link
Contributor

Use #[cfg(…)] annotations to ensure that the ABI identifiers are only used in combination with targets for which the requested ABI is well specified.

Can you please provide samples of this? I'm not sure how'd you write this without having to duplicate the entire function or extern block...

@nagisa
Copy link
Member Author

nagisa commented Aug 1, 2022

Macros are a traditional solution to duplication woes. Something like this, perhaps:

macro_rules! extern_item {
    ($($toks: tt)+) => {
        extern "C" $($toks)+
    };
}

// Use `sysv64` on x86_64 targets instead of C
#[cfg(target_arch = "x86_64")]
macro_rules! extern_item {
    ($($toks: tt)+) => {
        extern "sysv64" $($toks)+
    };
}

extern_item! { {
    fn extern_declaration();
} }

extern_item! { fn definition() { ... } }

Otherwise, yes, the expectation is that the function definitions and extern declarations are duplicated in well-formed code.

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 1, 2022
…ottom, r=estebank

Move lint level source explanation to the bottom

So, uhhhhh

r? `@estebank`

## User-facing change

"note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic:
```diff
-   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue rust-lang#87678 <rust-lang#87678>
+   = note: `#[warn(unsupported_calling_conventions)]` on by default
```

Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO.

## Developer-facing change

`struct_span_lint` and similar methods have a different signature.

Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)`
After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>`

The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion.

Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere.

## Subtle problem

By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings...

I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate.

## P.S.

I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢
RalfJung pushed a commit to RalfJung/miri that referenced this issue Oct 2, 2022
…estebank

Move lint level source explanation to the bottom

So, uhhhhh

r? `@estebank`

## User-facing change

"note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic:
```diff
-   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #87678 <rust-lang/rust#87678>
+   = note: `#[warn(unsupported_calling_conventions)]` on by default
```

Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO.

## Developer-facing change

`struct_span_lint` and similar methods have a different signature.

Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)`
After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>`

The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion.

Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere.

## Subtle problem

By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings...

I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate.

## P.S.

I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢
RalfJung pushed a commit to RalfJung/miri that referenced this issue Oct 4, 2022
…estebank

Move lint level source explanation to the bottom

So, uhhhhh

r? `@estebank`

## User-facing change

"note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic:
```diff
-   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #87678 <rust-lang/rust#87678>
+   = note: `#[warn(unsupported_calling_conventions)]` on by default
```

Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO.

## Developer-facing change

`struct_span_lint` and similar methods have a different signature.

Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)`
After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>`

The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion.

Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere.

## Subtle problem

By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings...

I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate.

## P.S.

I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Oct 6, 2022
…estebank

Move lint level source explanation to the bottom

So, uhhhhh

r? `@estebank`

## User-facing change

"note: `#[warn(...)]` on by default" and such are moved to the bottom of the diagnostic:
```diff
-   = note: `#[warn(unsupported_calling_conventions)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #87678 <rust-lang/rust#87678>
+   = note: `#[warn(unsupported_calling_conventions)]` on by default
```

Why warning is enabled is the least important thing, so it shouldn't be the first note the user reads, IMO.

## Developer-facing change

`struct_span_lint` and similar methods have a different signature.

Before: `..., impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>)`
After: `..., impl Into<DiagnosticMessage>, impl for<'a, 'b> FnOnce(&'b mut DiagnosticBuilder<'a, ()>) -> &'b mut DiagnosticBuilder<'a, ()>`

The reason for this is that `struct_span_lint` needs to edit the diagnostic _after_ `decorate` closure is called. This also makes lint code a little bit nicer in my opinion.

Another option is to use `impl for<'a> FnOnce(LintDiagnosticBuilder<'a, ()>) -> DiagnosticBuilder<'a, ()>` altough I don't _really_ see reasons to do `let lint = lint.build(message)` everywhere.

## Subtle problem

By moving the message outside of the closure (that may not be called if the lint is disabled) `format!(...)` is executed earlier, possibly formatting `Ty` which may call a query that trims paths that crashes the compiler if there were no warnings...

I don't think it's that big of a deal, considering that we move from `format!(...)` to `fluent` (which is lazy by-default) anyway, however this required adding a workaround which is unfortunate.

## P.S.

I'm sorry, I do not how to make this PR smaller/easier to review. Changes to the lint API affect SO MUCH 😢
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 15, 2024
Check ABI target compatibility for function pointers

Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

try-job: aarch64-gnu
try-job: aarch64-apple
try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
try-job: test-various
try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 16, 2024
Check ABI target compatibility for function pointers

Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Places to check in the tests:
* [x] Check if we can use another ABI here
```
tests/debuginfo/type-names.rs
20:// gdb-check:type = type_names::GenericStruct<type_names::Struct1, extern "fastcall" fn(isize) -> usize>
375:    let generic_struct2: GenericStruct<Struct1, extern "fastcall" fn(isize) -> usize> =
```

try-job: aarch64-gnu
try-job: aarch64-apple
try-job: x86_64-msvc
try-job: x86_64-mingw
try-job: i686-msvc
try-job: i686-mingw
try-job: test-various
try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 16, 2024
Check ABI target compatibility for function pointers

Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

try-job: aarch64-gnu
try-job: aarch64-apple
try-job: x86_64-msvc
try-job: i686-msvc
try-job: test-various
try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 16, 2024
Check ABI target compatibility for function pointers

Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Open Questions
* [ ] Should this report a future incompatibility warning like rust-lang#87678 ?

try-job: aarch64-gnu
try-job: aarch64-apple
try-job: x86_64-msvc
try-job: i686-msvc
try-job: test-various
try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 24, 2024
Check ABI target compatibility for function pointers

Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Open Questions
* [ ] Should this report a future incompatibility warning like rust-lang#87678 ?
* [ ] Is this the best place to perform the check?

try-job: aarch64-gnu
try-job: aarch64-apple
try-job: x86_64-msvc
try-job: i686-msvc
try-job: test-various
try-job: armhf-gnu
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 26, 2024
Check ABI target compatibility for function pointers

Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Open Questions
* [ ] Should this report a future incompatibility warning like rust-lang#87678 ?
* [ ] Is this the best place to perform the check?
@fmease fmease changed the title Tracking issue for UNSUPPORTED_CALLING_CONVENTIONS future compatibility lint Tracking issue for future-incompatibility lint unsupported_calling_conventions Sep 14, 2024
@fmease fmease added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-ABI Area: Concerning the application binary interface (ABI) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. labels Sep 14, 2024
@tmandry
Copy link
Member

tmandry commented Oct 3, 2024

Cross posting from #128784, a couple of T-lang members thought we might want to support this for uncalled functions since adding a cfg everywhere could be quite annoying.

But we would need to ensure that the function never gets codegenned, if we want to avoid uncomfortable questions like "what the heck is the backend supposed to do with this".

@RalfJung RalfJung linked a pull request Oct 4, 2024 that will close this issue
@RalfJung
Copy link
Member

RalfJung commented Oct 4, 2024

Note that this here is about defining functions with unsupported calling conventions / ABIs. In #129935 I am suggesting to make this a hard error. It is already a hard error in most cases, but a few were forgotten, and IMO we should make this consistent.

#128784 (tracked at #130260) is about using function pointer types with unsupported ABIs.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 12, 2024
…iler-errors

Check ABI target compatibility for function pointers

Tracking issue: rust-lang#130260
Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Open Questions
* [x] Should this report a future incompatibility warning like rust-lang#87678 ?
* [ ] Is this the best place to perform the check?
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 12, 2024
…iler-errors

Check ABI target compatibility for function pointers

Tracking issue: rust-lang#130260
Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Open Questions
* [x] Should this report a future incompatibility warning like rust-lang#87678 ?
* [ ] Is this the best place to perform the check?
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 13, 2024
Rollup merge of rust-lang#128784 - tdittr:check-abi-on-fn-ptr, r=compiler-errors

Check ABI target compatibility for function pointers

Tracking issue: rust-lang#130260
Related tracking issue: rust-lang#87678

Compatibility of an ABI for a target was previously only performed on function definitions and `extern` blocks. This PR adds it also to function pointers to be consistent.

This might have broken some of the `tests/ui/` depending on the platform, so a try run seems like a good idea.

Also this might break existing code, because we now emit extra errors. Does this require a crater run?

# Example
```rust
// build with: --target=x86_64-unknown-linux-gnu

// These raise E0570
extern "thiscall" fn foo() {}
extern "thiscall" { fn bar() }

// This did not raise any error
fn baz(f: extern "thiscall" fn()) { f() }
```

# Open Questions
* [x] Should this report a future incompatibility warning like rust-lang#87678 ?
* [ ] Is this the best place to perform the check?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-future-incompatibility Category: Future-incompatibility lints C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants