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

Codegen features are no longer recognized by rustc #96472

Open
a1phyr opened this issue Apr 27, 2022 · 11 comments
Open

Codegen features are no longer recognized by rustc #96472

a1phyr opened this issue Apr 27, 2022 · 11 comments
Labels
C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@a1phyr
Copy link
Contributor

a1phyr commented Apr 27, 2022

On stable, giving additional codegen features to rustc works as expected, for example:

$ rustc --target=wasm32-unknown-unknown -C target-feature=+bulk-memory test.rs --crate-type=cdylib

On beta and nightly, compiling with the same options works too, but warns that the feature is not recognized.

$ rustc +nightly --target=wasm32-unknown-unknown -C target-feature=+bulk-memory test.rs --crate-type=cdylib
warning: unknown feature specified for `-Ctarget-feature`: `bulk-memory`
  |
  = note: it is still passed through to the codegen backend
  = note: consider filing a feature request

warning: 1 warning emitted

Target features work well.
This may be caused by #95202.

Version it worked on

It most recently worked on: 1.60

Version with regression

Beta (1.61)

rustc --version --verbose:

rustc 1.61.0-beta.4 (69a6d12e9 2022-04-25)
binary: rustc
commit-hash: 69a6d12e9f0372e3c6d82bc7c7e410dab02d0500
commit-date: 2022-04-25
host: x86_64-unknown-linux-gnu
release: 1.61.0-beta.4
LLVM version: 14.0.0

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

@a1phyr a1phyr added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Apr 27, 2022
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-untriaged Untriaged performance or correctness regression. labels Apr 27, 2022
@a1phyr a1phyr changed the title Codegen feature are no longer recognized by rustc Codegen features are no longer recognized by rustc Apr 27, 2022
@ehuss
Copy link
Contributor

ehuss commented Apr 27, 2022

This is caused by #87402 which contains some motivation for displaying a warning.

@bjorn3
Copy link
Member

bjorn3 commented Apr 27, 2022

When doing a local rebuild of rustc I'm getting the following which I think is related:

170 | |     );
171 | | }
    | |_- in this expansion of `thread_local!` (#1)
...
178 |   macro_rules! __thread_local_inner {
    |  _-
    | |_|
    | |
179 | |     // used to generate the `LocalKey` value for const-initialized thread locals
180 | |     (@key $t:ty, const $init:expr) => {{
181 | |         #[cfg_attr(not(windows), inline)] // see comments below
...   |
259 | |                 not(all(target_family = "wasm", not(target_feature = "atomics"))),
    | |                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
...   |
363 | |             $crate::__thread_local_inner!(@key $t, $($init)*);
    | |             ------------------------------------------------- in this macro invocation (#3)
364 | |     }
365 | | }
    | | -
    | |_|
    | |_in this expansion of `$crate::__thread_local_inner!` (#2)
    |   in this expansion of `$crate::__thread_local_inner!` (#3)
    |
   ::: library/std/src/sys_common/thread_info.rs:13:1
    |
13  |   thread_local! { static THREAD_INFO: RefCell<Option<ThreadInfo>> = const { RefCell::new(None) } }
    |   ------------------------------------------------------------------------------------------------ in this macro invocation (#1)

warning: unexpected `cfg` condition name
 --> library/std/src/sys_common/thread_parker/mod.rs:5:37
  |
5 |         all(target_arch = "wasm32", target_feature = "atomics"),
  |                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^

warning: unexpected `cfg` condition name
   --> library/std/src/thread/local.rs:319:55
    |
148 | / macro_rules! thread_local {
149 | |     // empty (base case for the recursion)
150 | |     () => {};
151 | |
...   |
169 | |         $crate::__thread_local_inner!($(#[$attr])* $vis $name, $t, $init);
    | |         ----------------------------------------------------------------- in this macro invocation (#2)
170 | |     );
171 | | }
    | |_- in this expansion of `thread_local!` (#1)
...
178 |   macro_rules! __thread_local_inner {
    |  _-
    | |_|
    | |
179 | |     // used to generate the `LocalKey` value for const-initialized thread locals
180 | |     (@key $t:ty, const $init:expr) => {{
181 | |         #[cfg_attr(not(windows), inline)] // see comments below
...   |
319 | |                 #[cfg(all(target_family = "wasm", not(target_feature = "atomics")))]
    | |                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
...   |
363 | |             $crate::__thread_local_inner!(@key $t, $($init)*);
    | |             ------------------------------------------------- in this macro invocation (#3)
364 | |     }
365 | | }
    | | -
    | |_|
    | |_in this expansion of `$crate::__thread_local_inner!` (#2)
    |   in this expansion of `$crate::__thread_local_inner!` (#3)
    |
   ::: library/std/src/panicking.rs:328:5
    |
328 |       thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = Cell::new(0) }
    |       ---------------------------------------------------------------------- in this macro invocation (#1)

warning: unexpected `cfg` condition name
   --> library/std/src/thread/local.rs:326:57
    |
148 | / macro_rules! thread_local {
149 | |     // empty (base case for the recursion)
150 | |     () => {};
151 | |
...   |
169 | |         $crate::__thread_local_inner!($(#[$attr])* $vis $name, $t, $init);
    | |         ----------------------------------------------------------------- in this macro invocation (#2)
170 | |     );
171 | | }
    | |_- in this expansion of `thread_local!` (#1)
...
178 |   macro_rules! __thread_local_inner {
    |  _-
    | |_|
    | |
179 | |     // used to generate the `LocalKey` value for const-initialized thread locals
180 | |     (@key $t:ty, const $init:expr) => {{
181 | |         #[cfg_attr(not(windows), inline)] // see comments below
...   |
326 | |                     not(all(target_family = "wasm", not(target_feature = "atomics"))),
    | |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^
...   |
363 | |             $crate::__thread_local_inner!(@key $t, $($init)*);
    | |             ------------------------------------------------- in this macro invocation (#3)
364 | |     }
365 | | }
    | | -
    | |_|
    | |_in this expansion of `$crate::__thread_local_inner!` (#2)
    |   in this expansion of `$crate::__thread_local_inner!` (#3)
    |
   ::: library/std/src/panicking.rs:328:5
    |
328 |       thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = Cell::new(0) }
    |       ---------------------------------------------------------------------- in this macro invocation (#1)

warning: unexpected `cfg` condition name
   --> library/std/src/thread/local.rs:333:57
    |
148 | / macro_rules! thread_local {
149 | |     // empty (base case for the recursion)
150 | |     () => {};
151 | |
...   |
169 | |         $crate::__thread_local_inner!($(#[$attr])* $vis $name, $t, $init);
    | |         ----------------------------------------------------------------- in this macro invocation (#2)
170 | |     );
171 | | }
    | |_- in this expansion of `thread_local!` (#1)
...
178 |   macro_rules! __thread_local_inner {
    |  _-
    | |_|
    | |
179 | |     // used to generate the `LocalKey` value for const-initialized thread locals
180 | |     (@key $t:ty, const $init:expr) => {{
181 | |         #[cfg_attr(not(windows), inline)] // see comments below
...   |
333 | |                     not(all(target_family = "wasm", not(target_feature = "atomics"))),
    | |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^
...   |
363 | |             $crate::__thread_local_inner!(@key $t, $($init)*);
    | |             ------------------------------------------------- in this macro invocation (#3)
364 | |     }
365 | | }
    | | -
    | |_|
    | |_in this expansion of `$crate::__thread_local_inner!` (#2)
    |   in this expansion of `$crate::__thread_local_inner!` (#3)
    |
   ::: library/std/src/panicking.rs:328:5
    |
328 |       thread_local! { static LOCAL_PANIC_COUNT: Cell<usize> = Cell::new(0) }
    |       ---------------------------------------------------------------------- in this macro invocation (#1)

@Urgau
Copy link
Member

Urgau commented Apr 27, 2022

The warnings reported by @bjorn3 are the symptom of an unrelated bug in the --check-cfg implementation, specifically target features are handle in different way inside the compiler that had made target_feature (the cfg) being recognize as a known cfg condition name in all targets that have at least one target feature but it seems that for wasm32-unknown-unknown the compiler doesn't get any `arget features from the codegen leading to the warnings being emitted in a1phyr case and bjorn3 case.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Apr 28, 2022
…etrochenkov

Add missing `target_feature` to the list of well known cfg names

This PR adds the missing `target_feature` cfg name to the list of well known cfg names.

It was notice missing in rust-lang#96472 thanks to `@bjorn3,` the reason being that `--check-cfg=names()` automatically inherit the names passed by `--cfg` (or internal to `rustc`) and is seems that the vast majority of targets have at least one target feature leading to `target_feature` being a well known name in most target but it should always be a well known name so this PR add it unconditionally to list.

r? `@petrochenkov`
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 28, 2022
@apiraino
Copy link
Contributor

apiraino commented May 12, 2022

Removing the regression flag as discussed in the Zulip thread of the Prioritization Working Group.

Although this looks to the end user as a regression, as explained in the comments here this is more a case of unhelpful/incomplete warning message. A patch could probably improve on that.

@rustbot label -I-prioritize -regression-from-stable-to-beta

@rustbot rustbot removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 12, 2022
@inquisitivecrystal inquisitivecrystal added the D-confusing Diagnostics: Confusing error or lint that should be reworked. label May 13, 2022
@ojeda
Copy link
Contributor

ojeda commented May 19, 2022

It might be nice to (in addition) suggest to move the unknown feature to the custom target specification file if such a file was used to begin with instead of a built-in target.

ojeda added a commit to ojeda/linux that referenced this issue May 19, 2022
…t spec file

Only a subset of the LLVM codegen features are recognized by `rustc`,
and since rust-lang/rust#87402 (Rust 1.61.0)
the compiler gives a warning about it:

    warning: unknown feature specified for `-Ctarget-feature`: `mmx`
      |
      = note: it is still passed through to the codegen backend
      = note: consider filing a feature request

...since those features may be renamed or removed at any point:

    $ rustc --print target-features

    [...]

    Code-generation features cannot be used in cfg or #[target_feature],
    and may be renamed or removed in a future version of LLVM or rustc.

Thus move them back to the target spec generated file.

See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <[email protected]>
ojeda added a commit to ojeda/linux that referenced this issue May 19, 2022
…t spec file

Only a subset of the LLVM codegen features are recognized by `rustc`,
and since rust-lang/rust#87402 (Rust 1.61.0)
the compiler gives a warning about it:

    warning: unknown feature specified for `-Ctarget-feature`: `mmx`
      |
      = note: it is still passed through to the codegen backend
      = note: consider filing a feature request

...since those features may be renamed or removed at any point:

    $ rustc --print target-features

    [...]

    Code-generation features cannot be used in cfg or #[target_feature],
    and may be renamed or removed in a future version of LLVM or rustc.

Thus move them back to the target spec generated file.

See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <[email protected]>
@daxpedda
Copy link
Contributor

daxpedda commented Jul 23, 2022

I stumbled on this bug and did't understand why this is a suboptimal diagnostic and not a regression.
#87402, which is what introduced the warning to my understanding, only checks the actual existance of target features and if they were added with the proper + or - sign.

The bug report uses +bulk-memory, which has the + and is an existing target feature for the target used: wasm32-unknown-unknown.
Indeed the issue was a missing entry in the list of existing target features, which was fixed by @alexcrichton here: #97808.

So unless I'm simply clueless on whats actually going on here, I believe this issue can be closed.

@bjorn3
Copy link
Member

bjorn3 commented Jul 23, 2022

I stumbled on this bug and did't understand why this is a suboptimal diagnostic and not a regression.

Because it is still passed to LLVM even if rustc doesn't understand it. Rustc itself didn't understand it in the past either, but it did pass them through to LLVM. The only difference is that rustc now gives a warning if it doesn't understand a feature.

@daxpedda
Copy link
Contributor

Ah! Thanks @bjorn3!
You seem to appear out of thin air every time I don't understand something ❤️.

Just to clarify then, +bulk-memory is something rustc should understand, which was already fixed in #97808.
The remaining issue is to improve the diagnostic message to make it less confusing for target features that should actually not be recognized.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Jul 25, 2022
Add `sign-ext` target feature to the WASM target

Some target features are still missing from that list.
See rust-lang#97808 for basically the same PR by `@alexcrichton.`

Related issue: rust-lang#96472.
PR introducing this issue: rust-lang#87402.
@JennDahm
Copy link

Does this warning actually indicate a real problem, or is it just "Hey, you should ask the rust-lang devs to add this to the list of known/sanctioned LLVM codegen options"? If it's not actually indicating a real problem, is there a way to suppress the warning? It's unclear from the warning message what I need to pass to suppress it. My team uses an LLVM option not recognized by rustc, and the noise from this warning actively pushes us away from upgrading past 1.60.

@workingjubilee
Copy link
Member

workingjubilee commented Jul 31, 2022

We could offer a way to suppress the warning, but then users would not request that we canonicalize in rustc the target features LLVM currently recognizes and Rust does not, as they would simply turn off the warning instead. I understand it can be quite annoying, but the reason to have these canonicalizing mappings is not just an abstract decision about supporting multiple codegen backends. It is at least partly to make it so that we do not expose features LLVM may decide to change its support levels for, as happened with e.g. -Cinline-threshold, or at least to allow us to silence the problems in our own logic rather than simply emitting an LLVM error.

This applies to target features because LLVM reserves the right to change its own target feature definitions and has in the recent past, which is part of why we now map certain features to multiple features, now:

if get_version() >= (14, 0, 0) {
smallvec!["sse4.2", "crc32"]
} else {
smallvec!["sse4.2"]
}
}

Without a canonicalizing mapping in Rust's lowering, any target feature you use may break on the next LLVM upgrade if you are using our bundled LLVM. LLVM cuts releases on a 6 month cadence. We try to offer more stable interfaces in rustc than "my code works today, but in six months it may break". Target featureful code is effectively unsafe by definition for most targets, and unsafe weakens certain clauses of our stability promises, but I assume you're actually using it in a way that should in fact be quite defined behavior and that we should consider quite sound.

With that prelude, to answer your question:

  • Q. Does this warning actually indicate a real problem?
  • A. That depends on how you feel about the stability of your code compiling.

ojeda added a commit to ojeda/linux that referenced this issue Aug 1, 2022
…t spec file

Similar to c5eae3a ("x86: rust: move unknown-to-`rustc` codegen
features back to the target spec file"), but for `retpoline-external-thunk`:

    Only a subset of the LLVM codegen features are recognized by `rustc`,
    and since rust-lang/rust#87402 (Rust 1.61.0)
    the compiler gives a warning about it:

        warning: unknown feature specified for `-Ctarget-feature`: `mmx`
          |
          = note: it is still passed through to the codegen backend
          = note: consider filing a feature request

    ...since those features may be renamed or removed at any point:

        $ rustc --print target-features

        [...]

        Code-generation features cannot be used in cfg or #[target_feature],
        and may be renamed or removed in a future version of LLVM or rustc.

    Thus move them back to the target spec generated file.

    See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <[email protected]>
ojeda added a commit to ojeda/linux that referenced this issue Aug 1, 2022
…t spec file

Similar to c5eae3a ("x86: rust: move unknown-to-`rustc` codegen
features back to the target spec file"), but for `retpoline-external-thunk`:

    Only a subset of the LLVM codegen features are recognized by `rustc`,
    and since rust-lang/rust#87402 (Rust 1.61.0)
    the compiler gives a warning about it:

        warning: unknown feature specified for `-Ctarget-feature`: `mmx`
          |
          = note: it is still passed through to the codegen backend
          = note: consider filing a feature request

    ...since those features may be renamed or removed at any point:

        $ rustc --print target-features

        [...]

        Code-generation features cannot be used in cfg or #[target_feature],
        and may be renamed or removed in a future version of LLVM or rustc.

    Thus move them back to the target spec generated file.

    See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <[email protected]>
@nagisa
Copy link
Member

nagisa commented Oct 28, 2022

I suspect that what might be a good general solution to the D-confusing part of this issue is to rework this issue (or open a new tracking issue) with more information on what this is, and link the issue from the diagnostic. We already do something along the lines for the future compatibility warnings, and this is kinda similar in nature.

Manishearth added a commit to Manishearth/rust that referenced this issue Nov 2, 2022
Add `multivalue` target feature to WASM target

This PR is similar to rust-lang#99643 and rust-lang#97808. It addresses rust-lang#96472 for the `multivalue` target feature.

The problem I am trying to fix is to remove the following warning when compiling with `-C target-feature=+multivalue` for `--target=wasm32-unknown-unknown`.

```
warning: unknown feature specified for `-Ctarget-feature`: `multivalue`
  |
  = note: it is still passed through to the codegen backend
  = note: consider filing a feature request
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. 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