Skip to content

std: move os/darwin.zig and related to c/darwin.zig #14921

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

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

kubkon
Copy link
Member

@kubkon kubkon commented Mar 15, 2023

Move to c/darwin.zig as they really are libSystem/libc imports/wrappers.

As an added bonus, get rid of the nasty usingnamespaces which are now unneeded.

Finally, add os.ptrace but currently only implemented on darwin.

@kubkon kubkon changed the title std: remove os/darwin.zig and related std: move os/darwin.zig and related to c/darwin.zig Mar 15, 2023
Move to c/darwin.zig as they really are libSystem/libc imports/wrappers.

As an added bonus, get rid of the nasty `usingnamespace`s which are now
unneeded.

Finally, add `os.ptrace` but currently only implemented on darwin.
@kubkon kubkon force-pushed the darwin-c-api-cleanup branch from 4ca57d7 to 52c15e4 Compare March 15, 2023 10:05
@mllken
Copy link
Contributor

mllken commented Mar 15, 2023

Hey, just a note in case it would change your definition for os.ptrace - On Linux, it seems the fourth argument to ptrace is a pointer-sized type, as it can be used as an address or a bitmask (my attempt was here: #14839)

@kubkon
Copy link
Member Author

kubkon commented Mar 15, 2023

Hey, just a note in case it would change your definition for os.ptrace - On Linux, it seems the fourth argument to ptrace is a pointer-sized type, as it can be used as an address or a bitmask (my attempt was here: #14839)

I'll let @andrewrk decide how to go about doing this. This PR doesn't conflict with his addition of ptrace for Linux which is exposed only in os/linux.zig so we can could merge this one as-is and then roll out another PR after Andrew merges his to fix the signature ptrace.

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.

I like the idea of moving "must link to c" to a dir with name c. However, mixing error abstractions with mere bindings feels inconsistent to os/windows.zig, which only has error abstractions and data.

If we go that route of mixing bindings + error abstractions, then I would favor keeping the error abstraction next to the binding or file reference to the binding unless we abstract over multiple OSes.

Any thoughts? I'm also asking to have a clue how I should (re)structure my PR #14726.

@@ -3085,3 +3094,471 @@ pub const PT_DENY_ATTACH = 31;
pub const caddr_t = ?[*]u8;

pub extern "c" fn ptrace(request: c_int, pid: pid_t, addr: caddr_t, data: c_int) c_int;

Copy link
Contributor

@matu3ba matu3ba Mar 15, 2023

Choose a reason for hiding this comment

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

Would be nice to add an indication that "error abstracted functions", so that "mere bindings" are separated from "error abstractions". Maybe something like the following for each section
// ======= Bindings =======
// ======= Error abstractions =======?
Or keep things next to another.

Copy link
Member Author

@kubkon kubkon Mar 15, 2023

Choose a reason for hiding this comment

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

I am not sure I follow. Why would we want any wrapper for ptrace here and not directly in os.zig?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am happy with adding separating comments but I think it's somewhat premature - I don't even yet know how to structure this file, and currently it's a bit of a mess. The reason I created this PR was revert an erroneous decision by me of introducing os/darwin.zig which seems backwards as everything on macOS is done via libSystem anyhow. Also, this way we avoid unnecessary and pointless usingnamespaces.

Copy link
Contributor

@matu3ba matu3ba Mar 15, 2023

Choose a reason for hiding this comment

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

Why would we want any wrapper for ptrace here and not directly in os.zig?

Since the function name is identical (+ we have no posix.zig yet to distinguish unifiable and non-unifiable posix semantics), definitely yes. If the function name is not identical or it would offer semantics not available on other OSes, then this would sound less good.

I don't even yet know how to structure this file, and currently it's a bit of a mess

Ah, kk. (This is resolved)

@kubkon
Copy link
Member Author

kubkon commented Mar 15, 2023

I like the idea of moving "must link to c" to a dir with name c. However, mixing error abstractions with mere bindings feels inconsistent to os/windows.zig, which only has error abstractions and data.

If we go that route of mixing bindings + error abstractions, then I would favor keeping the error abstraction next to the binding or file reference to the binding unless we abstract over multiple OSes.

Any thoughts? I'm also asking to have a clue how I should (re)structure my PR #14726.

I think Windows is a little special as there are many layers the user can opt into: use kernel32 or use only ntdll. For Windows we also want to implement kernel32 wrappers ourselves where possible, while in case of macOS, os.zig is the closest to it where we would only implement things like posix_spawnp or execveZ_expandArg0 or whatnot. Additionally, note that macOS bindings mirror what we do for BSDs already, and all Darwin-specific interfaces are still available via std.os.darwin. Finally, I don't see any point in moving the error bindings into a different file, at least not yet - these bindings are incredibly specific to macOS as they are used to interface with the Mach kernel from the user-space. What this PR offers works pretty well already - have a look at changes in https://github.com/kubkon/zdb/compare/start-with-fork

@kubkon
Copy link
Member Author

kubkon commented Mar 16, 2023

Andrew has merged his parallel builds branch adding more goodies including raw ptrace syscall wrapper on Linux, so here's my plan. I will merge this as-is, and then while adding HCS PoC for macOS, I will try to add a draft os.ptrace wrapper that works on both linux and darwin.

@kubkon kubkon merged commit e1e414e into master Mar 16, 2023
@kubkon kubkon deleted the darwin-c-api-cleanup branch March 16, 2023 10:42
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.

3 participants