fix arm64 alignment issues in signal()

[ryanschneider]

I tried building on an arm64 linux host and ran into this compilation error:

/home/ryanschneider.linux/ziglibc/src/cstd.zig:560:45: error: pointer type '?*const fn(c_int) callconv(.C) void' requires aligned address
                return @intToPtr(?SignalFn, @bitCast(usize, @as(isize, -1)));
                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The issue seemed to be that since arm64 align is higher than 1 byte, -1 ends up as an invalid pointer value, so the compiler stops us from returning it.

I'm still very new to zig, so explored a couple work-arounds:

• option 1: try messing w/ @alignOf etc. I never found an option that let me proceed w/ returning -1 as a ?SignalFn, so ultimiately gave up on this approach.
• option 2: return anyopaque or some other void*-like option. I considered this approach but didn't like losing the type of the result.
• option 3: return a packed union: this is the option I ended up implementing after looking around at other zig code.

I think this is idiomatic zig (or at least as idiomatic as one can get emulating signal :) ) but am welcome to feedback or changes, or feel free to ignore.

[marler8997]

Funny, I tried this exact same technique of wrapping a pointer in a union to avoid alignment issues in the win32 bindings for Zig (see microsoft/win32metadata#623). Unfortunately, platforms are free to change the ABI of a function depending on if it's a primitive value like a pointer or a struct/union, even if they are the same size. So this gives us no guarantee of ABI compatibility.

As for the other options, option 1 would mean using @alignCast, but, zig can and will enforce the alignment at runtime which would lead to a panic in this case. Option 2 would work, but, can be improved. Instead of using anyopaque, we can continue to use the function pointer type but just annotate it as having no alignment with align(1), i.e.

const SignalFn = switch (builtin.zig_backend) {
    // TODO: don't know what stage1 will do with an unaligned function pointer?
    .stage1 => fn(c_int) callconv(.C) void,
    else => *align(1) const fn(c_int) callconv(.C) void,
};

There's also one more option, namely, not using -1 for SIG_ERR. However, which value could we use? We could pick a value that tries to be maximally aligned for all platforms, 16 might work.

All this being said, I think I prefer the improved option 2 for now. It's a simple easy fix and I don't really see much downside to it.

[ryanschneider]

Awesome, thanks for the reply and the link to the win32 discussion, I didn’t know about ABI distinctions like that either! Given that I totally agree that `align(1)` is the best option. Replying over email now but will close my PR when I’m back at my desk.

…

On Thu, Oct 27, 2022 at 4:50 AM Jonathan Marler ***@***.***> wrote: Funny, I tried this exact same technique of wrapping a pointer in a union to avoid alignment issues in the win32 bindings for Zig (see microsoft/win32metadata#623 <microsoft/win32metadata#623>). Unfortunately, platforms are free to change the ABI of a function depending on if it's a primitive value like a pointer or a struct/union, even if they are the same size. So this gives us no guarantee of ABI compatibility. As for the other options, option 1 would mean using @alignCast, but, zig can and will enforce the alignment at runtime which would lead to a panic in this case. Option 2 would work, but, can be improved. Instead of using anyopaque, we can continue to use the function pointer type but just annotate it as having no alignment with align(1), i.e. const SignalFn = switch (builtin.zig_backend) { // TODO: don't know what stage1 will do with an unaligned function pointer? .stage1 => fn(c_int) callconv(.C) void, else => *align(1) const fn(c_int) callconv(.C) void, }; There's also one more option, namely, not using -1 for SIG_ERR. However, which value could we use? We could pick a value that tries to be maximally aligned for all platforms, 16 might work. All this being said, I think I prefer the improved option 2 for now. It's a simple easy fix and I don't really see much downside to it. — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAANCEH6SVBUWBHNJUZDRZ3WFJT7TANCNFSM6AAAAAARPGZCCA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[ryanschneider]

FWIW I tried your align(1) suggestion but then run into an error creating the os.std.Sigaction struct:

/home/ryanschneider.linux/ziglibc/src/cstd.zig:544:38: error: expected type '?*const fn(c_int) callconv(.C) void', found '*align(1) const fn(c_int) callconv(.C) void'
            .handler = .{ .handler = func },
                                     ^~~~
/home/ryanschneider.linux/ziglibc/src/cstd.zig:544:38: note: pointer alignment '1' cannot cast into pointer alignment '4'

I think I worked around that via use of @alignCast, in that I can run gdb zig-out/bin/yacc and confirm that signal is called w/o crashing, but can't say for certain I did the alignment change correctly (and definitely not cleanly, not sure if there's a simpler signature I could cast to instead of the full handler signature, for example).

Updated my PR to include those changes instead.

[marler8997]

That alignCast will panic if a user passes SIG.IGN for example. You've found a bug in the Zig std library, which, probably only occurs on ARM64 since it hasn't been caught yet. Fix will be to modify the alignment of handler_fn to be align(1) in the Zig std library. I can't make a PR right now, feel free to make one yourself and let me know if you do. If not I"ll try to remember to make one later.

[ryanschneider]

I took a stab at trying to make a PR for the std library, but I think it's getting over my head. :). I'll open an issue at the very least and document what I tried but couldn't get to work over there. Thanks for the pointers I'll update here w/ a link to the issue when done.

[ryanschneider]

Oh hah, I should've started out by searching for any similar issues: ziglang/zig#13216. but at least this way I got somewhat setup to test the std library locally.

[ryanschneider]

BTW I started on the upstream PR here: ziglang/zig#13418. taking it somewhat slow since this is my first PR and wanted to make sure I do everything correctly.

[marler8997]

Now that your PR for the std library is in, I think you should be able to remove the @alignCast in this PR.

[ryanschneider]

Ok, I removed the alignCast, but unfortunately haven't been able to test it yet, I'm getting a segfault in any new zig binaries I compile locally under arm64 linux, and it doesn't look like the CI/CD infra produces arm64 linux tarballs. I can't tell if it's something that I've messed up on my end (most likely).

[matu3ba]

@ryanschneider The CI now uses aarch64 = arm64 for windows, mac and linux: https://github.com/ziglang/zig/tree/master/ci now and they are also available as download: https://ziglang.org/download/.