rust-random / getrandom

A small cross-platform library for retrieving random data from (operating) system source
Apache License 2.0
275 stars 180 forks source link

use_file: `std::sync::Mutex`, dropping all libpthread use. #457

Closed briansmith closed 3 months ago

briansmith commented 3 months ago

use_file: std::sync::Mutex, dropping all libpthread use.

pthreads mutexes are not safe to move. While it is very unlikely that the mutex we create will ever be moved, we don't actively do anything to actively prevent it from being moved. (libstd, when it used/uses pthreads mutexes, would box them to prevent them from being moved.)

Also, now on Linux and Android (and many other targets for which we don't use use_std), libstd uses futexes instead of pthreads mutexes. Thus using libstd's Mutex will be more efficient and avoid adding an often-otherwise-unnecessary libpthreads dependency on these targets.

This will not affect our plans for --linux-none, since we don't plan to use use_file for it. OnceLock

This breaks 32-bit x86 QNX Neutrino, which doesn't have libstd because the target itself is abandoned [3]. the other QNX Neutrino targets didn't get libstd support until Rust 1.69, so this effectively raises the MSRV for them to 1.69.

Otherwise, the MSRV increases to 1.63 for the above-mentioned targets, as that's when Mutex::new() became a const fn.

I tried to use Once to avoid the MSRV increase but it doesn't support fallible initialization even in Nightly. OnceLock wasn't added until 1.70.

On x86_64 Linux, this change removes all libpthreads dependencies:

-       pthread_mutex_lock
-       pthread_mutex_unlock

and adds these libstd dependencies:

+       std::panicking::panic_count::GLOBAL_PANIC_COUNT
+       std::panicking::panic_count::is_zero_slow_path
+       std::sys::sync::mutex::futex::Mutex::lock_contended
+       std::sys::sync::mutex::futex::Mutex::wake

as measured using cargo asm.

[1] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L4-L10 [2] https://github.com/rust-lang/rust/blob/c1dba09f263cbff6170f130aa418e28bdf22bd96/library/std/src/sys/sync/mutex/mod.rs#L17-L20 [3] https://github.com/rust-random/getrandom/issues/453#issuecomment-2148124364

josephlr commented 3 months ago

If we are adding a std dependency for these targets, should we also move to using std::File so that we don't even need DropGuard anymore? Also, should use of std require enabling the "std" cargo feature?

briansmith commented 3 months ago

If we are adding a std dependency for these targets, should we also move to using std::File so that we don't even need DropGuard anymore?

Unless/until we can use std::syc::OnceLock or a polyfill for it, it doesn't make much sense right not to use std::File, because we'd end up using std::File::open() and then immediately calling into_raw_fd().

Also, should use of std require enabling the "std" cargo feature?

No. Note that this PR effectively only uses libstd on targets for which we know libstd is already available.

briansmith commented 3 months ago

Unless/until we can use std::syc::OnceLock or a polyfill for it, it doesn't make much sense right not to use std::File, because we'd end up using std::File::open() and then immediately calling into_raw_fd().

Well, actually, it is possible to use std::File in wait_until_rng_ready now.

briansmith commented 3 months ago

In https://github.com/rust-random/getrandom/pull/458, instead of moving wait_until_rng_ready, I took @josephlr suggestion and replaced DropGuard with std::fs::File.

briansmith commented 3 months ago

Closing this in favor of #458 based on @newpavlov's comment in the issue.