Add sigaltstack syscall for Linux #2546

LemonBoy · 2019-05-23T22:25:00Z

#2380 is probably relevant here, this function is available on all the posix-compatible OS but we have to re-define it every time.

Also there's no c.zig implementation as I didn't know if stack_t had to be re-defined there or whatever.
The standard library has become way too tangled...

daurnimator · 2019-05-24T01:03:11Z

std/os/linux.zig

+pub const MINSIGSTKSZ = switch (builtin.arch) {
+    .i386, .x86_64, .arm => 2048,
+    .aarch64 => 6144,
+    else => @compileError("MINSIGSTKSZ not defined for this architecture"),


Doesn't this make things fail on all non-listed architectures?

Also we can grab all the values from the kernel, this search is useful: https://elixir.bootlin.com/linux/latest/ident/MINSIGSTKSZ

Note that so far all the per-arch things are in their own files (see e.g. linux/arm64.zig)

Doesn't this make things fail on all non-listed architectures?

Yes? IMO it's better to be explicit about missing definitions.

Also we can grab all the values from the kernel, this search is useful

That's not so easy, sometimes different libcs muddle the waters by changing that value. E.g. The aarch64 value is greater than the kernel-reported one. This begs another question, do we want to stay true to the kernel or try to paper over the kernel+libc differences?

Note that so far all the per-arch things are in their own files (see e.g. linux/arm64.zig)

See point 1, pub use masks the missing definitions unless you actually try to use it.

E.g. The aarch64 value is greater than the kernel-reported one

Is it? I thought it was only ia64 that was weird https://elixir.bootlin.com/linux/latest/source/arch/ia64/include/uapi/asm/signal.h#L95

do we want to stay true to the kernel or try to paper over the kernel+libc differences?

Usually I'd say "stay true to the kernel", but for the specific case of a "minimum space required", I'd take the larger of the two options (which in the case of ia64, is userspace)

See for yourself, here are glibc and musl definitions.

Interesting. Is this because libc reserves some stack space in signal handling for itself?
If that's the case, inline with my comment here, std/os/linux.zig should have the kernel definition, std/c/linux.zig should have the libc definition.

Is this because libc reserves some stack space in signal handling for itself?

Here's the rationale:

since the default (SIGSTKSZ) has always been 6k larger than the minimum, it is also increased to maintain the 6k usable by the signal handler. this happens to be able to store one pathname buffer and should be sufficient for calling any function in libc that doesn't involve conversion between floating point and decimal representations.

Any number is fine as long as you're able to satisfy the requirements imposed by the kernel.

I'm also interested in hearing your ideas on how to define this in c.zig, should stack_t be redefined there? Should it reference the definition in linux.zig?

the rationale:

So sounds like libc just adds some extra space "just in case": I would go with the kernel definition.
Though I could maybe be convinced to add a separate definition to std/c/linux.zig so that calling C libraries that assume enough space for a pathname buffer don't fail.

I'm also interested in hearing your ideas on how to define this in c.zig, should stack_t be redefined there? Should it reference the definition in linux.zig?

Unless the stack_t definition in std/c/linux.zig (note that it would not be in std/c.zig) needed to be something different to what the kernel provides, it would reference the definition in std/os/linux.zig.
i.e. stack_t would fall under the condition I outlined of:

If libc defines a function that e.g. "takes a pointer to a native struct sockaddr" then you may use type definitions from std/os/*

So sounds like libc just adds some extra space "just in case": I would go with the kernel definition.

The kernel prescribes the minimum amount of stack you need for an empty handler to execute, if you want to do anything remotely useful you need more space than that.

I agree on picking the kernel definition here, let's report the bare minimum and then have the user pick a good value.

Yep. The user should really be using the kernel minimum plus @stack_size(their_handler) (see #157 )

daurnimator · 2019-05-24T01:07:04Z

std/os/linux.zig

+};
+
+pub const stack_t = extern struct {
+    ss_sp: usize,


Should this be a [*]u8 or a *c_void?

Should this be a [*]u8 or a *c_void?

It's a matter of picking the poison you like the most, you either need a @pointerToInt or a @ptrCast.

With a pointer type you can't accidentally pass a number. Also pointers have additional properties such as alignment.

Oh well, some more type-safety is always welcome. I'll turn this into a *c_void for symmetry with the other structure definitions.

andrewrk

a couple things and then I think this is ready to merge. thanks daurnimator for the review comments

andrewrk · 2019-05-28T22:26:07Z

std/c/linux.zig

@@ -25,3 +25,5 @@ pub extern "c" fn getauxval(__type: c_ulong) c_ulong;

 pub const dl_iterate_phdr_callback = extern fn (info: *dl_phdr_info, size: usize, data: ?*c_void) c_int;
 pub extern "c" fn dl_iterate_phdr(callback: dl_iterate_phdr_callback, data: ?*c_void) c_int;
+
+pub extern "c" fn sigaltstack(ss: ?*stack_t, old_ss: ?*stack_t) c_int;


these can go in std/c.zig yeah? they're the same on every platform

They are, but they're not supported by every platform and that's why they've been duplicated.

I suppose that's better than a link error if accidentally used. Makes sense to me.

andrewrk · 2019-05-28T22:30:19Z

std/os/linux/test.zig

@@ -83,3 +83,12 @@ test "dl_iterate_phdr" {
    expect(linux.dl_iterate_phdr(usize, iter_fn, &counter) != 0);
    expect(counter != 0);
 }
+
+test "sigaltstack" {
+    var st: linux.stack_t = undefined;


can you add the wrapper function in std.os? Then this test can go into std/os/test.zig and test ~~all~~ most OS's including libc-linked builds. probably would have to be disabled for windows and wasi.

can you add the wrapper function in std.os

I thought that was only for abstractions that were valid across all the OSs.

it's a candidate for os.zig if:

it's available on more than 1 os (or available on libc and non libc for linux), or

it would be worthwhile to have zig-friendly function signature

andrewrk

I found the cause of the test failures

andrewrk · 2019-05-29T14:54:58Z

std/c/freebsd.zig

 extern "c" fn __error() *c_int;
 pub const _errno = __error;

 pub extern "c" fn getdents(fd: c_int, buf_ptr: [*]u8, nbytes: usize) usize;
+pub extern "c" fn sigaltstack(ss: ?*stack_t, old_ss: ?*stack_t) c_int;


wow I didn't catch this the first time.

Suggested change

pub extern "c" fn sigaltstack(ss: ?*stack_t, old_ss: ?*stack_t) c_int;

pub extern "c" fn signaltstack(ss: ?*stack_t, old_ss: ?*stack_t) c_int;

(the other ones too)

Wait what? It's signal alternate stack also in FreeBSD OpenBSD and Linux

Ah, my mistake. I saw /home/vsts/work/1/s/std/os.zig:2458:25: error: container 'c' has no member called 'sigaltstack' and made an incorrect assumption.

So this looks like a bug with use then? damn

So this looks like a bug with use then? damn

ah right of course. it was just the same darwin bug on all 3 of the CI because they each test cross compilation.

So this looks like a bug with use then? damn

Nope, I just forgot to add the declaration in darwin.zig

well I feel silly. sorry for all the noise

andrewrk · 2019-05-29T14:56:03Z

std/os.zig

+
+pub fn sigaltstack(ss: ?*stack_t, old_ss: ?*stack_t) SigaltstackError!void {
+    if (windows.is_the_target or uefi.is_the_target or wasi.is_the_target)
+        @compileError("std.os.sigaltstack not available for this target");


this if and explicit compile error is just fine, but not required. everywhere else just lets the natural compile error happen if you call the function on an unsupported target.

Explicit is better than implicit, I believe that goes for error messages too.

andrewrk · 2019-05-29T14:59:11Z

std/os/test.zig

@@ -149,3 +149,14 @@ test "realpath" {
    var buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
    testing.expectError(error.FileNotFound, fs.realpath("definitely_bogus_does_not_exist1234", &buf));
 }
+
+test "sigaltstack" {


~~it's misspelled here too and below~~ oops. my mistake

andrewrk · 2019-05-29T15:00:23Z

std/os/test.zig

@@ -149,3 +149,14 @@ test "realpath" {
    var buf: [std.fs.MAX_PATH_BYTES]u8 = undefined;
    testing.expectError(error.FileNotFound, fs.realpath("definitely_bogus_does_not_exist1234", &buf));
 }
+
+test "sigaltstack" {
+    if (builtin.os != .windows and builtin.os != .uefi and builtin.os != .wasi) {


this is fine. note you can also do this:

if (builtin.os == .windows or builtin.os == .uefi or builtin.os == .wasi) return error.SkipZigTest;

also this test isn't run for uefi so you don't need that check.

Oh, that looks better.

andrewrk

Ready to merge as soon as the tests pass

daurnimator reviewed May 24, 2019

View reviewed changes

LemonBoy force-pushed the sigaltstack branch from 934a42e to e7380c1 Compare May 24, 2019 11:51

Add sigaltstack syscall

bcdbd8d

LemonBoy force-pushed the sigaltstack branch from e7380c1 to bcdbd8d Compare May 28, 2019 13:22

andrewrk requested changes May 28, 2019

View reviewed changes

andrewrk reviewed May 29, 2019

View reviewed changes

LemonBoy added 2 commits May 29, 2019 22:38

Add sigaltstack wrapper in os.zig

399e026

Add missing sigaltstack definition for darwin

b8d1060

LemonBoy force-pushed the sigaltstack branch from f95d674 to b8d1060 Compare May 29, 2019 20:38

andrewrk approved these changes May 29, 2019

View reviewed changes

andrewrk merged commit c66a747 into ziglang:master May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sigaltstack syscall for Linux #2546

Add sigaltstack syscall for Linux #2546

LemonBoy commented May 23, 2019

daurnimator May 24, 2019

daurnimator May 24, 2019

daurnimator May 24, 2019

LemonBoy May 24, 2019

daurnimator May 24, 2019

daurnimator May 24, 2019

LemonBoy May 24, 2019

daurnimator May 24, 2019 •

edited

Loading

LemonBoy May 24, 2019

daurnimator May 24, 2019

daurnimator May 24, 2019

LemonBoy May 24, 2019

daurnimator May 24, 2019

LemonBoy May 24, 2019

andrewrk left a comment

andrewrk May 28, 2019

LemonBoy May 28, 2019

andrewrk May 28, 2019

andrewrk May 28, 2019 •

edited

Loading

LemonBoy May 28, 2019

andrewrk May 28, 2019

andrewrk left a comment

andrewrk May 29, 2019

LemonBoy May 29, 2019

andrewrk May 29, 2019 •

edited

Loading

andrewrk May 29, 2019

LemonBoy May 29, 2019

andrewrk May 29, 2019

andrewrk May 29, 2019

LemonBoy May 29, 2019

andrewrk May 29, 2019 •

edited

Loading

andrewrk May 29, 2019

LemonBoy May 29, 2019

andrewrk left a comment

	pub extern "c" fn sigaltstack(ss: ?stack_t, old_ss: ?stack_t) c_int;
	pub extern "c" fn signaltstack(ss: ?stack_t, old_ss: ?stack_t) c_int;

Add sigaltstack syscall for Linux #2546

Add sigaltstack syscall for Linux #2546

Conversation

LemonBoy commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daurnimator May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk May 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewrk left a comment

Choose a reason for hiding this comment

daurnimator May 24, 2019 •

edited

Loading

andrewrk May 28, 2019 •

edited

Loading

andrewrk May 29, 2019 •

edited

Loading

andrewrk May 29, 2019 •

edited

Loading