Skip to content
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

Update auxvec.h for Android and Linux #3125

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Feb 22, 2023

The NDK automatically generates these constants from Linux kernel headers, and they are available for all architectures. They are also repeated by musl and glibc, though this is mostly just a restatement to make use with getauxval more convenient.

New in this patch: AT_MINSIGSTKSZ

This const is needed to handle new extensions to platform arches that add state to save to signal stacks, replacing MINSIGSTKSZ with an ELF-loaded equivalent.

@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2023

r? @JohnTitor

(rustbot has picked a reviewer for you, use r? to override)

@JohnTitor
Copy link
Member

Thanks for the PR!
However, I don't think adding such a header-based module is not a good thing here. We already have the target-based (e.g. arch, OS, etc.) structure and it works fine to keep maintainability. Another structure like this adds unnecessary cfgs and introduces complexity (note that this crate has a lot of contributors including newcomers, so simplicity is important).
Also, it doesn't work for all the targets. for example, xous doesn't have C library.

@workingjubilee
Copy link
Member Author

This is scoped to Linux-likes alone, so it does not affect Xous to begin with. I am not sure why you are bringing that OS up.

I believe it is appropriate here because Android and Linux share this header, as they are in fact both Linux. In fact it is not clear to me that the 32-bit Android does not expose these auxiliary vector constants, though I would have to do slightly more research to confirm or deny either way. And the AT_SYSINFO_EHDR is probably inappropriately defined as it is architecture-specific, and thus it could conceivably be defined in another way by another architecture in terms of kernel usage.

I am aware that replicating the exact structure of the uapi headers is probably inappropriate, but duplicating data when they do share an exact copy of a header just allows people to add support to Linux or Android but not cover the other platform and allow one to go unsupported. This introduces needless divergence in functionality for a set of bindings that, I would think, is supposed to enable cross-platform functionality.

@workingjubilee
Copy link
Member Author

If my hunch is correct and these constants are also defined for the benefit of Android 32-bit userspace (again, I haven't checked yet), then there would not need to be any configuration here if it were not for emscripten, honestly. I am not sure why that "OS" is classified as Linux-like.

@workingjubilee
Copy link
Member Author

However, I don't think adding such a header-based module is not a good thing here. We already have the target-based (e.g. arch, OS, etc.) structure and it works fine to keep maintainability.

It may work fine for you, but I personally think the lack of structure is confusing because it results in things like AT_FDCWD as freestanding names which gave me several false starts as I tried to find where all the AT_ auxiliary vector constants were. I believe the appropriate structure need not be in my preferred form, even comments would do, but the current mode of totally context-free values floating around cost me at least another hour of befuddled looking back and forth and sanity-checking myself, on top of the cross-referencing I had to do just to verify this as correct.

@JohnTitor
Copy link
Member

I'm repeating but it adds an entirely different structure, and it confuses contributors, like "which ways should I follow? target-based or header-based".
This would also add a lot of cfg_if branches if we target more headers in the future, resulting in losing maintainability.

@workingjubilee
Copy link
Member Author

AT_SYSINFO_EHDR

For some reason glibc and musl both believe it is appropriate to globally define this, invariant of architecture. Okay, I guess?! They're the C libs, I'll believe them.

I did some checking and I am 100% certain that getauxval is defined as of Android NDK 18 (which will soon become lower than the minimum) and 95% sure that all the same constants are defined on 32-bit and 64-bit, since the NDK specifically recommends using getauxval it to introspect on AT_HWCAPS for Thumb instructions where appropriate. Will doublecheck after I get some more sleep.

100% sure that Emscripten defines all the same constants in their musl fork:
https://github.com/emscripten-core/musl/blob/85e0e3519655220688e757b9d5bfd314923548bd/include/elf.h#L981-L1047

I assume the Emscripten "kernel" doesn't provide any useful information when the auxiliary vector is queried but no worries, user code wasn't promised a better response than ENOENT when it runs getauxval. So assuming I don't turn up any last-minute gotchas in my doublechecking, the fix is that this should all be without cfg. I suppose I will simply put it in the same mod.rs instead, without cfg.

I presume that will be an acceptable approach to reducing needless duplication?

@workingjubilee workingjubilee changed the title Unify **/uapi/linux/auxvec.h in auxvec.rs Unify **/uapi/linux/auxvec.h in linux_like/mod.rs Feb 22, 2023
@workingjubilee workingjubilee force-pushed the joy/unified-auxvec.rs branch 2 times, most recently from ea657a9 to 426c088 Compare February 22, 2023 17:14
@workingjubilee
Copy link
Member Author

I cannot find a single thing to indicate these are unavailable or conditionally-available constants in the AArch32 case, considering you literally cannot use getauxval otherwise and the aforementioned "these constants are defined by the kernel in all cases" situation. While 32-bit Android is fading fast, these constants are nonetheless necessary on every system that defines a Linux-like API so as to query the hardware capabilities via getauxval, and they are meaningless otherwise, so the only concern is whether they ever get a future clashing definition. And in this case we can be confident they won't, because that would break the kernel-to-userspace ABI, which is intended to break approximately never (as distinct from the kernel-to-kernelspace ABI, which sometimes breaks every week).

Except for AT_SYSINFO_EHDR. That could get a clashing definition.
But that was already overeagerly defined anyways.

They are also important because other systems, like FreeBSD, define some of these constants, and for all I know they give them values in the millions instead. It's a ulong. It has space for that. These systems don't offer getauxval but they do tend to offer an equivalent, like elf_aux_info.

@workingjubilee
Copy link
Member Author

That is to say:
Please take another look.

@JohnTitor
Copy link
Member

Fair enough, thanks! @bors r+

@bors
Copy link
Contributor

bors commented Mar 3, 2023

📌 Commit 426c088 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Mar 3, 2023

⌛ Testing commit 426c088 with merge 2e05555...

bors added a commit that referenced this pull request Mar 3, 2023
Unify **/uapi/linux/auxvec.h in linux_like/mod.rs

The NDK automatically generates this from Linux kernel headers. It seems best to simply unify these, since where the kernel goes, Android will undoubtedly follow. Also, note relevant versions.

New in this patch: `AT_MINSIGSTKSZ`

This const is needed to handle new extensions to platform arches that add state to save to signal stacks, replacing `MINSIGSTKSZ` with an ELF-loaded equivalent.
@bors
Copy link
Contributor

bors commented Mar 3, 2023

💔 Test failed - checks-actions

@workingjubilee
Copy link
Member Author

workingjubilee commented Mar 6, 2023

...Oookay, so the Emscripten toolchain uses the musl libc last I checked but they just... omit... half the libc...? I mean fine, I guess! I would have expected they took the "don't return a useful answer" route (which software should be prepared for) instead of "don't support getauxval entirely", but sure sure sure. I'm going to split things back up a little so we can land the "add the constants to all the actually Linux environments" and then I am going to reimplement the simplifying change by making a pass at #1915

These values are universal across architectures,
as they are defined primarily by the Linux UAPI.
Thus adding them to only 64-bit was in error.
These values are universal across architectures,
as they are defined primarily by the Linux UAPI.
They are also echoed exactly by musl and glibc.
@workingjubilee
Copy link
Member Author

Okay, finally back to this! Sorry for taking so long!

@workingjubilee
Copy link
Member Author

I have no idea what the Cirrus CI failures are all about.

Some return values require special thought in Rust,
e.g. addresses that may violate "strict provenance",
or ones with special, hardware-specific meaning.

So note when the return value is meant to be
an address or has platform-specific interpretation.
@workingjubilee
Copy link
Member Author

I added some comments to some of the ELF AT_ values since some of them are of special interest for Rust programmers, most particularly in being harder to handle type-safely, and with a suggestion to find getauxval's documentation.

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 1, 2023

📌 Commit fa3fa1d has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 1, 2023

⌛ Testing commit fa3fa1d with merge 878bbb5...

bors added a commit that referenced this pull request Nov 1, 2023
Update auxvec.h for Android and Linux

The NDK automatically generates these constants from Linux kernel headers, and they are available for all architectures. They are also repeated by musl and glibc, though this is mostly just a restatement to make use with getauxval more convenient.

New in this patch: `AT_MINSIGSTKSZ`

This const is needed to handle new extensions to platform arches that add state to save to signal stacks, replacing `MINSIGSTKSZ` with an ELF-loaded equivalent.
@bors
Copy link
Contributor

bors commented Nov 1, 2023

💔 Test failed - checks-actions

@bors
Copy link
Contributor

bors commented Nov 1, 2023

⌛ Testing commit fa3fa1d with merge 5856fb7...

bors added a commit that referenced this pull request Nov 1, 2023
Update auxvec.h for Android and Linux

The NDK automatically generates these constants from Linux kernel headers, and they are available for all architectures. They are also repeated by musl and glibc, though this is mostly just a restatement to make use with getauxval more convenient.

New in this patch: `AT_MINSIGSTKSZ`

This const is needed to handle new extensions to platform arches that add state to save to signal stacks, replacing `MINSIGSTKSZ` with an ELF-loaded equivalent.
@bors
Copy link
Contributor

bors commented Nov 1, 2023

💔 Test failed - checks-actions

@JohnTitor
Copy link
Member

Failed on i686:

  cargo:warning=/checkout/target/i686-linux-android/debug/build/libc-test-aa3b5ef22d9f3b80/out/main.c:25113:75: error: use of undeclared identifier 'AT_MINSIGSTKSZ'

  cargo:warning=            static const unsigned  long __test_const_AT_MINSIGSTKSZ_val = AT_MINSIGSTKSZ;

@workingjubilee
Copy link
Member Author

@JohnTitor I don't really understand the nature of this test so I don't know what I'm supposed to be doing here?

@tgross35
Copy link
Contributor

@workingjubilee it's just a test that the identifiers are available on the host platforms, which I guess that one wasn't. It would just need an exception added to libc-test/build.rs.

Might be worth just rebasing to see if it works first, CI was updated kinda recently so maybe it fixed itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants