Skip to content
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

Add rustix backend #470

Closed
wants to merge 3 commits into from
Closed

Add rustix backend #470

wants to merge 3 commits into from

Conversation

notgull
Copy link

@notgull notgull commented Jun 8, 2024

This commit adds a backend based on the rustix crate. The main advantage
of rustix, in addition to greater safety, is that it uses raw syscalls
instead of going through libc.

I've tried to modify the existing libc code as little as possible, in
order to make this change as auditable as possible. I haven't touched
the pthreads code yet, as that's a little tricky.

This exists for discussion purposes.

cc #401

This commit adds a backend based on the rustix crate. The main advantage
of rustix, in addition to greater safety, is that it uses raw syscalls
instead of going through libc.

I've tried to modify the existing libc code as little as possible, in
order to make this change as auditable as possible. I haven't touched
the pthreads code yet, as that's a little tricky.

This exists for discussion purposes.

cc rust-random#401

Signed-off-by: John Nunley <[email protected]>
src/use_file.rs Outdated
}
}
}

struct Mutex(UnsafeCell<libc::pthread_mutex_t>);
Copy link
Contributor

Choose a reason for hiding this comment

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

The most important improvement rustix could make to this code would be replacing the pthreads_mutex_t here with whatever is used by libstd on the target (e.g. futex on Linux/Android).

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I've replaced Mutex with one based on futexes, only on Rustix.

libc still uses pthreads, but on rustix we use a homegrown futex implementation
based on the rustix-futex-sync crate.

Signed-off-by: John Nunley <[email protected]>
[target.'cfg(any(target_os = "linux", target_os = "android"))'.dependencies]
rustix = { version = "0.38.0", features = ["event", "fs", "rand", "thread"], default-features = false, optional = true }
[target.'cfg(target_os = "linux")'.dependencies]
rustix = { version = "0.38.0", features = ["event", "fs", "rand", "thread"], default-features = false }
Copy link
Contributor

Choose a reason for hiding this comment

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

We should only need event, fs, and thread if we're going to use the file-based fallback, but unfortunately that's very hard to specify in Cargo.toml, as you can see by the actual cfg logic in lib.rs.

I guess there must be a reason why Rustix makes those things optional features. (Why?) Would people who don't want those features activated care that getrandom would be activating them?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so, the features are mostly to reduce code size.

@newpavlov
Copy link
Member

This PR is outdated, so I will close it. We probably will revisit the rustix question while adding support for none Linux targets.

@newpavlov newpavlov closed this Jun 21, 2024
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