Skip to content

When should cfg(target_feature = "backchain") (s390x target feature) be enabled? #142412

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
Tracked by #44839
RalfJung opened this issue Jun 12, 2025 · 4 comments · May be fixed by #142500
Open
Tracked by #44839

When should cfg(target_feature = "backchain") (s390x target feature) be enabled? #142412

RalfJung opened this issue Jun 12, 2025 · 4 comments · May be fixed by #142500
Labels
C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. O-SystemZ Target: SystemZ processors (s390x)

Comments

@RalfJung
Copy link
Member

RalfJung commented Jun 12, 2025

#127506 added support for the "backchain" target feature, but doing so required adding some hacks to the compiler. Usually, we compute cfg(target_feature) by asking the codegen backend, but apparently that is not possible here. Those hacks already were partially fixed in #129940, but looking at them again I think the logic is still wrong: the logic here

// skip checking special features, as LLVM may not understand them
if RUSTC_SPECIAL_FEATURES.contains(feature) {
return true;
}

will conclude that the target feature is always enabled in the base target! So if someone adds a s390x target where backchain is disabled by default, we'll falsely claim that it is enabled. And right now, without a -Ctarget-feature argument, we'll say the feature is enabled. I have no clue if that makes sense, but it's definitely not future-proof. (Or even worse, if a different architecture also starts using this feature name, we'll conclude the feature is enabled.)

I think RUSTC_SPECIAL_FEATURES needs to be removed and redeveloped from scratch, the current design makes little sense. (It is also extremely easy to be confused with the very different RUSTC_SPECIFIC_FEATURES hack.)

Is this hack even still needed? There were some comments indicating that this hack exists to deal with LLVM 18 and older, which we do not support any more.

Cc @liushuyu @uweigand @cuviper

@RalfJung RalfJung added the C-bug Category: This is a bug. label Jun 12, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 12, 2025
@RalfJung RalfJung added the O-SystemZ Target: SystemZ processors (s390x) label Jun 12, 2025
@uweigand
Copy link
Contributor

And right now, without a -Ctarget-feature argument, we'll say the feature is enabled.

Well, this is definitely wrong - the feature is disabled by default in LLVM. You only get the backchain when explicitly asking for it.

Is this hack even still needed? There were some comments indicating that this hack exists to deal with LLVM 18 and older, which we do not support any more.

Yes, as of LLVM 18 backchain is handled as a normal target feature in the LLVM back-end, so I don't think this hack is needed any more.

@RalfJung
Copy link
Member Author

Yes, as of LLVM 18 backchain is handled as a normal target feature in the LLVM back-end, so I don't think this hack is needed any more.

Great. I have a local patch that should clean this up, but it is waiting for #140920 to land.

@RalfJung
Copy link
Member Author

Is it still also necessary to set the backchain attribute on each function, or does enabling the LLVM target feature suffice?

@uweigand
Copy link
Contributor

Is it still also necessary to set the backchain attribute on each function, or does enabling the LLVM target feature suffice?

The only thing the backchain attribute still does is to build the function using a subtarget with the target feature enabled. So if you're already setting the target feature, the attribute doesn't do anything.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: This is a bug. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. O-SystemZ Target: SystemZ processors (s390x)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants