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

Warn if feature settings may break compilation #89586

Open
3 tasks
workingjubilee opened this issue Oct 6, 2021 · 4 comments
Open
3 tasks

Warn if feature settings may break compilation #89586

workingjubilee opened this issue Oct 6, 2021 · 4 comments
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. O-x86_32 Target: x86 processors, 32 bit (like i686-*) O-x86_64 Target: x86-64 processors (like x86_64-*) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@workingjubilee
Copy link
Member

workingjubilee commented Oct 6, 2021

Currently, if a user instructs the compiler to adjust the features for compilation, or uses #[target_feature] to set unusual feature settings for a function, or introduces a custom target, this can trigger miscompilations if the settings are improperly "aligned" with each other (to do correct parameter passing and so on). This mostly impacts x86, due to its particular architectural extensions, but it could affect other platforms.

Some examples we definitely want to warn on:

Some of these issues are currently caught by LLVM, but Rust programmers often find underlying LLVM errors surfacing to be mysterious and cryptic, and in this case we can definitely detect them and warn about them ourselves.

In addition, due to the desire for binary floating point conformance per #10087, we probably want to emit a warning for any build configuration such that, despite having an FPU we consider to be conformant and desirable, disables the ability to use such a floating point unit, such that it could introduce non-conformant floating point code. However, that can be extended into a future issue when all the known-100%-bad bases are covered.

@workingjubilee workingjubilee added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-diagnostics Area: Messages for errors, warnings, and lints O-x86 O-x86_64 Target: x86-64 processors (like x86_64-*) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. and removed E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Oct 6, 2021
@workingjubilee
Copy link
Member Author

workingjubilee commented Oct 6, 2021

This requires digging deep into the target_feature code and likely refactoring it. Much of it lives in https://github.com/rust-lang/rust/tree/master/compiler/rustc_codegen_llvm, most particularly this file, which handles things like the interface to LLVM's target features:
https://github.com/rust-lang/rust/blob/master/compiler/rustc_codegen_llvm/src/llvm_util.rs
Especially this:

/// The list of LLVM features computed from CLI flags (`-Ctarget-cpu`, `-Ctarget-feature`,
/// `--target` and similar).
// FIXME(nagisa): Cache the output of this somehow? Maybe make this a query? We're calling this
// for every function that has `#[target_feature]` on it. The global features won't change between
// the functions; only crates, maybe…
pub fn llvm_global_features(sess: &Session) -> Vec<String> {
// FIXME(nagisa): this should definitely be available more centrally and to other codegen backends.
/// These features control behaviour of rustc rather than llvm.
const RUSTC_SPECIFIC_FEATURES: &[&str] = &["crt-static"];

And this related function may also need to be reviewed, as it handles function-level setting of codegen features, like with #[target_feature]:
/// Composite function which sets LLVM attributes for function depending on its AST (`#[attribute]`)
/// attributes.
pub fn from_fn_attrs(cx: &CodegenCx<'ll, 'tcx>, llfn: &'ll Value, instance: ty::Instance<'tcx>) {

And at least some of this logic should probably be, as part of this, hoisted into https://github.com/rust-lang/rust/tree/master/compiler/rustc_codegen_ssa. Accordingly, while it is a scoped piece of work, it is not a trivial task either.

@workingjubilee workingjubilee added E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. C-enhancement Category: An issue proposing an enhancement or a PR with one. labels Dec 6, 2021
@Wardenfar
Copy link
Contributor

Hello @workingjubilee,

I'm trying to understand the issue and i have some questions.

  • For the first sub-task : i need to detect imcompatible global features or at function level ?
    and i have not found a way to detect x87 FPU.
  • To report an error : should i use sess.err(...) ?

Thanks

@bstrie
Copy link
Contributor

bstrie commented Mar 14, 2022

@Wardenfar You may want to try asking on the #t-compiler Zulip channel: https://rust-lang.zulipchat.com/#streams/131828/t-compiler

@workingjubilee
Copy link
Member Author

My apologies for taking so long to answer. Yes, I am more reachable on Zulip.

    • Q: Should we detect incompatible features at the global feature level?
    • A: Yes, definitely.
    • Q: Should we detect incompatible features at the function feature level?
    • A: Ideally also yes.
    • Q. How is the x87 FPU denoted?
    • A. The x87 FPU feature uses the string "+x87" to enable and "-x87" to disable
    • Q. How to report an error?
    • A. Yes, sess.err or any of the other typical error emitters should be fine, including builders like struct_span_err.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 22, 2022
Fold aarch64 feature +fp into +neon

Arm's FEAT_FP and Feat_AdvSIMD describe the same thing on AArch64:
The Neon unit, which handles both floating point and SIMD instructions.
Moreover, a configuration for AArch64 must include both or neither.
Arm says "entirely proprietary" toolchains may omit floating point:
https://developer.arm.com/documentation/102374/0101/Data-processing---floating-point
In the Programmer's Guide for Armv8-A, Arm says AArch64 can have
both FP and Neon or neither in custom implementations:
https://developer.arm.com/documentation/den0024/a/AArch64-Floating-point-and-NEON

In "Bare metal boot code for Armv8-A", enabling Neon and FP
is just disabling the same trap flag:
https://developer.arm.com/documentation/dai0527/a

In an unlikely future where "Neon and FP" become unrelated,
we can add "[+-]fp" as its own feature flag.
Until then, we can simplify programming with Rust on AArch64 by
folding both into "[+-]neon", which is valid as it supersets both.

"[+-]neon" is retained for niche uses such as firmware, kernels,
"I just hate floats", and so on.

I am... pretty sure no one is relying on this.

An argument could be made that, as we are not an "entirely proprietary" toolchain, we should not support AArch64 without floats at all. I think that's a bit excessive. However, I want to recognize the intent: programming for AArch64 should be simplified where possible. For x86-64, programmers regularly set up illegal feature configurations because it's hard to understand them, see rust-lang#89586. And per the above notes, plus the discussion in rust-lang#86941, there should be no real use cases for leaving these features split: the two should in fact always go together.

Fixes rust-lang#95002.
Fixes rust-lang#95122.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 23, 2022
…sa,Amanieu

Fold aarch64 feature +fp into +neon

Arm's FEAT_FP and Feat_AdvSIMD describe the same thing on AArch64:
The Neon unit, which handles both floating point and SIMD instructions.
Moreover, a configuration for AArch64 must include both or neither.
Arm says "entirely proprietary" toolchains may omit floating point:
https://developer.arm.com/documentation/102374/0101/Data-processing---floating-point
In the Programmer's Guide for Armv8-A, Arm says AArch64 can have
both FP and Neon or neither in custom implementations:
https://developer.arm.com/documentation/den0024/a/AArch64-Floating-point-and-NEON

In "Bare metal boot code for Armv8-A", enabling Neon and FP
is just disabling the same trap flag:
https://developer.arm.com/documentation/dai0527/a

In an unlikely future where "Neon and FP" become unrelated,
we can add "[+-]fp" as its own feature flag.
Until then, we can simplify programming with Rust on AArch64 by
folding both into "[+-]neon", which is valid as it supersets both.

"[+-]neon" is retained for niche uses such as firmware, kernels,
"I just hate floats", and so on.

I am... pretty sure no one is relying on this.

An argument could be made that, as we are not an "entirely proprietary" toolchain, we should not support AArch64 without floats at all. I think that's a bit excessive. However, I want to recognize the intent: programming for AArch64 should be simplified where possible. For x86-64, programmers regularly set up illegal feature configurations because it's hard to understand them, see rust-lang#89586. And per the above notes, plus the discussion in rust-lang#86941, there should be no real use cases for leaving these features split: the two should in fact always go together.

- Fixes rust-lang#95002.
- Fixes rust-lang#95064.
- Fixes rust-lang#95122.
@workingjubilee workingjubilee added A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. and removed E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. labels Mar 3, 2023
@workingjubilee workingjubilee self-assigned this Mar 3, 2023
@Noratrieb Noratrieb added O-x86_32 Target: x86 processors, 32 bit (like i686-*) and removed O-x86-all labels Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-enhancement Category: An issue proposing an enhancement or a PR with one. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. O-x86_32 Target: x86 processors, 32 bit (like i686-*) O-x86_64 Target: x86-64 processors (like x86_64-*) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants