introduce X86_FEATURES for linux glibc/musl #2942
Conversation
|
r? @Amanieu (rust-highfive has picked a reviewer for you, use r? to override) |
devnexen
force-pushed
the
hwcap_x86_linux
branch
3 times, most recently
from
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. |
devnexen
force-pushed
the
hwcap_x86_linux
branch
from
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. |
devnexen
force-pushed
the
hwcap_x86_linux
branch
from
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?
devnexen
force-pushed
the
hwcap_x86_linux
branch
from
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. |
Closes #2929