reject unsound toggling of RISCV target features by RalfJung · Pull Request #134337 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation27 Commits2 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

~~Stacked on top of #133417, only the last commit is new.~~

Works towards #132618 (but more remains to be done)
Part of #116344

Cc @beetrees I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.
r? @workingjubilee

Ideally we'd also reject target specs that disable the f feature but set an ABI that requires f... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Cc @beetrees I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks.

@RalfJung So, "Zfinx" is literally

f: "do float operations like those of f0-f31"
in
x: "integer registers x0-x31"

Basically, it's a dedicated extension for "I can't believe it's not soft float!" Hardware that has it must use a soft-float ABI, since it has no float registers, and this means it cannot implement a float ABI.

...but as long as we require that "ilp32f" means you implement "f" then we're fine.

Err("feature is required by ABI")
}
"ilp32e" if enable => {
// The `d` feature apparently is incompatible with this ABI.

Choose a reason for hiding this comment

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

// The `d` feature apparently is incompatible with this ABI.
// ilp32e is incompatible with features that need aligned load/stores > 32 bits, like `d`

Choose a reason for hiding this comment

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

I am confused: Rust lets you always define types that need >32bit alignment, so that can't be a problem in itself?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

ack, I didn't mean to confuse.

yeah, it's about the actual CPU-internal alignment requirements imposed by the way the hardware moves things around, as opposed to our language requirements.

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Okay, makes sense.

Comment on lines +614 to +617

// Given that this is a negative feature, consider this before stabilizing:
// does it really make sense to enable this feature in an individual
// function with `#[target_feature]`?
nightly_feature: sym::riscv_target_feature,

Choose a reason for hiding this comment

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

not really, but it does allow #[cfg(target_feature = "e")], unfortunately, but that may not be correct if it tries to be dependent on the ABI

Choose a reason for hiding this comment

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

Sounds like we may want to mark this feature as "forbidden", i.e., "must be fixed by target spec and cannot be queried by cfg", and have a separate way to query the ABI.

But anyway it's a nightly-only feature so that can be done in a future PR.

| allow_toggle: |target, enable| { | | --------------------------------------------------------------- | | match &*target.llvm_abiname { | | _ if !enable => { | | // This is a negative feature, *disabling* it is always okay. |

Choose a reason for hiding this comment

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

// This is a negative feature, disabling it is always okay.
// ilp32e can run on rv32i, treating x16-x31 as caller-save

Choose a reason for hiding this comment

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

reason for explicit reasoning: "negative features" have a weird tendency to become "not" at some point (although there's reservations which suggest that shouldn't happen in this case), so having a clear justification for why is preferable in case we have to revisit it.

Choose a reason for hiding this comment

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

Unfortunately now the comment has so much jargon that I don't understand it...

Choose a reason for hiding this comment

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

r=me with comment adjustments or not

Can you check this correctly warns when using -Ctarget-feature=-zicsr with one of the hard-float ABIs (zicsr is required by the f extension, so disabling it automatically disabled the f extension)?

Stability::Unstable {
nightly_feature: sym::riscv_target_feature,
allow_toggle: \|target, enable
"ilp32d" \| "lp64d" if !enable => {

Choose a reason for hiding this comment

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

Btw do you know why the 32bit ABIs start with "ilp" but the 64bit ABIs start with "lp"?

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

So it should really be I32LP64 then...

Choose a reason for hiding this comment

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

Yes, but a convention at some point developed that

we omit the "c8s16" prefix... types that are the smallest possible value
we all quietly pretend the smallest possible value of int is 32 bits and not 16

names deeply fermented in tradition.

r=me with comment adjustments or not

I attempted to make the comments understandable by people who are not RISCV experts; could you take a look?

Can you check this correctly warns when using -Ctarget-feature=-zicsr with one of the hard-float ABIs (zicsr is required by the f extension, so disabling it automatically disabled the f extension)?

It will definitely not warn. We don't even have that feature in the known feature list.

Yes, those are fine~

@bors r=workingjubilee
(I hope I interpreted you correctly.)

📌 Commit 934ed85 has been approved by workingjubilee

It is now in the queue for this repository.

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

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request

…workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment))) Part of rust-lang#116344

Cc @beetrees I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks. r? @workingjubilee

Ideally we'd also reject target specs that disable the f feature but set an ABI that requires f... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.

bors added a commit to rust-lang-ci/rust that referenced this pull request

…llaumeGomez

Rollup of 9 pull requests

Successful merges:

rust-lang#132056 (Stabilize #[diagnostic::do_not_recommend])
rust-lang#134124 (CI: use free runners for x86_64-gnu-llvm jobs)
rust-lang#134197 (rustc_mir_build: Clarify that 'mirrored' does not mean 'flipped' or 'reversed')
rust-lang#134260 (Correctly handle comments in attributes in doctests source code)
rust-lang#134277 (rustdoc-search: handle impl Into<X> better)
rust-lang#134284 (Keep track of patterns that could have introduced a binding, but didn't)
rust-lang#134337 (reject unsound toggling of RISCV target features)
rust-lang#134385 (tests/ui/asm: Remove uses of rustc_attrs, lang_items, and decl_macro features by using minicore)
rust-lang#134386 (Some trait method vs impl method signature difference diagnostic cleanups)

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

…iaskrgr

Rollup of 9 pull requests

Successful merges:

rust-lang#134124 (CI: use free runners for x86_64-gnu-llvm jobs)
rust-lang#134197 (rustc_mir_build: Clarify that 'mirrored' does not mean 'flipped' or 'reversed')
rust-lang#134260 (Correctly handle comments in attributes in doctests source code)
rust-lang#134277 (rustdoc-search: handle impl Into<X> better)
rust-lang#134284 (Keep track of patterns that could have introduced a binding, but didn't)
rust-lang#134337 (reject unsound toggling of RISCV target features)
rust-lang#134371 (Check for array lengths that aren't actually usize)
rust-lang#134385 (tests/ui/asm: Remove uses of rustc_attrs, lang_items, and decl_macro features by using minicore)
rust-lang#134386 (Some trait method vs impl method signature difference diagnostic cleanups)

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Rollup merge of rust-lang#134337 - RalfJung:riscv-target-features, r=workingjubilee

reject unsound toggling of RISCV target features

~~Stacked on top of rust-lang#133417, only the last commit is new.~~

Works towards rust-lang#132618 (but more [remains to be done](rust-lang#134337 (comment))) Part of rust-lang#116344

Cc @beetrees I hope I got everything. I didn't do anything about "The f and zfinx features are incompatible" and that's not an ABI thing (right?) and I am not sure how to handle it with these ABI checks. r? @workingjubilee

Ideally we'd also reject target specs that disable the f feature but set an ABI that requires f... but I don't want to duplicate this logic. I have some ideas for how maybe the entire float ABI check logic should be different, now that we have some examples of what these ABI checks look like, but that will be a future PR.

bors added a commit to rust-lang-ci/rust that referenced this pull request

…s, r=workingjubilee

Add a notion of "some ABIs require certain target features"

I think I finally found the right shape for the data and checks that I recently added in rust-lang#133099, rust-lang#133417, rust-lang#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both -Ctarget-feature and #[target_feature] are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features.

We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this before applying -Ctarget-feature to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested.

For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;)

As a side-effect this also (unstably) allows enabling x87 when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang#133611, but no such change is happening in this PR.

r? @workingjubilee

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

…ingjubilee

Add a notion of "some ABIs require certain target features"

I think I finally found the right shape for the data and checks that I recently added in rust-lang/rust#133099, rust-lang/rust#133417, rust-lang/rust#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both -Ctarget-feature and #[target_feature] are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features.

We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this before applying -Ctarget-feature to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested.

For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;)

As a side-effect this also (unstably) allows enabling x87 when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang/rust#133611, but no such change is happening in this PR.

r? @workingjubilee

antoyo pushed a commit to rust-lang/rustc_codegen_gcc that referenced this pull request

…ingjubilee

Add a notion of "some ABIs require certain target features"

I think I finally found the right shape for the data and checks that I recently added in rust-lang/rust#133099, rust-lang/rust#133417, rust-lang/rust#134337: we have a notion of "this ABI requires the following list of target features, and it is incompatible with the following list of target features". Both -Ctarget-feature and #[target_feature] are updated to ensure we follow the rules of the ABI. This removes all the "toggleability" stuff introduced before, though we do keep the notion of a fully "forbidden" target feature -- this is needed to deal with target features that are actual ABI switches, and hence are needed to even compute the list of required target features.

We always explicitly (un)set all required and in-conflict features, just to avoid potential trouble caused by the default features of whatever the base CPU is. We do this before applying -Ctarget-feature to maintain backward compatibility; this poses a slight risk of missing some implicit feature dependencies in LLVM but has the advantage of not breaking users that deliberately toggle ABI-relevant target features. They get a warning but the feature does get toggled the way they requested.

For now, our logic supports x86, ARM, and RISC-V (just like the previous logic did). Unsurprisingly, RISC-V is the nicest. ;)

As a side-effect this also (unstably) allows enabling x87 when that is harmless. I used the opportunity to mark SSE2 as required on x86-64, to better match the actual logic in LLVM and because all x86-64 chips do have SSE2. This infrastructure also prepares us for requiring SSE on x86-32 when we want to use that for our ABI (and for float semantics sanity), see rust-lang/rust#133611, but no such change is happening in this PR.

r? @workingjubilee

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Relevant to the compiler team, which will review and decide on the PR/issue.