-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Add the cpuid
target feature
#146560
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
base: master
Are you sure you want to change the base?
Add the cpuid
target feature
#146560
Conversation
Some changes occurred in compiler/rustc_codegen_gcc
cc @Amanieu, @folkertdev, @sayantn |
This comment has been minimized.
This comment has been minimized.
8282246
to
d1b9074
Compare
d1b9074
to
5372c28
Compare
This comment has been minimized.
This comment has been minimized.
5372c28
to
38e000a
Compare
This comment has been minimized.
This comment has been minimized.
This PR adds a new Since CPUID support is implicitly enabled on most of our x86 targets, this effectively makes the CPUID intrinsic safe to call even before the @rfcbot merge |
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
cc @RalfJung |
I am a bit surprised to see this exposed as a target feature. The tracking issue says
However, even very old CPUs will have x87, right? So it seems like this will be enabled there? Is the CPUID instruction actually unsafe in those situations? If it reliably segfaults, it would not be unsafe (as long as the inline asm block doesn't claim it will always return). |
The Intel reference says that
so yes, it will reliably abort the program. This could result in problems though if LLVM decides to add a On SGX, |
LLVM is not involved here at all, |
|
I don't think hypothetical future LLVM extensions are that relevant here. Even if that happens, we can just keep using the inline asm version.
Hm, interesting. I don't think we have used target features for "should not be used" before. Also this does not resolve the problem of "what if we ever want to enable any of the other target features (that imply To be clear, I don't have a strong opinion either way, this just feels like an "odd" target feature and I couldn't find much discussion of why it is a target feature in the first place. |
Sorry, I forgot to mention that I am not sure if
I guess it's fine, the wikipedia page says
Also, target features are supported to be additive, so not enabling any target features on sgx can't be problematic (I think, please correct me if I'm wrong).
The main rationale behind making it a target feature is |
It can be problematic if AVX in SGX becomes available and someone actually wants to use it. ;)
Technically it seems to be safe to use even without that feature gate. |
Our 32-bit targets are i586 and i686, and I don't think we should try to support older x86-32 CPUs without CPUID. That leaves SGX. And that's enough of a special case that it seems like it should be its own thing, not a general-purpose target feature that implies code on x86 can't in general call CPUID with impunity. |
Ok in that case we can make |
I'm not 100% certain that CPUID is always guaranteed to be an innocuous opcode on pre-586 CPUs, including the various 386/486 clones. For those, there is a well-defined procedure for testing whether the CPUID instruction is supported. On the other hand in #60123 we decided to effectively drop support for 386/486 by removing the explicit code to check for the presense of CPUID and instead assume it is always available. So far nobody has complained... |
- Make `cpuid` and friends safe with `#[target_feature(enable = "cpuid")]` - Update documentation
38e000a
to
22b4712
Compare
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
Tracking issue: #146558
r? @Amanieu
@rustbot label O-x86_32 O-x86_64 A-target-feature T-compiler T-libs-api