search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.8k stars 12.66k forks source link

Codegen features are no longer recognized by rustc #96472

Open a1phyr opened 2 years ago

a1phyr commented 2 years ago

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

ehuss commented 2 years ago

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

bjorn3 commented 2 years ago

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> = 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 = 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 = 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 = Cell::new(0) } | ---------------------------------------------------------------------- in this macro invocation (#1) ```
Urgau commented 2 years ago

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.

apiraino commented 2 years ago

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

ojeda commented 2 years ago

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.

daxpedda commented 2 years ago

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: https://github.com/rust-lang/rust/pull/97808.

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

bjorn3 commented 2 years ago

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 commented 2 years ago

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

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.

JennDahm commented 2 years ago

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 commented 2 years ago

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: https://github.com/rust-lang/rust/blob/0f4bcadb46006bc484dad85616b484f93879ca4e/compiler/rustc_codegen_llvm/src/llvm_util.rs#L177-L182

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:

nagisa commented 1 year ago

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.