Skip to content

Enable statx on supported musl versions #154981

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.

Sign up for GitHub

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

Open
strophy wants to merge 1 commit into
base: main
Choose a base branch
from
+7 −7
Open

Enable statx on supported musl versions #154981

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 7 additions & 7 deletions library/std/src/sys/fs/unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#[cfg(test)]
mod tests;

#[cfg(all(target_os = "linux", target_env = "gnu"))]
#[cfg(target_os = "linux")]
use libc::c_char;
#[cfg(any(
all(target_os = "linux", not(target_env = "musl")),
Expand Down Expand Up @@ -93,7 +93,7 @@ use crate::sys::fd::FileDesc;
pub use crate::sys::fs::common::exists;
use crate::sys::helpers::run_path_with_cstr;
use crate::sys::time::SystemTime;
#[cfg(all(target_os = "linux", target_env = "gnu"))]
#[cfg(target_os = "linux")]
use crate::sys::weak::syscall;
Comment on lines -96 to 97
Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 23, 2026 •
edited by rustbot
Loading

Choose a reason for hiding this comment

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

This still doesn't seem quite right. Why are we using weak linkage and dynamic detection when we know the version of musl we link?

View changes since the review

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 24, 2026

Choose a reason for hiding this comment

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

Some discussion about this pattern here #t-libs > Problems with `syscall!`

Copy link
Copy Markdown
Author

@strophy strophy Apr 27, 2026

Choose a reason for hiding this comment

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

I'm more of a package maintainer and am relatively unfamiliar with both Rust and libc, so the conversation you referenced is a bit over my head unfortunately. What is the preferred approach here, how should I amend my PR?

Copy link
Copy Markdown
Contributor

@tgross35 tgross35 Apr 27, 2026

Choose a reason for hiding this comment

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

Would it be possible to config things such that statx remains "maybe available" on GNU with syscall! and the fallback, like it is now, but then on musl it's treated as unconditionally available? I.e. call libc::statx directly.

If this isn't easily possible then it's probably fine, this config is pretty messy. But the Zulip thread I linked is talking about how LTO is kind of buggy when there are both strong (calling the function directly) and weak (using syscall!) definitions, which I expect we may hit with the current implementation.

Copy link
Copy Markdown
Member

@Darksonn Darksonn Apr 27, 2026

Choose a reason for hiding this comment

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

It sounds a lot simpler to me to just always use syscall! here, even if it could be slightly faster on musl.

#[cfg(target_os = "android")]
use crate::sys::weak::weak;
Expand All @@ -102,14 +102,14 @@ use crate::{mem, ptr};

pub struct File(FileDesc);

// FIXME: This should be available on Linux with all `target_env`.
// But currently only glibc exposes `statx` fn and structs.
// We don't want to import unverified raw C structs here directly.
// statx is available on Linux when:
// - target_env = "gnu": added in glibc >= 2.28
// - target_env = "musl": added in musl >= 1.2.5
// https://github.com/rust-lang/rust/pull/67774
macro_rules! cfg_has_statx {
Comment thread
strophy marked this conversation as resolved.
({ $($then_tt:tt)* } else { $($else_tt:tt)* }) => {
cfg_select! {
all(target_os = "linux", target_env = "gnu") => {
target_os = "linux" => {
$($then_tt)*
}
_ => {
Expand All @@ -118,7 +118,7 @@ macro_rules! cfg_has_statx {
}
};
($($block_inner:tt)*) => {
#[cfg(all(target_os = "linux", target_env = "gnu"))]
#[cfg(target_os = "linux")]
{
$($block_inner)*
}
Expand Down
Loading