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
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 54 additions & 3 deletions src/cpu/arm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,62 @@ fn detect_features() -> u32 {
use libc::c_ulong;

// XXX: The `libc` crate doesn't provide `libc::getauxval` consistently
// across all Android/Linux targets, e.g. musl.
// 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.

extern "C" {
fn getauxval(type_: c_ulong) -> c_ulong;
}

#[cfg(not(target_env = "uclibc"))]
fn getauxval_safe(type_: c_ulong) -> c_ulong {
// safety: getauxval seems to have no safety concerns
unsafe { getauxval(type_) }
}

// Provide a compatible implementation for uclibc based on procfs.
#[cfg(all(target_env = "uclibc"))]
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!

};
if fd == -1 {
return 0;
}
const AUXV_LEN: usize = core::mem::size_of::<c_ulong>();

let val = 'done: loop {
let mut keyval = [0u8; AUXV_LEN * 2];
let mut read = 0;
let total = core::mem::size_of_val(&keyval);
while read < total {
let block = &mut keyval[read..];
let cnt = unsafe {
// safety: memory block length is managed by rust.
libc::read(fd, block.as_mut_ptr() as *mut libc::c_void, block.len())
};
if cnt <= 0 {
break 'done 0;
}
read += cnt as usize;
}
let key = c_ulong::from_ne_bytes(keyval[0..AUXV_LEN].try_into().unwrap());
let val = c_ulong::from_ne_bytes(keyval[AUXV_LEN..].try_into().unwrap());
if key == type_ {
break 'done val;
}
};

let _ = unsafe {
// safety: fd was valid above, so it is still valid.
libc::close(fd)
};

val
}

const AT_HWCAP: c_ulong = 16;

#[cfg(target_arch = "aarch64")]
Expand All @@ -38,7 +89,7 @@ fn detect_features() -> u32 {
#[cfg(target_arch = "arm")]
const HWCAP_NEON: c_ulong = 1 << 12;

let caps = unsafe { getauxval(AT_HWCAP) };
let caps = getauxval_safe(AT_HWCAP);

// We assume NEON is available on AARCH64 because it is a required
// feature.
Expand All @@ -61,7 +112,7 @@ fn detect_features() -> u32 {
#[cfg(target_arch = "arm")]
let caps = {
const AT_HWCAP2: c_ulong = 26;
unsafe { getauxval(AT_HWCAP2) }
getauxval_safe(AT_HWCAP2)
};

const HWCAP_AES: c_ulong = 1 << 0 + OFFSET;
Expand Down