-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Linux: Nuke Stat bits in favour of Statx #25639
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
base: master
Are you sure you want to change the base?
Conversation
Also removes the blank lines between members, and a comptime sizeOf check.
07f169b to
2550570
Compare
|
Forgot to compile the compiler and added a call to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me. I've got some questions, and ideas, but feel free to ignore them.
| .{ .major = 2, .minor = 28, .patch = 0 }); | ||
| const sys = if (use_c) std.c else std.os.linux; | ||
|
|
||
| var stx = std.mem.zeroes(Statx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little surprised to see this wrapper is stack allocating and zero'ing a statx buffer, instead of taking a pointer to a statx buffer and letting the caller decide if/how/when to allocate and zero it. AFAICT, none of the other syscall wrappers in here do anything quite like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because
- Most call sites used
posix.*stat. posix.*statdid the same thing.
My understanding is that the wrappers act like posix in that they translate errors and allocate structures for you. Happy to change if not.
| test "linkat with different directories" { | ||
| if ((builtin.cpu.arch == .riscv32 or builtin.cpu.arch.isLoongArch()) and builtin.os.tag == .linux and !builtin.link_libc) return error.SkipZigTest; // No `fstatat()`. | ||
| if (builtin.cpu.arch.isMIPS64()) return error.SkipZigTest; // `nstat.nlink` assertion is failing with LLVM 20+ for unclear reasons. | ||
| fn getLinkInfo(fd: posix.fd_t) !struct { posix.ino_t, posix.nlink_t } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
That seems reasonable for this test. |
a13f0bf to
09ec5b7
Compare
Maintaining the POSIX `stat` bits for Zig is a pain. Not only is `struct stat` incompatable between architectures, but maddingly annoying so; timestamps are specified as machine longs or fixed-width ints, members can be signed or unsigned. The libcs deal with this by introducing the own version of `struct stat` and copying the kernel structure members to it. In the case of glibc, they did it twice thanks to the largefile transition! In practice, the project needs to maintain three versions of `struct stat`: - What the kernel defines. - What musl wants for `struct stat`. - What glibc wants for `struct stat64`. Make sure to use `fstatat64`! And it's not as simple as running `zig translate-c`. In ziglang#21440 I had to create test programs in C and use `pahole` to dump the structure of `stat` for each arch, and I was constantly running into issue regarding padding and signed/unsigned ints. The fact that so many target checks in the `linux` and `posix` tests exist is most likely due to writing to padding bits and failing later. The solution to this madness is `statx(2)`: - It takes a single structure that is the same for all arches AND libcs. - It uses a custom timestamp format, but it is 64-bit ready. - It gives the same info as `fstatat(2)` and more! - Unlike `fstatat(2)`, you can request a subset of the info required based on passing a mask. It's so good that modern Linux arches (e.g. riscv) don't even implement `stat`, with the libcs using a generic `struct stat` and copying from `struct statx`. Therefore, this commit rips out all the `stat` bits from `std.os.linux` and `std.c`. `std.posix.Stat` is now `void`, and calling `std.posix.*stat` is an compile-time error. A wrapper around `statx` has been added to `std.os.linux`, and callers have been upgraded to use it. Tests have also been updated to use `statx` where possible. While I was here, I converted the mask and file attributes to be packed struct bitfields. A nice side effect is checking that you actually recieved the members you asked for via `Statx.mask`, which I have used by adding `assert`s at specific callsites. In the future I expect types like `mode_t`/`dev_t` to be audited and removed, as they aren't being used to define members of `struct stat`.
Commit #fc7a5f2 moved many of the `_t` types up a level, but didn't remove them from arch_bits. Since `Stat` is gone, all but `time_t` can be removed.
09ec5b7 to
f7a6449
Compare
|
Would you mind opening a mirror PR on Codeberg for LoongArch and RISC-V CI? |
|
Also, it doesn't have to be done in this PR, but it would be nice to decouple |
Done. |
|
@The-King-of-Toasters could you adapt the structure I used in #25394 for the typed flags changes in Line 7064 in 321a739
Line 7122 in 321a739
STATX_ATTR_* and STATX_MASK_* constants to easily find it when searching for them on https://ziglang.org/documentation/master/std/ which I think seems to be the main motivation for most of the identifiers been UPPER_CASE in linux.zig. We can do better in Zig 😄 than C, and this would also help to reduce conflicts when #25394 is been merged.
|
Maintaining the POSIX
statbits for Zig is a pain. Not only isstruct statincompatable between architectures, but maddingly annoying so; timestamps are specified as machine longs or fixed-width ints, members can be signed or unsigned. The libcs deal with this by introducing the own version ofstruct statand copying the kernel structure members toit. In the case of glibc, they did it twice thanks to the largefile transition!
In practice, the project needs to maintain three versions of
struct stat:struct stat.struct stat64. Make sure to usefstatat64!And it's not as simple as running
zig translate-c. In #21440 I had to create test programs in C and usepaholeto dump the structure ofstatfor each arch, and I was constantly running into issue regarding padding and signed/unsigned ints. The fact that so many target checks in thelinuxandposixtests exist is most likely due to writing to padding bits and failing later.The solution to this madness is
statx(2):fstatat(2)and more!fstatat(2), you can request a subset of the info required based on passing a mask.It's so good that modern Linux arches (e.g. riscv) don't even implement
stat, with the libcs using a genericstruct statand copying fromstruct statx.Therefore, this PR rips out all the
statbits fromstd.os.linuxandstd.c.std.posix.Statis nowvoid, and callingstd.posix.*statis an compile-time error. A wrapper aroundstatxhas been added tostd.os.linux, and callers have been upgraded to use it. Tests have also been updated to usestatxwhere possible.While I was here, I converted the mask and file attributes to be packed struct bitfields. A nice side effect is checking that you actually received the members you asked for via
Statx.mask, which I have used by addingasserts at specific callsites.Progress towards #21738