Stabilise std::is_aarch64_feature_detected · Issue #86941 · rust-lang/rust · Git...
source link: https://github.com/rust-lang/rust/issues/86941
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
This issue constitutes the stabilisation report as per the rustc dev guide
Summary
I'm hoping to stabilise std::is_aarch64_feature_detected which allows for runtime feature detection on aarch64 platforms. It was recently updated to include all features supported by both linux and LLVM 12, though the detection for some features isn't on non-linux platforms yet.
Documentation
The documentation for it is in this file. There's doc comments for individual features in the macro but I don't see these exposed anywhere, just on a hidden enum. Perhaps this could be improved. I've also got a small patch to mention it in the Rust Reference here.
Tests
There are tests for it in stdarch as well as rust. These are currently all up to date.
Unresolved questions
The only backwards-compatible question left to resolve is whether to split pauth into paca (address authentication) and pacg (generic authentication) as linux does. The current behaviour (together) matches LLVM (and the ARMv8-A feature FEAT_PAuth).
The x86 macro was stabilised under the simd_x86 feature - does this mean I should create a new simd_aarch64 feature to stabilise this and other aarch64 stdsimd work under?
See also:
#48556 for the tracking issue for the stdsimd feature.
#90620 for stabilising the target_feature attribute on aarch64 platforms.
One possible issue I've noticed is that I can't see a way to view https://doc.rust-lang.org/std/macro.is_aarch64_feature_detected.html under a different platform like you can with other crates on docs.rs. Until we can I'm not sure it's possible to make the documentation for the macro easily visible.
The documentation on doc.rust-lang.org is for all platforms at the same time.
The issue is there's different output for multiple different platforms. I'll try removing the doc comment on the error macro in there to see if that helps. It's not useful on the docs page anyway and should just be a normal comment
Is the general ARM one different enough to not just stabilize it at the same time as well?
@rfcbot merge
@rfcbot merge
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:
Concerns:
- move to a module? resolved by #86941 (comment)
- to split
pauthor not to splitpauthresolved by #86941 (comment)
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!
See this document for info about what commands tagged team members can give me.
Have the unresolved questions here been addressed?
Also, are we stabilizing any corresponding #[target_feature(enable = "...")] values for aarch64?
Is the general ARM one different enough to not just stabilize it at the same time as well?
One issue is that we can't remove the crypto feature for ARM until this commit makes it to Rust's minimum LLVM version.
I don't see a problem with stabilising the other ARM features though (this seems to be an option based on how the macro is written). Just that no-one has asked for it yet.
I'm not sure how well it works on FreeBSD, nor if there's any more ARMv8 features that affect aarch32 execution (though neither of these arguably block stabilisation).
Have the unresolved questions here been addressed?
Amanieu seems happy with simd_aarch64 and I can't see any issues with it.
Personally I'm happy keeping PAuth as one feature as LLVM does as it would then match target_features.
Also, are we stabilizing any corresponding
#[target_feature(enable = "...")]values for aarch64?
These should be mostly up to date.
const AARCH64_ALLOWED_FEATURES: &[(&str, Option<Symbol>)] = &[
Since the minimum LLVM version has moved up quickly I reckon a few could be uncommented - such as PAuth
To clarify, we are only stabilizing feature detection, not the stdarch intrinsics or #[target_feature(enable = "..")]. Although now that you mention it, we should probably stabilize aarch64_target_feature as well for #[target_feature(enable = "..")] since it uses the same feature names.
@rfcbot concern move to a module?
Should we move the macro out of the root of std? Now it's just std::is_aarch64_feature_detected. Since nowadays we have the ability to put macros into modules, it might be good to not clutter the root of the crate with very specific macros like this.
The (stable) x86/x86_64 variant is in the crate root too. Moving only the aarch64 variant would be confusing.
After some internal discussion my opinion on splitting PAuth has changed a little - apparently there have been attempts to enable partial userspace support of the feature in the past and it's reasonably possible that it could happen in the future, in which case we would want separate paca and pacg options. These could of course be added in the future, though we'll end up with those in addition to pauth, slightly confusingly.
Would it make sense to split them preemptively for runtime feature detection? Or would the above scenario be ok?
I would rather preemptively split the features. We also need to figure out how the two features map to LLVM's pointer authentication support.
PAuth demonstrates some rather interesting properties that could apply to other features too, so it's worth examining, and perhaps using this as a model for other cases that arise. Notably:
- Individual PAuth features could be used explicitly (e.g. assembly guarded by run-time detection),
- or implicitly (e.g. as compiler-generated CFI),
- or some combination of the two.
A likely example application could involve the compiler using paca to implement CFI, whilst user code explicitly uses pacg to do something else.
So, as a general policy, at least for runtime detection, I would argue for splitting features down to the point that they're unlikely to be split further in the future, as we've done here. A useful guide for is "unlikely to be split further" is the hardware ID registers, and those are what we used for VIXL. Most Linux CPU features map 1:1 onto minimum values of fields in those registers.
The compile-time detection — not strictly this issue but related — is made complicated by interactions with LLVM. As a user, I would prefer those to be split in the same way, but I don't fully understand the interactions with LLVM, so it may not be possible. I know that Rust has a mapping function so we can deal with renamings, but dealing with combinations may be trickier.
I agree that we should be consistent in locating this; I don't think it does any harm in the root, since it has a sufficiently specific name that it won't conflict with anything.
I checked LLVM's code: the pauth feature only controls whether the assembly instructions are available (and whether LLVM's codegen is allowed to use emit). CFI is controlled by a different flag.
I agree that we should try to have fine-grained features where possible. The main issue with exposing finer-grained features than LLVM is that there may not be a precise way to map #[target_feature(enable = "paca")] to LLVM. Should we just enable the pauth feature entirely? Should we give an error and require the user to enable both paca and pacg for the LLVM pauth feature to be enabled?
The (stable) x86/x86_64 variant is in the crate root too. Moving only the aarch64 variant would be confusing.
What I'd prefer is to move both into a submodule, just like asm!(), and re-export the x86 one through the prelude/root for backwards compatibility but acknowledge that was a mistake.
All of std's macros were originally in the root, because of #[macro_export]. But nowadays with the new pub macro syntax, we have the option of putting them in modules, which we've been using for std::ptr::addr_of, std::arch::asm, etc. I don't think we should continue putting macros in the root/prelude by default, even if we have to keep a few older ones there because of historical reasons.
I'd support that if and only if we can actually deprecate the re-export. I think we could deprecate the re-export by writing a deprecated wrapper macro that calls the version in std::arch.
Otherwise, if we can't deprecate it, I think we should re-export both at the top level.
That said, I also think these names will never conflict with anything, and exporting them will not do any harm, so while I don't object to unblocking the issue by taking that approach, I also would be fine with just exporting these at the top level and have no objection to that approach either.
I think we could deprecate the re-export by writing a deprecated wrapper macro that calls the version in
std::arch.
Yeah, good idea. That should work.
That said, I also think these names will never conflict with anything, and exporting them will not do any harm
I'm not worried about naming conflicts. I'm concerned about adding more clutter to the front page of the standard library reference documentation (https://doc.rust-lang.org/stable/std/) and to auto-completion features in editors.
That said, I also think these names will never conflict with anything, and exporting them will not do any harm
I'm not worried about naming conflicts. I'm concerned about adding more clutter to the front page of the standard library reference documentation (https://doc.rust-lang.org/stable/std/) and to auto-completion features in editors.
I agree with this. It's unfortunate that one macro in this series was stabilized long before macro namespacing was possible, but it's possible to avoid exacerbating this annoyance, as well as avoid setting a precedent for future macros in this series. As mentioned, it's possible to deprecate is_x86_feature_detected by defining a wrapper macro, and deprecating this would give IDE tooling authors cover to avoid autocompleting it; consider today that autocompleting i or std::i would suggest is_x86_feature_detected before suggesting Iterator or std::iter. And considering that the arch module already exists, it's not difficult to figure out where these should be naturally defined instead.
Copy link
rfcbot commented 20 days ago
This is now entering its final comment period, as per the review above.
Copy link
rfcbot commented 10 days ago
The final comment period, with a disposition to merge, as per the review above, is now complete.
As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.
This will be merged soon.
Sorry for the silence on this issue for a while. The implementations should be mostly up to date now. I'll summarise my open PRs for this in one place:
Runtime detection macro:
- rust-lang/reference#1061 An small Rust Reference update to mention the macro.
- rust-lang/stdarch#1239 Stabilises the macro (minus pauth) in stdarch.
- #90271 Stabilises this macro in rust. Needs the stdarch patch to be merged and the submodule updated.
- rust-lang/stdarch#1259 Split pauth in stdarch, and stabilise paca/pacg
Target feature:
- #90621 Stabilise
aarch64_target_feature. Needs a small update depending on what we do with this: - #93782 Split pauth target feature.
- rust-lang/reference#1102 Document target features. Currently mentions paca/pacg and not pauth.
See also #90620 for the aarch64_target_feature stabilisation issue.
Is there anything else needed for this issue, or can it be closed?
#90271 (comment) @ehuss continuing discussion here.
Looks like the error macro (in stdarch/crates/std_detect/src/detect/error_macros.rs) is still marked as unstable under stdsimd.
The same is also true for the x86 version - so you get an unstable warning when using is_x86_feature_detected! on aarch64 platforms, for instance.
Both should clearly be fixed I think - I'll have a patch up shortly.
@ehuss also mentioned that the docs still show the error_macro docs for none-x86 platforms. Yet to find the reason but everything's fine with cargo doc in stdarch, but not with ./x.py doc.
Maybe some issue due to std_detect being a separate crate from std which causes #[cfg(doc)] to not work correctly?
When a dependency is built for rustdoc, cfg(doc) is not set. So the re-export is just re-exporting the normal macros, not the cfg(doc) ones.
That makes sense rust-lang/cargo#8811
So, with beta cutoff next week, we could:
- Modify the rustc-args via a section like
[package.metadata.docs.rs]in std_detect's Cargo.toml. Does that docs.rs section also affect doc.rust-lang? Is there another (presumably unused as I couldn't find any) section for doc.rust-lang.org? - Apply the doc comments in
aarch64.rsto the error macro. It will look the same with the exception of thesrclink, and the list of cases in the macro (which are duplicates of what's in the doc comments anyway. - Is there a way to expose the feature detection macro (under
#[doc(hidden)]) regardless of architecture so we can put some#[cfg(doc)]logic inlibrary/std/src/lib.rs? Potentially through a wrapper macro or reexporting under a different name/module.
The third option seems to be more invasive so I'm going to go with the second for now until someone says otherwise.
cutoff next week
Cutoff is Friday morning this week.
Does that docs.rs section also affect doc.rust-lang?
It does not.
Adding comments to the error macros might help a little, but I imagine will still show up as unstable so it might still be confusing.
Unfortunately I don't have much time to help here. It's a bit of a thorny issue that can be tricky to fix.
I opened rust-lang/stdarch#1283 which should fix this.
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK