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

How should cfg(target-feature) behave around forbidden target features? #132351

Closed
RalfJung opened this issue Oct 30, 2024 · 1 comment · Fixed by #133099
Closed

How should cfg(target-feature) behave around forbidden target features? #132351

RalfJung opened this issue Oct 30, 2024 · 1 comment · Fixed by #133099
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-discussion Category: Discussion or questions that doesn't represent real issues. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Oct 30, 2024

See #129884 and #116344 for context explaining what a forbidden target feature is.

The plan for target features like fpreg on ARM is that they will be forbidden unless the target has soft-float in its base feature set. This means our hardfloat ARM targets require fpregs, but on our softfloat ARM targets users can opt-in to having the FPU avialable, it just won't be used for the ABI. See #124404 for why we can't just forbid fpregs outright.

What should the behavior of cfg(target-feature = "fpregs") be, then? For now, #129884 makes it so that forbidden features are never set in cfg. But for this one it seems like when we unstable add fpregs for softfloat targets, then we'll want to unstably expose the ability to do cfg(target-feature = "fpregs") on all ARM targets. So we'll need a concept of target features that are forbidden to set, but can be unstably (and eventually, stably) queried. At the same time, other target features like soft-float likely should never be queried.

So seems like for these more complicated features, the question whether a feature is forbidden to set is somewhat orthogonal to its stability. I have some plans for a follow-up PR to #129884 to be able to represent that, but still need to figure out the details.

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 30, 2024
@jieyouxu jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-discussion Category: Discussion or questions that doesn't represent real issues. A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 30, 2024
@workingjubilee workingjubilee added the A-ABI Area: Concerning the application binary interface (ABI) label Nov 4, 2024
@RalfJung
Copy link
Member Author

RalfJung commented Nov 5, 2024

My current plan is to have a representation like this:

pub enum Stability {
    /// This target feature is stable. It can be used in `#[cfg(target_feature)]` and
    /// `allow_toggle` controls when it can be used in `#[target_feature]` or
    /// `-Ctarget-feature`.
    Stable {
        allow_toggle: fn(&Session) -> Result<(), &'static str>,
    },
    /// This target feature is unstable. It can be used in `#[cfg(target_feature)]` on nightly,
    /// and `allow_toggle` controls when it can be used in `#[target_feature]` or
    /// `-Ctarget-feature`; the former additionally requires enabling the given nightly feature.
    Unstable {
        feature: Symbol,
        allow_toggle: fn(&Session) -> Result<(), &'static str>,
    },
    /// This feature can not be set via `-Ctarget-feature` or `#[target_feature]`, it can only be set in the basic
    /// target definition. Used in particular for features that change the floating-point ABI.
    Forbidden { reason: &'static str },
}

For instance, fpreg would be Unstable (and later Stable) with an allow_toggle function that ensures that the target base definition enables the soft-float feature.

What I am not sure about is whether a query can return function pointers. Also it may be good to evaluate those functions only once... but we actually do access this target feature stuff without going through the query in a bunch of early codegen setup code. We'll just have to benchmark this.

@bors bors closed this as completed in 327c7ee Dec 13, 2024
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Dec 15, 2024
…ingjubilee

forbid toggling x87 and fpregs on hard-float targets

Part of rust-lang/rust#116344, follow-up to rust-lang/rust#129884:

The `x87`  target feature on x86 and the `fpregs` target feature on ARM must not be disabled on a hardfloat target, as that would change the float ABI. However, *enabling* `fpregs` on ARM is [explicitly requested](rust-lang/rust#130988) as it seems to be useful. Therefore, we need to refine the distinction of "forbidden" target features and "allowed" target features: all (un)stable target features can determine on a per-target basis whether they should be allowed to be toggled or not. `fpregs` then checks whether the current target has the `soft-float` feature, and if yes, `fpregs` is permitted -- otherwise, it is not. (Same for `x87` on x86).

Also fixes rust-lang/rust#132351. Since `fpregs` and `x87` can be enabled on some builds and disabled on others, it would make sense that one can query it via `cfg`. Therefore, I made them behave in `cfg` like any other unstable target feature.

The first commit prepares the infrastructure, but does not change behavior. The second commit then wires up `fpregs` and `x87` with that new infrastructure.

r? `@workingjubilee`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-target-feature Area: Enabling/disabling target features like AVX, Neon, etc. C-discussion Category: Discussion or questions that doesn't represent real issues. E-needs-design This issue needs exploration and design to see how and if we can fix/implement it 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.

4 participants