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

Rewrite Linux syscalls generation / Migrate to 64-bit time #21440

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

The-King-of-Toasters
Copy link
Contributor

@The-King-of-Toasters The-King-of-Toasters commented Sep 18, 2024

Changes by Arnd Bergmann have migrated all supported architectures to use a table for their syscall lists. This removes the need of calling a C pre-processor and simplifies the logic dramatically.
The side effect is the number of names changed on targets that use the "generic" table. That list (located under scripts/syscall.tbl) adds the _time64 suffix to syscalls taking a timespec64 on 32-bit targets. Similarly, the _time32 suffix has been removed.

The result is a lot of breakage in our Linux wrappers, which makes me worried that the logic for determining the proper timespec to use was subtly broken all this time. Should be a good chance to finish #4726 - we only have 14 years after all...

Closes #21738.

@alexrp
Copy link
Member

alexrp commented Sep 18, 2024

The result is a lot of breakage in our Linux wrappers, which makes me worried that the logic for determining the proper timespec to use was subtly broken all this time.

This situation is highly confusing because timespec and kernel_timespec are not the same thing, and the latter is usually what you want. But yes, the time stuff needs to be cleaned up and the standard library probably just wholesale switched to time64. (I can't think of a good reason why we would want to maintain time32 support?)

@The-King-of-Toasters
Copy link
Contributor Author

Kinda scary that the CI passed 🙃 - do we not test for time overflow?

@The-King-of-Toasters The-King-of-Toasters changed the title Rewrite Linux syscalls generation Rewrite Linux syscalls generation / Migrate to 64-bit time Sep 29, 2024
@The-King-of-Toasters
Copy link
Contributor Author

Time to finish #4726, starting with stat.
For any readers, expect a lot of breakage until this is ready for review.

@The-King-of-Toasters The-King-of-Toasters force-pushed the linux-6.11 branch 2 times, most recently from c970b18 to 2a35eb7 Compare September 29, 2024 10:12
lib/std/os/linux.zig Outdated Show resolved Hide resolved
lib/std/os/linux.zig Outdated Show resolved Hide resolved
Changes by Arnd Bergmann have migrated all supported architectures to
use a table for their syscall lists. This removes the need of calling a
C pre-processor and simplifies the logic dramatically.
The generic syscall table has different names for syscalls that take a
timespec64 on 32-bit targets, in that it adds the `_time64` suffix.
Similarly, the `_time32` suffix has been removed.

I'm not sure if the existing logic for determining the proper timespec
struct to use was subtly broken, but it should be a good chance to
finish ziglang#4726 - we only have 14 years after all...

In other news:

- x86_64 gets `uretprobe`, a syscall to speed up returning BPF probes.
- Hexagon gets `clone3`, but don't be fooled: it just returns ENOSYS.
As per ziglang#21738, the minimum kernel has been bumped to 5.11, and glibc to
2.34. The maximum has also been updated to the new 6.11 release.
@The-King-of-Toasters The-King-of-Toasters force-pushed the linux-6.11 branch 2 times, most recently from efc5103 to f5567d5 Compare October 29, 2024 01:02
`std.os.linux` has been reworked to use 64-bit time APIs on all targets.
This required auditing all types used by the Kernel ABI for each of the
supported targets, along with other some rewrites in `std.posix`.

= `std.os.linux`

The `Stat` structures for each target are now merged together into the
`KernelStat` struct that switches on the target, similar to existing
types. Targets that have a `stat64` structure have been modified to make
that it's default, and all definitions have been re-written to match
the kernel headers exactly using the `c_` int types.

Of course, newer linux ports don't even implement the `stat(2)` family,
instead requiring userspace to wrap it using `statx(2)`. Thus, a new
`Stat` type has been created to hold information from both APIs. The new
public variable `has_fstatat` has also been introduced, so that callers
can check if the current target at least implements `fstatat(2)`, and to
use `statx(2)` if not. The `major`, `minor` and `makedev` functions have
also been ported over to make the translation from `statx(2)` possible.

== `timespec`

The KernelStat `(a|c|m)time` fields are no longer defined as
`timespecs`, since their signedness and bit size vary by target.
Instead, the new `timespec.makeTimespec` function is used, which does
some comptime checks on the time types before performing `@intCasts` on
each field.

Speaking of, the `timespec` type has been redefined to be the same as
`__kernel_timespec`, as it is the modern 64-bit type that the kernel is
using going forward. Since some syscalls (e.g. `timerfd_(get|set)time`)
require the `timespec64` type, it has been added as well. Note that the
only difference between it and `__kernel_timespec` is that `ts_nsec` is
defined as a `c_long`, thus explicit padding fields are added and zeroed
out for 32-bit targets to avoid issued with uninitialised memory.

== Misc.

- The VDSO `clock_gettime` symbol now points to the proper 64-bit
  verrsion for each arch.
- The `Time64` struct has been created to hold all the proper `time64`
  syscalls for each target.

= `std.posix`

- Add `fstatatLinux` that either uses `fstatat` or `statx` depending on
  the value of `linux.has_fstatat`.
- Move `std.os.fstat(at)_wasi` into the `std.posix.fstat(at)Wasi` in
  light of ziglang#21023.
- Move the libc path for `fstatat` into the new function `fstatatC`.
- Mark `fstatatZ` as `inline` since the logic is simplified.
This commit follows the work done in `std.os.linux`, in that the `Stat`,
`time_t` and `timespec` types have been audited against the libc
definitions and fixed appropriately.

Targeting the `largefile` and `time64` functions require linking to
specific symbols. In order not to over-complicate `std.posix`, which
already uses `lfs64_abi`, the logic for selecting the right function has
been moved into `std.c`. These functions are imported from the new file
`vlfts.zig` along with the two options `largefile_abi` and `time64_abi`.
This allows `std.c` to select the proper symbol for e.g. fstatat, which
could be one of the following:

- `fstatat`.
- `fstatat64`.
- `__fstatat64_time64`.
- `fstatat_time64`.

Simple, isn't it...
@The-King-of-Toasters The-King-of-Toasters marked this pull request as ready for review October 29, 2024 11:38
@The-King-of-Toasters
Copy link
Contributor Author

I'm marking this ready for review as I believe the aarch64 runners are simply using an older version of glibc. While that is fixed, I'd like some feedback on my changes.

@alexrp
Copy link
Member

alexrp commented Nov 2, 2024

Looks like I caused a merge conflict here with #21860.

@alexrp alexrp self-requested a review November 12, 2024 15:43
.arm,
.armeb,
.csky,
.hexagon,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.hexagon,

Upstream glibc/musl don't support Hexagon (yet).

.sparc,
.thumb,
.thumbeb,
.xtensa,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.xtensa,

Upstream glibc/musl don't support xtensa.

// 64-bit targets running in a 32-bit mode.
.mips64,
.mips64el,
=> builtin.abi == .gnuabin32,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
=> builtin.abi == .gnuabin32,
=> builtin.abi == .gnuabin32 or builtin.abi == .muslabin32,

?

.mips64,
.mips64el,
=> builtin.abi == .gnuabin32,
.x86 => builtin.abi != .gnux32,
Copy link
Member

Choose a reason for hiding this comment

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

gnux32 only applies to x86_64. (Also note that muslx32 exists.)

rdev: dev_t,
__pad1: [2]i32,
size: off_t,
.x86_64 => extern struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct for native_abi == .gnux32?

return self.ctim;
}
},
.mips64, .mips64el => extern struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct for native_abi == .muslabin32?

return self.ctim;
}
},
.x86_64 => extern struct {
Copy link
Member

Choose a reason for hiding this comment

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

Is this correct for native_abi == .muslx32?

/// - Modern Linux ports will not implement `fstatat(2)`,
/// in favor of libraries like libc using `statx(2)` to implement it anyway.
///
/// See `std.os.fstatat_linux` for how this is done.
Copy link
Member

Choose a reason for hiding this comment

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

This function doesn't seem to exist.

}

/// Same as `fstatat` except it uses `statx(2)` if the target does not implement `fstatat(2)`.
fn fstatatLinux(
Copy link
Member

@alexrp alexrp Nov 13, 2024

Choose a reason for hiding this comment

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

As discussed earlier, this kind of emulation gets too far into libc territory. We don't want to be doing that, especially since this requires making up a Stat type that isn't actually part of the real ABI.

Is there a particular reason you implemented this? Note that std.fs already uses statx() on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My reasoning was that there is a lot of code that calls std.os.fstat(at) directly, and having it return a compile error if the fstatat syscall isn't implemented won't be a great experience when people upgrade to newer zig versions. I'm fine removing it, but I would like a definitive answer either way (perhaps this can be added to #21738).

Copy link
Member

Choose a reason for hiding this comment

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

But does this affect anything other than arc and riscv32? (Support for those architectures is new in this Zig release anyway.)

Copy link
Member

Choose a reason for hiding this comment

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

cc @andrewrk given our recent discussion on the future of std.posix.

@The-King-of-Toasters
Copy link
Contributor Author

I am going to be busy this week (and possibly the next) with education stuff. If you're itching to fix some stuff, feel free.

@@ -505,11 +505,11 @@ pub const Os = struct {
.linux => .{
.linux = .{
.range = .{
.min = .{ .major = 4, .minor = 19, .patch = 0 },
.max = .{ .major = 6, .minor = 10, .patch = 3 },
.min = .{ .major = 5, .minor = 1, .patch = 0 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I miss something, but in this line Linux kernel minimum is set to 5.1. not to 5.11 like a16b32a says?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I just learned that "Pending" meant I need to submit review, I thought it meant "waiting for reply from author" and thought why no one noticed my comment for so long :)

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.

Proposal: A plan for the Linux 64-bit time migration in Zig
3 participants