Odin icon indicating copy to clipboard operation
Odin copied to clipboard

More os2

Open jasonKercher opened this issue 2 years ago • 12 comments

This pull request puts a dent into os2 for Linux:

  • env
  • pipe
  • errors
  • process

For process, I changed the API based on my best guess of the original intention.

For process_signal, I changed the signature to not take a ^Process argument. My guess was this was meant to act like the signal function that can be found on Linux and Windows where it installs signal handlers. Neither of these platforms (as far as I am aware), can install signal handlers to other processes. I defined a common set of signals and signal handlers that I believe both systems support. Also, I changed out the Process_Error for regular os2.Error.

Old:

Signal :: #type proc()

Kill:      Signal = nil 
Interrupt: Signal = nil 

process_signal :: proc(p: ^Process, sig: Signal) -> Process_Error {
        return .None
}

New:

Signal :: enum {
        Abort,
        Floating_Point_Exception,
        Illegal_Instruction,
        Interrupt,
        Segmentation_Fault,
        Termination,
}

Signal_Handler_Proc :: #type proc "c" (c.int)
Signal_Handler_Special :: enum {
        Default,
        Ignore,
}
 
Signal_Handler :: union {
        Signal_Handler_Proc,
        Signal_Handler_Special,
}

process_signal :: proc(sig: Signal, h: Signal_Handler) -> Error {
        return _process_signal(sig, h)
}

I added a process_get_state proc:

process_get_state :: proc(p: Process) -> (Process_State, Error) {
        return _process_get_state(p)
}

I also added a Duration to process_wait as it seems like a common ability between different systems.

I changed the Process_Attributes struct to contain explicit pointers to stdin, stdout and stderr instead of []^File. These get populated if they are passed to process_start as non-nil.

Process_Attributes :: struct {
        dir: string,
        env: []string,
        stdin: ^File,
        stdout: ^File,
        stderr: ^File,
        sys: ^Process_Attributes_OS_Specific,
}

As far as it goes, I'm not married to any of the API decisions I made. For example, to open a child process with pipes to stdout and stdin, the code looks like this:

child_stdout: os2.File
child_stdin: os2.File
attr: os2.Process_Attributes = {
        stdin = &child_stdin,
        stdout = &child_stdout,
}
os2.process_start("cat", nil, &attr)

I don't know.. it feels clunky to me.

jasonKercher avatar Oct 14 '23 03:10 jasonKercher

Just noticed some issues concerning the Stat struct. The definition of the struct is platform-dependent, meaning this won't necessarily compile on x86-32 and arm linucees.

I'll link some definitions of these structs from the linux kernel. You can find them in the following files (relative to repository root):

  • x86: /arch/x86/include/uapi/asm/stat.h
  • arm32: /arch/arm/include/uapi/asm/stat.h
  • arm64: /include/uapi/asm-generic

Quick explanation of the structure: uapi means user api, the files in the arch-specific folders are bindings that old architectures used, before linux decided to make the definition of stat work similar on the different architectures. x86-64, x86-32 and arm32 use the old way, whereas arm64 uses the new way.

I think we only care about struct stat and struct stat64, struct old_stat is pretty much obsolete.

Some noteable observations about this struct definition's differences:

  • x86-64 and x86-32 have the order of st_nlink and st_mode fields flipped
  • The new stat deprecates register-sized inode and puts the new inode at the end of the struct

I'm currently working on binding more syscalls in the sys package, potentially adding Odin's types, so that it's a bit easier to do cross-arch stuff, but I'll provide the definition of Stat that I have

when ARCH == "amd64" {
    Stat :: struct {
        st_dev:        c_ulong,
        st_ino:        c_ulong,
        st_nlink:      c_ulong,
        st_mode:       c_uint,
        st_uid:        c_uint, // 32-bit uid
        st_gid:        c_uint, // 32-bit uid
        _:             u32,
        st_rdev:       c_ulong,
        st_size:       c_ulong,
        st_blksize:    c_ulong,
        st_blocks:     c_ulong,
        st_atim:       c_timespec,
        st_mtim:       c_timespec,
        st_ctim:       c_timespec,
        _:             [3]c_ulong,
    }
} else {
    Stat :: struct {
        st_dev:        c_ulong,
        st_ino:        c_ulong,
        st_mode:       c_ushort,
        st_nlink:      c_ushort,
        st_uid:        c_ushort, // 16-bit gid
        st_gid:        c_ushort, // 16-bit uid
        st_rdev:       c_ulong,
        st_size:       c_ulong,
        st_blksize:    c_ulong,
        st_blocks:     c_ulong,
        st_atim:       c_timespec,
        st_mtim:       c_timespec,
        st_ctim:       c_timespec,
        _:             c_ulong,
        _:             c_ulong,
    }
}

This doesn't have the ARM bindings yet (because I'm sure mine are incorrect), but that should at least point out the problem with stat.

flysand7 avatar Oct 14 '23 12:10 flysand7

@flysand7 Thanks dude. I just copied the stat structure out of core/os/os_linux.odin. So this change should probably be back-ported. Looks like I jumped

jasonKercher avatar Oct 14 '23 14:10 jasonKercher

Haha. I fat fingered the close button while typing in that reply. Looks like I jumped the gun not making CI pass first anyway.

I'm currently working on binding more syscalls in the sys package, potentially adding Odin's types, so that it's a bit easier to do cross-arch stuff

Definitely looking forward to this!

jasonKercher avatar Oct 14 '23 14:10 jasonKercher

Please don't merge yet. I want to go the buildah/podman route and get everything working across architectures. Should I just close?

jasonKercher avatar Oct 15 '23 05:10 jasonKercher

image

In the top-right corner of the page there's a button that makes PR a draft which prevents it from being merged prematurely. I think you can undraft once you're finished

flysand7 avatar Oct 15 '23 05:10 flysand7

Any updates?

flysand7 avatar Oct 18 '23 08:10 flysand7

Any updates?

=] I don't have as much free time as you it would seem (full-time job + 4 kids). Note how the first commit on this was in May =] I'm sure you are right about the stat struct, but I want to make sure there are not similar issues with other structs. I'm going to write a bash script to run my tests across all architectures automatically using buildah/podman. I have a private repo for it that I will make public when that part works.

jasonKercher avatar Oct 18 '23 13:10 jasonKercher

Ooh that's interesting! I didn't know about buildah/podman, so thanks for name dropping. Something that executes tests in a VM sounds like it could also be useful for my current work with syscalls, as I'm not sure about whether I've done a correct job on arch-dependent stuff in ARM/x86-32

flysand7 avatar Oct 19 '23 03:10 flysand7

Something I've realised is that there's a difference between handle inheritance between linux and windows that should get addressed. When you create a child process some handles may or may not be inherited by that process. On linux that is controlled by O_CLOEXEC flag. On windows that is controlled by bInheritHandles value in the security attributes struct.

The difference lies in the default behavior (source). On linux by default (unless the flag is specified) the handle is inherited. On windows it is not inherited. I think the easiest thing to do here is to use the windows behavior as Odin's default, at least for pipes. Could you pass O_CLOEXEC flag when creating linux pipes, or at least set both ends of the pipe to close on exec?

flysand7 avatar Mar 26 '24 00:03 flysand7

@flysand7 Will do! I just changed some things in sys/linux. I wanted to ask you about your Sig_Set definition. I thought sigset_t was a u64 as it's just a bit set of the signals. This is going to be 16 uints?

@private _SIGSET_NWORDS :: (1024 / (8 * size_of(uint)))
Sig_Set :: [_SIGSET_NWORDS]uint

jasonKercher avatar Apr 06 '24 12:04 jasonKercher

@flysand7 Will do! I just changed some things in sys/linux. I wanted to ask you about your Sig_Set definition. I thought sigset_t was a u64 as it's just a bit set of the signals. This is going to be 16 uints?

I'm not totally sure about it but it should probably be verified with a C compiler, but when I was making this I simply copied the typedefs from libc. It certaintly is not incorrect, but it might be wasteful to allocate that much space.

flysand7 avatar Apr 14 '24 22:04 flysand7

Should be good to go. This passes these tests on amd64 and arm64 with the aid of Puklaj's fork fix (#3476 ).

I made a local CI-like runner bash script thing to emulate other architectures in case anyone wants to do that on their own machine.

jasonKercher avatar May 04 '24 05:05 jasonKercher