-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Split glibc and linux sigset_t ABIs and the accessor functions #23601
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
Draft
rootbeer
wants to merge
5
commits into
ziglang:master
Choose a base branch
from
rootbeer:sig-split
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+368
−125
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexrp
reviewed
Apr 18, 2025
alexrp
reviewed
Apr 18, 2025
alexrp
reviewed
Apr 18, 2025
alexrp
reviewed
Apr 18, 2025
alexrp
reviewed
Apr 18, 2025
alexrp
reviewed
Apr 18, 2025
Thanks for quick the review! I've got some test failures to dig into, in addition to your feedback to address. I'll bring this back from a draft when its ready for another look. |
Export the sigset_t ops (sigaddset, etc) from the C library. Don't rely on the linux.zig defintions (which will be defined to use the kernel ABI). Move Darwin sigset and NSIG declarations into darwin.zig. Remove extraneous (?) sigaddset. The C library sigaddset can reject some signals being added, so need to defer to it.
The kernel ABI sigset_t is smaller than the glibc one. Define the right-sized sigset_t and fixup the sigaction() wrapper to leverage it. The Sigaction wrapper here is not an ABI, so relax it (drop the "extern" and the "restorer" fields), the existing `k_sigaction` is the ABI sigaction struct. Linux defines `sigset_t` with a c_ulong, so it can be 32-bit or 64-bit, depending on the platform. This can make a difference on big-endian systems. Patch up `ucontext_t` so that this change doesn't impact its layout. AFAICT, its currently the glibc layout.
Unify the C library sigset_t and Linux native sigset_t and the accessor operations. Add tests that the various sigset_t operations are working. And clean up existing tests a bit.
When linking a libc, Zig should defer to the C library for sigset operations. The pre-filled constants signal sets (empty_sigset, filled_sigset) are not compatible with C library initialization, so remove them and use the runtime `sigemptyset` and `sigfillset` methods to initialize any sigset.
…gset_t By returning an initialized sigset (instead of taking the set as an output parameter), these functions can be used to directly initialize the `mask` parameter of a `Sigaction` instance.
Looks like I still have some work to do ...
So far all I know is that |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Zig's
c.zig
glibc/musl layer and thelinux.zig
kernel syscall wrapper layers both definesigset_t
. Currentlylinux.zig
uses a definition compatible with glibc/musl, and thenc.zig
imports that definition. The glibc ABI defines a 1024-bitsigset_t
. The linux kernel ABI generally defines a 64-bitsigset_t
(or 128-bit on MIPS). At run-time, glibc (and Zig) just presents the lowest bits of the largersigset_t
to the kernel. Glibc "needs" the extra space in its ABI so it can accommodate future kernels that support a larger number of signals. Zig does not need that compatibility unless linking the C library. Thelinux.zig
ABI should be able to define asigset_t
that matches the underlying kernel ABI.The array of words backing
sigset_t
array must match the kernel's layout. Two 32-bit words are not the same as one 64-bit word on a big-endian platform. The kernel headers useunsigned long
as the element, so I usedc_ulong
for consistency. This means the array is sometimes of 32-bit words, and sometimes made from 64-bit words. I added some tests that make sure blocking specific signals works to check we're getting the bit indexes correct. (In practice most signal numbers are less than 32, so this wasn't much of a practical problem.)The
sigfillset()
call needs to go through the dynamically linked C library, when one is present. The run-time library might decide to reserve some signals. Similarly,sigaddset()
might reject some signal numbers reserved by the C library. Also, Zig's pre-filledfilled_sigset
can't work when linking a C library, because Zig can't invoke external symbols in a static initializer (at least, that's what I understood the error I got to mean). This PR provides POSIX-likesigemptyset()
andsigfillset()
functions, but in Zig they return the initialized set (instead of taking the set as an output parameter). This PR also removeslinux.all_mask
.Sigaction
contains asigset_t
. In Zig this structure is already mapped (in thesigaction
wrapper) into ak_sigaction
that has a "restorer" field and an ad-hoc, right-sized signal mask field. With a properly definedsigset_t
, some differences instd.os.linx.Sigaction
versions collapse (e.g., the MIPS variations were just encapsulatingsigset_t
differences.) The Zig-onlySigaction
structure doesn't need a restorer, so remove that. Also doesn't need to be anextern
struct.The
context_t
structure also contains a glibc-compatiblesigset_t
. I made that explicit in this PR, and avoided fixing it for now, as I think there are deeper changes required around this structure.Using a
u6
for the signal number is too narrow. Signal numbers can go up to 128 on some platforms. Changed to au8
parameter since the actual boundary isn't always a power of two. Means we need to@truncate
when converting signal numbers into bit masks in some places.I am unclear about the darwin.zig changes. I'm not sure if the sigset accessors (
sigaddset()
,sigfillset()
, etc) are defined in the Darwin C library (but I assume they are?).Added a std.os.posix layer to hide differences (mostly just error handling) between C-library sigset manipulators and the direct linux kernel accessor. I can drop these if folks would rather not see new code added to posix, but they were handy for making tests to make sure all the implementations are consistent.
PR inspired by conversation on https://github.com/ziglang/zig/pull/23502\#discussion\_r2034064507