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

enable builds on target_env = uclibc by providing a getauxval implementation #1800

Closed
wants to merge 1 commit into from

Conversation

skrap
Copy link
Contributor

@skrap skrap commented Nov 9, 2023

Currently, ring does not build on uclibc, a libc alternative used for embedded platforms. This patch enables building for uclibc targets, such as armv7-unknown-linux-uclibceabihf. Note that I'm the designated developer for this tier-3 platform.

The issue is that uclibc does not implement the nonstandard glibc function getauxval. Building for uclibc results in a linker error due to the lack of getauxval.

This patch provides a simple implementation only for target_env = "uclibc" which only depends on the already-present libc crate.

This is my first contribution to ring. Thanks for all your work on this crate!

@briansmith
Copy link
Owner

Thanks for the contribution.

I noticed that uClibc-ng does seem to support getauxval in some configurations. In particular, it seems to support it when built as a shared library? Is this only a problem when statically linking uClibc-ng? Or are we trying to support older versions of uClibc-ng that don't implement getauxval?

If so, why not improve uClibc-ng so that it supports getauxval in static configurations too?

Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, if uclibc doesn't want to implement getauxval because of its intent to be very very small (what other reason would they have for intentionally not supporting it?), and uclibc is primarily used on embedded targets where the CPU feature set is known and fixed, then another solution would be to simply not do dynamic feature detection when uclibc is not providing getauxval. Note that we already fully support static CPU feature detection for aarch64 and arm.

fn getauxval_safe(type_: c_ulong) -> c_ulong {
let fd = unsafe {
// safety: static string has interned NUL terminator
libc::open(b"/proc/self/auxv\0".as_ptr(), libc::O_RDONLY)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just use libstd here since the target_os is linux and libstd is supported by this target. Thus we could avoid the unsafe code completely, right?

OTOH, we've traditionally gone to some lengths to avoid File I/O whenever practical in this library. Is there really no way to find auxv in memory for uClibc-ng?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I originally did a libstd implementation, but then I guessed that it would be less preferred than one which didn't add any dependencies. Would you prefer I submit my libstd implementation?

glibc uses some tight integration with the kernel's ELF loader to find a spot in memory where the aux vector has been stored away. I could try to duplicate that here, but it felt much larger and more fragile, having looked at the recent addition of getauxval to uclibc here.

I get the desire to avoid file IO. Is it any consolation that no actual I/O device will be involved? procfs just talks with the kernel, which just printfs into the user space buffer, so it's very fast.

I'm very happy to move this in whatever direction you prefer!

// across all Android/Linux targets, e.g. uclibc.
// If weak linking of extern symbols becomes available, this code could
// choose to dynamically fall back to the procfs version below.
#[cfg(not(target_env = "uclibc"))]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If uclibc supports this when dynamically linking, then the conditional logic should include not(all(target_env = "uclibc", target_feature = "crt-static")). Since this is specific to Linux and not Android (why would we support uclibc on Android?), AFAICT, then we'd have not(all(target_env = "uclibc", target_os = "linux", target_feature = "crt-static")), or similar?

Copy link
Contributor Author

@skrap skrap Nov 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant "weak linking" in the sense of defining a symbol which would be non-null when available via the dynamic linker, or null if it were not available at dylib linking time. This is a feature which Rust has considered adding from time to time, mainly for situations these. But Rust doesn't have it (yet) so it's sorta a moot point. I can certainly remove this comment if it's causing confusion.

Here is the possible future Rust "linkage" feature which would support this case: rust-lang/rust#29603

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the comment about weak linking isn't confusing. (Also we do have weak linking with dlsym, right?)

In your other comment, you said we can't rely on uClibc having getauxval even when dynamically linked, so my comment regarding crt-static is moot, probably. However, the part where we should restrict this to linux (not do it for android) is still relevant.

@skrap
Copy link
Contributor Author

skrap commented Nov 9, 2023

I noticed that uClibc-ng does seem to support getauxval in some configurations. In particular, it seems to support it when built as a shared library? Is this only a problem when statically linking uClibc-ng? Or are we trying to support older versions of uClibc-ng that don't implement getauxval?

The support for getauxval in uclibc is pretty recent - only first appearing less than a year ago. So the issue is that most embedded distributions will not have this symbol for many years, rather than specifically a dynamic or static linking use case.

I wonder, if uclibc doesn't want to implement getauxval because of its intent to be very very small (what other reason would they have for intentionally not supporting it?), and uclibc is primarily used on embedded targets where the CPU feature set is known and fixed, then another solution would be to simply not do dynamic feature detection when uclibc is not providing getauxval. Note that we already fully support static CPU feature detection for aarch64 and arm.

I am fine with this solution, but I don't know how to implement it, as I don't know how to detect, at compile time, whether uclibc will be providing getauxval or not.

@briansmith
Copy link
Owner

I am fine with this solution, but I don't know how to implement it, as I don't know how to detect, at compile time, whether uclibc will be providing getauxval or not.

Basically what I'm suggesting is that we change this:

#[cfg(all(
    any(target_os = "android", target_os = "linux"),
    any(target_arch = "aarch64", target_arch = "arm")
))]
fn detect_features() -> u32 {

to

#[cfg(all(
    any(target_os = "android", target_os = "linux"),
    any(target_arch = "aarch64", target_arch = "arm"),
    not(target_env = "uclibc")
))]
fn detect_features() -> u32 {

and then in Feature::available(), make a similar change so that for uCLibc, we never look at OPENSSL_armcap_P at all. This should cause all feature detection to be done statically, and the compiler/linker should be able to automatically discard all the implementations that aren't enabled by target features, at least in optimized builds.

@skrap
Copy link
Contributor Author

skrap commented Nov 10, 2023

Ah, ok I understand now! Seems pretty simple. I could try doing that change. Probably Monday would be the next time I can work on this.

@skrap
Copy link
Contributor Author

skrap commented Nov 13, 2023

and then in Feature::available(), make a similar change so that for uCLibc, we never look at OPENSSL_armcap_P at all. This should cause all feature detection to be done statically, and the compiler/linker should be able to automatically discard all the implementations that aren't enabled by target features, at least in optimized builds.

I took a stab at this and filed it as this PR: #1805

If I understood your intent above correctly, we can close this PR and proceed with that one instead.

@skrap
Copy link
Contributor Author

skrap commented Nov 14, 2023

Closing in favor of #1805.

@skrap skrap closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants