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.

Sign up for GitHub

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

Jump to bottom

Explicitly choose x86 softfloat/hardfloat ABI #136146

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Explicitly choose x86 softfloat/hardfloat ABI #136146

wants to merge 2 commits into from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 27, 2025 •
edited
Loading

Part of #135408:
Instead of choosing this based on the target features listed in the target spec, make that choice explicit.
This means that all x86 (32bit and 64bit) softfloat target need to explicitly set rustc-abi to x86-softfloat.

Also fix some mistakes in the platform-docs where the x87 errata footnotes were missing or wrong.

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2025

r? @cjgillot

rustbot has assigned @cjgillot.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

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

rustbot commented Jan 27, 2025

These commits modify compiler targets.
(See the Target Tier Policy.)

@RalfJung
Copy link
Member Author

Cc @workingjubilee

@RalfJung RalfJung mentioned this pull request Jan 27, 2025
Draft
@RalfJung RalfJung changed the title Explicit choose x86 softfloat/hardfloat ABI Explicitly choose x86 softfloat/hardfloat ABI Jan 27, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot
Copy link
Contributor

r? compiler

@rustbot rustbot assigned lcnr and unassigned cjgillot Jan 28, 2025
@RalfJung RalfJung force-pushed the x86-abi branch 3 times, most recently from bd35e7c to 89fa12a Compare January 28, 2025 01:40
RalfJung
RalfJung commented Jan 28, 2025
View reviewed changes
compiler/rustc_target/src/target_features.rs
("soft-float", Stability::Forbidden { reason: "unsound because it changes float ABI" }, &[]),
// This cannot actually be toggled, the ABI always fixes it, so it'd make little sense to
// stabilize. It must be in this list for the ABI check to be able to use it.
("soft-float", Stability::Unstable(sym::x87_target_feature), &[]),
Copy link
Member Author

Choose a reason for hiding this comment

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

Making this "forbidden" just leads to duplicate warnings so there's little point in doing that.

This makes riscv's forced-atomics the only remaining "forbidden" feature.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense to me.

workingjubilee
workingjubilee approved these changes Jan 28, 2025
View reviewed changes
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.

My understanding is that while it changes some error outputs, this PR shouldn't have any practical effect on people, correct? Except those using our unstable target-spec.json, I suppose, and they bought the ticket for the ride they're going to get.

r=me with nits addressed

compiler/rustc_target/src/spec/json.rs Outdated
match s.parse::<super::RustcAbi>() {
Ok(rustc_abi) => base.$key_name = Some(rustc_abi),
_ => return Some(Err(format!(
"'{s}' is not a valid value for rust-abi. \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"'{s}' is not a valid value for rust-abi. \
"'{s}' is not a valid value for rustc-abi. \

compiler/rustc_target/src/spec/mod.rs Outdated
/// The Rustc-specific variant of the ABI used for this target.
#[derive(Clone, Copy, PartialEq, Hash, Debug)]
pub enum RustcAbi {
/// On x86-32/64 only: do not use any FPU or SIMD registers for the ABI/
Copy link
Member

Choose a reason for hiding this comment

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

the "ABI/"?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 28, 2025 •
edited
Loading

My understanding is that while it changes some error outputs, this PR shouldn't have any practical effect on people, correct? Except those using our unstable target-spec.json, I suppose, and they bought the ticket for the ride they're going to get.

Correct, for custom target specs this can add -soft-float if you don't also set rustc-abi: "x86-softfloat". This is probably surprising as there is no warning about this, it just silently happens.

Maybe we should land #136147 first; that would avoid changing behavior for target-spec users and instead just emit a warning.

@workingjubilee
Copy link
Member

I'm not worried about being maximally convenient for them, but either way. I'll try to review that when I finish my tea.

@lcnr
Copy link
Contributor

lcnr commented Jan 28, 2025

r? @workingjubilee

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@bors
Copy link
Contributor

bors commented Jan 29, 2025

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

@RalfJung
Copy link
Member Author

@bors r=workingjubilee

@bors
Copy link
Contributor

bors commented Jan 29, 2025

📌 Commit 11c35f3 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 Jan 29, 2025
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member Author

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 29, 2025
@RalfJung RalfJung force-pushed the x86-abi branch 4 times, most recently from a06c57d to 7ea032d Compare January 29, 2025 10:49
@RalfJung
Copy link
Member Author

@workingjubilee I have done a pass over the tests to ensure we cover both enabling and disabling of "forbidden" and of ABI-relevant target features... please take a look.

@RalfJung RalfJung added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2025
RalfJung
RalfJung commented Jan 29, 2025
View reviewed changes
tests/ui/target-feature/forbidden-hardfloat-target-feature-cfg.rs Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

x87 is just a normal target feature now, it didn't seem worth having a test for it.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
workingjubilee
workingjubilee reviewed Jan 30, 2025
View reviewed changes
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.

I have a nit apparently.

tests/ui/target-feature/forbidden-hardfloat-target-feature-flag.rs Outdated Show resolved Hide resolved
tests/ui/target-feature/forbidden-hardfloat-target-feature-flag.rs Outdated Show resolved Hide resolved
tests/ui/target-feature/forbidden-target-feature-attribute.rs
@@ -1,11 +1,12 @@
//@ compile-flags: --target=x86_64-unknown-linux-gnu --crate-type=lib
//@ needs-llvm-components: x86
//! Ensure "forbidden" target features cannot be enabled via `#[target_feature]`.
Copy link
Member

Choose a reason for hiding this comment

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

thanks for better documenting these!

@RalfJung
Copy link
Member Author

Fixed :)

@workingjubilee
Copy link
Member

workingjubilee commented Jan 30, 2025 •
edited
Loading

cool, r=me with a cleared CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees

@workingjubilee workingjubilee

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RalfJung @rustbot @rust-log-analyzer @cjgillot @workingjubilee @lcnr @bors