Skip to content

Conversation

@ryanschneider
Copy link
Contributor

@ryanschneider ryanschneider commented Nov 3, 2022

This fix (proposed independently by @Vexu here (#13216 (comment)) and @marler8997 here (marler8997/ziglibc#3 (comment)) allows one to pass os.SIG.IGN or other non-pointer aligned values for the handler_fn on word-aligned architectures like aarch64.

The PR is two separate commits simply because this is my first PR to zig and I wanted to make sure that the CI picks up the failing case first and will be fixed w/ the 2nd commit, IMO the PR can be squashed before merging.

I added the align(1) to all the lib/std/c implementations, even the ones like dragonfly that only (currently) target x86 for consistency but am happy to remove those changes if preferred.

Fixes #13216.

@ryanschneider ryanschneider marked this pull request as ready for review November 3, 2022 02:04
@ryanschneider
Copy link
Contributor Author

Hmm just realized that the existing test calls w/ RESETHAND so my test additions don't fully test that IGN works as expected (but do confirm that the alignment issues are fixed), I'll improve the test a bit more.

@ryanschneider
Copy link
Contributor Author

Ok I namespaced the handler_called_count and made sure the test fully tests the IGN case. PT(another)L.

@ryanschneider ryanschneider requested a review from matu3ba November 3, 2022 15:46
@ryanschneider ryanschneider force-pushed the signal-alignment-13216 branch from e652829 to d8b47ca Compare November 3, 2022 16:25
@matu3ba
Copy link
Contributor

matu3ba commented Nov 4, 2022

ci.ziglang fails in debug mode with

+ echo Looking for non-conforming code formatting...
+ stage3/bin/zig fmt --check .. --exclude ../test/cases/ --exclude ../build-debug --exclude ../build-release --exclude /workspace/zig-cache-local-debug --exclude /workspace/zig-cache-global-debug
Looking for non-conforming code formatting...
../lib/std/os/test.zig

Is there maybe some unformatted code?

@ryanschneider ryanschneider force-pushed the signal-alignment-13216 branch from d8b47ca to 31ac4f9 Compare November 4, 2022 14:59
@ryanschneider ryanschneider force-pushed the signal-alignment-13216 branch from 31ac4f9 to bcaa439 Compare November 4, 2022 15:00
@ryanschneider
Copy link
Contributor Author

Oops ya I thought I had my editor setup to run zig fmt but must not have it configured fully correctly, it didn't catch some of the empty lines I added having spaces on them, repushed just now will check the CI results when done.

@ryanschneider ryanschneider force-pushed the signal-alignment-13216 branch from bcaa439 to a68b27c Compare November 4, 2022 15:07
Copy link
Contributor

@matu3ba matu3ba left a comment

Choose a reason for hiding this comment

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

Not sure though, if some align(1) are missing on some OSes.

@Vexu Vexu merged commit 41b7e40 into ziglang:master Nov 9, 2022
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Jan 9, 2023
@andrewrk andrewrk added this to the 0.10.1 milestone Jan 9, 2023
andrewrk pushed a commit that referenced this pull request Jan 9, 2023
std.os: fix alignment of Sigaction.handler_fn
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.

can't compile std.os.SIG.IGN on aarch64 Linux and Darwin

4 participants