introduce X86_FEATURES for linux glibc/musl#2942
introduce X86_FEATURES for linux glibc/musl#2942devnexen wants to merge 1 commit intorust-lang:mainfrom
Conversation
|
r? @Amanieu (rust-highfive has picked a reviewer for you, use r? to override) |
efdc47e to
2f146e9
Compare
|
It's unfortunate that such a big change is introduced without a test, I'm not sure if it really fits the libc crate. Could you clarify why tests are missing and those should be here? |
|
I m not sure it will be accepted since it s not really libc but more kernel space, it took me a while to add those :-) but I understand if you close. |
|
@rust-lang/libc Any thoughts on this? I'm slightly in favor of closing this but if anyone has an objection, drop a comment here. |
|
@devnexen is there a reason we can't cover these with tests? What header are they in? |
|
They are in header only found in kernel source |
|
I'd like to see these added. But is there some way we could avoid duplicating them between glibc and musl, and for that matter between x86 and x86-64? They're identical in all four cases. In general, things from the Linux kernel headers should be identical across all C libraries. |
2f146e9 to
51a46c9
Compare
|
👍, this new version looks much better to me, and seems reasonable to merge. @JohnTitor Any objections, or would you be fine with merging this? |
|
I'm still concerned these don't have a test. |
|
@JohnTitor has a point. Fine by me if we close it. |
|
I don't think we can easily test these unless the kernel headers are available on the test system. And I do think these are important for users making use of auxv. |
|
Then these could be provided via another crate like mach on macOS. |
|
My apologies, I only just now realized that this is adding all the CPU feature constants, not just those visible in auxv. This should only add those visible in binary form to userspace; the visible ones are stable. |
|
Also, arguably, the visible ones should be moved to a uapi header in the kernel. |
|
If you only add items that you want to use and these are stable and small enough to review, I'm fine to accept ignoring tests. |
|
@devnexen Could you address #2942 (comment)? I think we could merge once it's done. |
51a46c9 to
1d199d8
Compare
|
@joshtriplett Could you review the new diff? |
| pub const X86_FEATURE_AVX512BW: ::c_ulong = 288 + 30; | ||
| pub const X86_FEATURE_AVX512VL: ::c_ulong = 288 + 31; | ||
| pub const X86_FEATURE_AVX512_BF16: ::c_ulong = 384 + 5; | ||
| pub const X86_FEATURE_AVX512VBMIK: ::c_ulong = 512 + 1; |
There was a problem hiding this comment.
I can't find this feature name on torvalds/linux master, is this a typo?
1d199d8 to
ce9e659
Compare
|
I don't know if we should be exposing these if they only are mentioned in the kernel, even if they are in the uapi headers (and they currently aren't, afaict) because as mentioned in #2713, pedantically but correctly, the libc is allowed to have a varying definition here, and I can't find them exposed by musl or glibc either. This is traditionally something where people have just bypassed the libc and kernel by using the CPUID interface, because on x86, that doesn't require privileges. |
|
☔ The latest upstream changes (presumably #3173) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We talked about the scope of libc at rustweek, and one of the decisions was that our The Linux uapi maintainers might be interested in taking a patch to make these public, as @joshtriplett mentioned, assuming there isn't any specific reason they aren't exposed there. If that happens, they will be available in Thank you for the proposal! |
Closes #2929