do not map llvm_features_by_flags back into Rust-level cfg(target_feature) - #160465
Closed
RalfJung wants to merge 1 commit into
Closed
do not map llvm_features_by_flags back into Rust-level cfg(target_feature)#160465RalfJung wants to merge 1 commit into
RalfJung wants to merge 1 commit into
Conversation
rustbot
added
A-LLVM
Collaborator
|
r? @mati865 rustbot has assigned @mati865. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Member
|
I think it's reasonable to consider the |
Member
Author
|
Looking at that CI failure... What I did not realize is that we put "forbidden" target features into For this PR that means we have to be on the look for things checking for the retpoline stuff or reserved-x18 in |
Member
Author
|
Too many parts of rustc rely on having access to "all" target features, even the ones we are not planning to expose as such to the user. So I gave up on the original PR here. But I noticed some opportunities for simplifying the code: #160530. |
rustbot
removed
the
S-waiting-on-review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For some reason, we currently apply
llvm_features_by_flagsalso to the "informational LLVM target machine" that we create to populatecfg(target_feature). This strikes me as odd: these are things that we do not treat as target features on the Rust level, why should they affectcfg(target_feature)?From what I was able to find, the only LLVM target feature set by
llvm_features_by_flagsthat has a corresponding Rust target feature is wasm'sexception-handling. That one is unstable (tracking: #150260), so this PR should change nothing for stable users. On unstable, this PR means that if you set-Cpanic=unwindyou no longer getcfg(target_feature = "exception-handling"). Is that okay? Is that a problem? I don't know. This behavior has been around since #121438 but it does not look like it was discussed much in that PR; it is not clear whether this was even intentional (this code is a total mess, after all).What is even supposed to happen in cases like
-Cpanic=unwind -Ctarget-feature=-exception-handling? It is very strange to have two flags both affect the same LLVM target feature. The easiest way to resolve this would be to mark theexception-handlingtarget feature as "forbidden" so it can no longer be toggled via-Ctarget-feature. But that may be a bad idea?Cc @bjorn3 @alexcrichton @cuviper @coolreader18