libc introduce X86_FEATURES for linux glibc/musl

Closes #2929

Oct 06 '22 06:10 devnexen

r? @Amanieu

(rust-highfive has picked a reviewer for you, use r? to override)

Oct 06 '22 06:10 rust-highfive

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?

Oct 09 '22 01:10 JohnTitor

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.

Oct 09 '22 06:10 devnexen

@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.

Oct 10 '22 01:10 JohnTitor

@devnexen is there a reason we can't cover these with tests? What header are they in?

Oct 10 '22 04:10 thomcc

They are in header only found in kernel source

Oct 10 '22 05:10 devnexen

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.

Oct 10 '22 16:10 joshtriplett

:+1:, this new version looks much better to me, and seems reasonable to merge.

@JohnTitor Any objections, or would you be fine with merging this?

Oct 11 '22 14:10 joshtriplett

I'm still concerned these don't have a test. And as far as I've noticed, these're also unstable, i.e. could be changed easily. Example commits are https://github.com/torvalds/linux/commit/d45476d9832409371537013ebdd8dc1a7781f97a, https://github.com/torvalds/linux/commit/356edeca2e811642b5848605c2f5f9a7c6c32112, https://github.com/torvalds/linux/commit/f098addbdb44c8a565367f5162f3ab170ed9404a. Since we don't have any policy for the supported kernel version, we cannot change it soonish when something is changed/removed (currently we haven't changed/removed items directly but have deprecated them, moreover CI won't let us know about these changes :(). So I'm not sure if these really fit with this crate.

Oct 12 '22 09:10 JohnTitor

@JohnTitor has a point. Fine by me if we close it.

Oct 14 '22 17:10 devnexen

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.

Oct 16 '22 15:10 joshtriplett

Then these could be provided via another crate like mach on macOS.

Oct 16 '22 21:10 JohnTitor

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.

Oct 17 '22 04:10 joshtriplett

Also, arguably, the visible ones should be moved to a uapi header in the kernel.

Oct 17 '22 04:10 joshtriplett

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.

Oct 18 '22 10:10 JohnTitor

@devnexen Could you address https://github.com/rust-lang/libc/pull/2942#issuecomment-1280270087? I think we could merge once it's done.

Dec 11 '22 00:12 JohnTitor

@joshtriplett Could you review the new diff?

Dec 14 '22 00:12 JohnTitor

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 https://github.com/rust-lang/libc/pull/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.

Mar 02 '23 01:03 workingjubilee

:umbrella: The latest upstream changes (presumably #3173) made this pull request unmergeable. Please resolve the merge conflicts.

Apr 18 '23 11:04 bors