s390x-unknown-linux-gnu: cfg!(target_feature = "backchain") is always true

[RalfJung]

#127506 introduced what I think is a logic error: cfg!(target_feature = "backchain") is now always true on s390x targets, even if the feature gate is not set. I verified this with Miri, which shares the cfg logic with rustc so it should be a correct test.

This should print false when built without -Ctarget-feature=+backchain (as the feature seems off-by-default), but prints true instead:

fn main() {
    dbg!(cfg!(target_feature = "backchain"));
}

Cc @liushuyu

[liushuyu]

cfg!(target_feature = "backchain") is now always true on s390x targets, even if the feature gate is not set.

I believe this is intended. backchain is an always-available code-generation option in LLVM disguised as a target feature.
Also, enabling backchain for a single function is useless due to how IBM Z's backchain mechanism works.

[RalfJung]

By "always available" you mean that there is no difference between -Ctarget-feature=+backchain and -Ctarget-feature=-backchain? But then why would one set this feature?

[liushuyu]

By "always available" you mean that there is no difference between -Ctarget-feature=+backchain and -Ctarget-feature=-backchain? But then why would one set this feature?

There are differences between the code generated with or without backchain attribute. Still, unlike other processors, this is a platform-specific (potential) unwinding mechanism that does not require additional processor features when you don't have DWARF information or don't want to use it (profilers on IBM Z systems may use it).

[RalfJung]

The point of cfg!(target_feature = "backchain") is to reflect whether the feature is enabled. It doesn't matter whether it could be enabled or anything like that. For example, cfg!(target_feature = "crt-static") is false unless you are building with -Ctarget-feature=+crt-static. It's not about whether static builds are "available" (that's not even a meaningful question), it's about the flags used to build this code.

Maybe you are confusing cfg! with is_ARCH_feature_detected!. Those are not the same thing.

code-generation option in LLVM disguised as a target feature

If this is not actually a target feature, then it is not clear that we want this to be a target feature in Rust. Though I guess crt-static is (unfortunately) precedent here.

What happens when I mix code built with and without +backchain? Does anything go wrong?

[liushuyu]

The point of cfg!(target_feature = "backchain") is to reflect whether the feature is enabled. It doesn't matter whether it could be enabled or anything like that. On x86, cfg!(target_feature = "avx") is false unless you build with -Ctarget-feature=+avx, even if you are building on a system that has AVX.

code-generation option in LLVM disguised as a target feature

If this is not actually a target feature, then it is not clear that we want this to be a target feature in Rust.

Well, I added this as a target-feature because it was categorized as such in LLVM, and before this change, you can still use -Ctarget-feature=+backchain to pass the option to LLVM. Still, because LLVM < 18 does not recognize this option as a target feature (it was a function-level attribute), the hack was needed to get LLVM to generate correct backchain information. If you use LLVM 18 or above, then without that pull request, your only option to get backchain enabled on IBM Z is still using -Ctarget-feature=+backchain (because Rust compiler will pass this unrecognized feature to LLVM anyways).

What happens when I mix code built with and without +backchain? Does anything go wrong?

No, it just means you won't be able to perform unwinding actions based purely on backchain information.

[liushuyu]

If you want to know how Clang passes backchain attribute in this situation, you can take a look at this Godbolt playground: https://godbolt.org/z/3TrxfqE4a.

In this example, -mbackchain is specified (see https://gcc.gnu.org/onlinedocs/gcc/S_002f390-and-zSeries-Options.html#index-mbackchain), so the compiler needs to generate backchain-enabled code.

[RalfJung]

No, it just means you won't be able to perform unwinding actions based purely on backchain information.

Okay, thanks. I don't fully understand the consequences of that -- if this makes Rust panic catching go wrong, that still seems bad -- but 🤷 whatever.

The point of the issue is that the behavior of cfg!(target_feature = "backchain") is wrong. Are you still arguing against that or have my examples above been convincing? It is currently inconsistent with how crt-static behaves, which is a similar kind of "target feature" that's really more like a boolean codegen toggle.

[liushuyu]

No, it just means you won't be able to perform unwinding actions based purely on backchain information.

Okay, thanks. I don't fully understand the consequences of that -- if this makes Rust panic catching go wrong, that still seems bad -- but 🤷 whatever.

No, it won't affect panic catching since Rust panic catching does not use this mechanism on IBM Z systems.

The point of the issue is that the behavior of cfg!(target_feature = "backchain") is wrong. Are you still arguing against that or have my examples above been convincing? It is currently inconsistent with how crt-static behaves, which is a similar kind of "target feature" that's really more like a boolean codegen toggle.

Hmm, I don't know what we should do here, if we need to fix this issue, how should we avoid passing the backchain to llvm::LLVMRustHasFeature in the current framework?

[RalfJung]

The logic that decides what to pass to LLVM and the logic that decides what is set in cfg are completely independent. (That's fairly recent and might have happened after you implemented this feature.)

I think all that needs to be done is change this

rust/compiler/rustc_codegen_llvm/src/llvm_util.rs

Line 357 in 3a0fbb5

RUSTC_SPECIAL_FEATURES.contains(feature) || features.contains(&Symbol::intern(feature))

to simply

features.contains(&Symbol::intern(feature))

The backchain feature will have been added to features in this loop. But better add a test for this to be sure.