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

AAPCS ABI is accepted for x86 target #57182

Closed
nagisa opened this issue Dec 28, 2018 · 2 comments · Fixed by #86231
Closed

AAPCS ABI is accepted for x86 target #57182

nagisa opened this issue Dec 28, 2018 · 2 comments · Fixed by #86231
Labels
A-ffi Area: Foreign Function Interface (FFI) A-target-specs Area: compile-target specifications C-bug Category: This is a bug. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nagisa
Copy link
Member

nagisa commented Dec 28, 2018

Currently the following code compiles fine when targetting x86_64.

pub extern "aapcs" fn  foo() {}

This code is non-sensical because AAPCS calling convention is only defined for ARM. An example of non-sensical ABI being disallowed is

pub extern "fastcall" fn  foo() {}

when compiling with --target=armv7-unknown-linux-gnueabihf which fails with:

error[E0570]: The ABI `"fastcall"` is not supported for the current target
 --> <source>:6:1
  |
6 | / extern "fastcall" fn foo() { 
7 | | }
  | |_^

Targets should be reviewed for such nonsensical ABIs and their blacklists updated. I feel that to avoid this issue in the future we should rather prefer a whitelist of ABIs, rather than a blacklist, and by default put no ABIs, so that the compilation fails until the ABI list is properly populated for the target.

@nagisa nagisa added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-target-specs Area: compile-target specifications labels Dec 28, 2018
@nagisa
Copy link
Member Author

nagisa commented Dec 28, 2018

Compiling extern "aapcs" on aarch64 will explode in LLVM with LLVM ERROR: Unsupported calling convention..

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. A-ffi Area: Foreign Function Interface (FFI) I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. labels Oct 16, 2019
@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 16, 2019

I ran into these same issues in #65443 .

One thing that I mentioned there, that is not mentioned here, is that because these errors can happen on stable safe Rust code, this is actually a soundness bug (safe Rust code has UB). Note also that for some ABIs, like stdcall, we don't error on tier-1 targets.

I feel that to avoid this issue in the future we should rather prefer a whitelist of ABIs, rather than a blacklist, and by default put no ABIs, so that the compilation fails until the ABI list is properly populated for the target.

I agree. There are some ABIs that the reference says are available on all targets (e.g. "C", "system", "Rust", etc.) but except for those, each target should have a white-list that explicitly allows using an ABI on the target.

bors added a commit to rust-lang-ci/rust that referenced this issue Jul 6, 2021
…henkov

Replace per-target ABI denylist with an allowlist

It makes very little sense to maintain denylists of ABIs when, as far as
non-generic ABIs are concerned, targets usually only support a small
subset of the available ABIs.

This has historically been a cause of bugs such as us allowing use of
the platform-specific ABIs on x86 targets – these in turn would cause
LLVM errors or assertions to fire.

In this PR we got rid of the per-target ABI denylists, and instead compute
which ABIs are supported with a simple match based on, mostly, the
`Target::arch` field. Among other things, this makes it impossible to
forget to consider this problem (in either direction) and forces one to
consider what the ABI support looks like when adding an ABI (rarely)
rather than target (often), which should hopefully also reduce the
cognitive load on both contributors as well as reviewers.

Fixes rust-lang#57182

Sponsored by: standard.ai

---

## Summary for teams

One significant user-facing change after this PR is that there's now a future compat warning when building…

* `stdcall`, `fastcall`, `thiscall` using code with targets other than 32-bit x86 (i386...i686) or *-windows-*;
* `vectorcall` using code when building for targets other than x86 (either 32 or 64 bit) or *-windows-*.

Previously these ABIs have been accepted much more broadly, even for architectures and targets where this made no sense (e.g. on wasm32) and would fall back to the C ABI. In practice this doesn't seem to be used too widely and the [breakages in crater](rust-lang#86231 (comment)) that we see are mostly about Windows-specific code that was missing relevant `cfg`s and just happened to successfully `check` on Linux for one reason or another.

The intention is that this warning becomes a hard error after some time.
@bors bors closed this as completed in 8240e7a Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ffi Area: Foreign Function Interface (FFI) A-target-specs Area: compile-target specifications C-bug Category: This is a bug. I-crash Issue: The compiler crashes (SIGSEGV, SIGABRT, etc). Use I-ICE instead when the compiler panics. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants