Replace per-target ABI denylist with an allowlist #86231

nagisa · 2021-06-11T20:19:57Z

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 #57182

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 that we see are mostly about Windows-specific code that was missing relevant cfgs 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.

rust-highfive · 2021-06-11T20:20:00Z

r? @varkor

(rust-highfive has picked a reviewer for you, use r? to override)

nagisa · 2021-06-11T20:21:16Z

r? @petrochenkov, perhaps? (@varkor appears to be inactive)

nagisa · 2021-06-11T21:25:42Z

@bors try (this will want a crater run – the PR restricts ABIs, some of which are stable and can be used in ill-formed ways)

bors · 2021-06-11T21:25:51Z

⌛ Trying commit dd33daf4085fbdbf744e546a19c267af9d7b8acd with merge e0f03aec47b100bc29555561bd62a99d1bc6aca5...

varkor · 2021-06-11T21:26:09Z

(@varkor appears to be inactive)

Certainly struggling to find time for larger PRs at the moment :)

bors · 2021-06-11T22:26:15Z

☀️ Try build successful - checks-actions
Build commit: e0f03aec47b100bc29555561bd62a99d1bc6aca5 (e0f03aec47b100bc29555561bd62a99d1bc6aca5)

nagisa · 2021-06-11T22:28:31Z

@craterbot check

src/test/ui/unwind-abis/feature-gate-thiscall-unwind.rs

compiler/rustc_target/src/spec/mod.rs

craterbot · 2021-06-11T22:28:36Z

👌 Experiment pr-86231 created and queued.
🤖 Automatically detected try build e0f03aec47b100bc29555561bd62a99d1bc6aca5
🔍 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 · 2021-06-18T09:30:41Z

🚧 Experiment pr-86231 is now running

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

craterbot · 2021-06-21T15:19:42Z

🎉 Experiment pr-86231 is completed!
📊 34 regressed and 11 fixed (167538 total)
📰 Open the full report.

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

nagisa · 2021-06-21T21:10:44Z

Regressions are:

couple download failures in a build.rs script of rusty_v8 or similar system-related problems;
a fair number of crates using "stdcall" ABI (in not cfg-d out on linux, but otherwise usually windows specific, code).

I believe the first class of errors is most likely unrelated to this PR and the second class of errors is likely an expected consequence that we cannot do much about other than going through the ecosystem crates and fixing them. Most of the affected crates are already platform specific anyway, and the fallout is fairly limited AFAICT.

bors · 2021-07-06T03:38:56Z

💔 Test failed - checks-actions

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. Fixes rust-lang#57182 Sponsored by: standard.ai

nagisa · 2021-07-06T10:13:46Z

@bors r=petrochenkov

bors · 2021-07-06T10:13:48Z

📌 Commit 8240e7a has been approved by petrochenkov

bors · 2021-07-06T14:02:25Z

⌛ Testing commit 8240e7a with merge b09dad3...

bors · 2021-07-06T16:50:26Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing b09dad3 to master...

Fix the logic for custom vendor targets to account for refactoring of the aarch64-unknown-linux-gnu target upstream: rust-lang/rust#86231 Signed-off-by: Ben Cressey <bcressey@amazon.com>

…ions, r= make unsupported_calling_conventions a hard error This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the PR was landed. This should get cratered, and I assume then it needs a t-compiler FCP. Fixes rust-lang#88397

…ntions, r=compiler-errors make unsupported_calling_conventions a hard error This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the lint was originally added. This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.) Fixes rust-lang#87678

…ions, r=compiler-errors make unsupported_calling_conventions a hard error This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang#86231 (comment)) even when the lint was originally added. This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.) Fixes rust-lang#87678

…ompiler-errors make unsupported_calling_conventions a hard error This has been a future-compat lint (not shown in dependencies) since Rust 1.55, released 3 years ago. Hopefully that was enough time so this can be made a hard error now. Given that long timeframe, I think it's justified to skip the "show in dependencies" stage. There were [not many crates hitting this](rust-lang/rust#86231 (comment)) even when the lint was originally added. This should get cratered, and I assume then it needs a t-compiler FCP. (t-compiler because this looks entirely like an implementation oversight -- for the vast majority of ABIs, we already have a hard error, but some were initially missed, and we are finally fixing that.) Fixes rust-lang/rust#87678

rust-highfive assigned varkor Jun 11, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2021

rust-highfive assigned petrochenkov and unassigned varkor Jun 11, 2021

nagisa mentioned this pull request Jun 11, 2021

The check_abi function in typeck is not called for ABIs in trait declarations or type aliases #86232

Open

This comment has been minimized.

Sign in to view

nagisa force-pushed the nagisa/abi-allowlist branch from b58bf80 to dd33daf Compare June 11, 2021 20:32

This comment has been minimized.

Sign in to view

petrochenkov reviewed Jun 11, 2021

View reviewed changes

src/test/ui/unwind-abis/feature-gate-thiscall-unwind.rs Outdated Show resolved Hide resolved

compiler/rustc_target/src/spec/mod.rs Outdated Show resolved Hide resolved

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 11, 2021

ghost mentioned this pull request Jun 12, 2021

Platform-specific function ABIs are not validated #81396

Closed

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 21, 2021

nagisa force-pushed the nagisa/abi-allowlist branch from dd33daf to 125a793 Compare June 21, 2021 21:15

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 6, 2021

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2021

nagisa force-pushed the nagisa/abi-allowlist branch from 95560b4 to 8240e7a Compare July 6, 2021 10:12

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 6, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 6, 2021

bors merged commit b09dad3 into rust-lang:master Jul 6, 2021

rustbot added this to the 1.55.0 milestone Jul 6, 2021

This was referenced Jul 6, 2021

[aarch64] add target feature outline-atomics #83655

Merged

Deprecate crate_type and crate_name nested inside #![cfg_attr] #83744

Merged

Do not allow JSON targets to set is-builtin: true #86062

Merged

Rename some Rust 2021 lints to better names #86717

Merged

petrochenkov mentioned this pull request Oct 23, 2021

Tracking issue for the thiscall calling convention #42202

Closed

RalfJung mentioned this pull request Sep 3, 2024

make unsupported_calling_conventions a hard error #129935

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace per-target ABI denylist with an allowlist #86231

Replace per-target ABI denylist with an allowlist #86231

nagisa commented Jun 11, 2021 •

edited

Loading

rust-highfive commented Jun 11, 2021

nagisa commented Jun 11, 2021

This comment has been minimized.

nagisa commented Jun 11, 2021

bors commented Jun 11, 2021

varkor commented Jun 11, 2021

bors commented Jun 11, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa commented Jun 11, 2021

craterbot commented Jun 11, 2021

craterbot commented Jun 18, 2021

craterbot commented Jun 21, 2021

nagisa commented Jun 21, 2021

This comment has been minimized.

bors commented Jul 6, 2021

nagisa commented Jul 6, 2021

bors commented Jul 6, 2021

bors commented Jul 6, 2021

bors commented Jul 6, 2021

Replace per-target ABI denylist with an allowlist #86231

Replace per-target ABI denylist with an allowlist #86231

Conversation

nagisa commented Jun 11, 2021 • edited Loading

Summary for teams

rust-highfive commented Jun 11, 2021

nagisa commented Jun 11, 2021

This comment has been minimized.

nagisa commented Jun 11, 2021

bors commented Jun 11, 2021

varkor commented Jun 11, 2021

bors commented Jun 11, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nagisa commented Jun 11, 2021

craterbot commented Jun 11, 2021

craterbot commented Jun 18, 2021

craterbot commented Jun 21, 2021

nagisa commented Jun 21, 2021

This comment has been minimized.

bors commented Jul 6, 2021

nagisa commented Jul 6, 2021

bors commented Jul 6, 2021

bors commented Jul 6, 2021

bors commented Jul 6, 2021

nagisa commented Jun 11, 2021 •

edited

Loading