linux_android_with_fallback: do not use dlsym on MUSL targets #602
Conversation
| #[cfg(target_env = "musl")] | ||
| let raw_ptr = { | ||
| let fptr: GetRandomFn = libc::getrandom; | ||
| unsafe { transmute::<GetRandomFn, *mut c_void>(fptr) } |
There was a problem hiding this comment.
This cast to a pointer and back smells like an indirect call.
There was a problem hiding this comment.
There is effectively no overhead to this approach. "Direct" call to libc::getradom compiles down to:
xor edx, edx
jmp qword ptr [rip + getrandom@GOTPCREL]While the "indirect" call compiles to:
mov rax, qword ptr [rip + GETRANDOM_FN]
xor edx, edx
jmp raxYes, the former is a bit friendlier to CPUs, but compared to the syscall cost the difference is negligible. If you care about this difference, then you should use the linux_getrandom opt-in backend.
|
@newpavlov just force push to remove the two spurious commits - you're going to clutter the git history this way =/ |
|
We always squash commits before merge, so local commit history in PR does not matter much. |
|
OK. If this were my project I'd ask you to back out all the unnecessary import churn and random comment changes - but it's not! |
There was a problem hiding this comment.
@newpavlov can we please merge this one way or the other to fix the breakage and cut 0.3.2? Thank you.
| @@ -17,18 +17,28 @@ type GetRandomFn = unsafe extern "C" fn(*mut c_void, libc::size_t, libc::c_uint) | |||
| /// or not supported by kernel. | |||
| const NOT_AVAILABLE: NonNull<c_void> = unsafe { NonNull::new_unchecked(usize::MAX as *mut c_void) }; | |||
|
|
|||
| static GETRANDOM_FN: AtomicPtr<c_void> = AtomicPtr::new(ptr::null_mut()); | |||
| static GETRANDOM_FN: AtomicPtr<c_void> = AtomicPtr::new(core::ptr::null_mut()); | |||
There was a problem hiding this comment.
could we avoid this noise in the diff? doesn't seem related to the fix being made.
| let res_ptr = match NonNull::new(raw_ptr) { | ||
| Some(fptr) => { | ||
| let getrandom_fn = unsafe { mem::transmute::<NonNull<c_void>, GetRandomFn>(fptr) }; | ||
| let dangling_ptr = ptr::NonNull::dangling().as_ptr(); | ||
| let getrandom_fn = unsafe { transmute::<NonNull<c_void>, GetRandomFn>(fptr) }; |
There was a problem hiding this comment.
the diff on these 2 lines is also not necessary.
| @@ -54,7 +64,7 @@ fn init() -> NonNull<c_void> { | |||
| res_ptr | |||
| } | |||
|
|
|||
| // prevent inlining of the fallback implementation | |||
| // Prevent inlining of the fallback implementation | |||
| @@ -78,7 +88,7 @@ pub fn fill_inner(dest: &mut [MaybeUninit<u8>]) -> Result<(), Error> { | |||
| use_file_fallback(dest) | |||
| } else { | |||
| // note: `transmute` is currently the only way to convert a pointer into a function reference | |||
| let getrandom_fn = unsafe { mem::transmute::<NonNull<c_void>, GetRandomFn>(fptr) }; | |||
| let getrandom_fn = unsafe { transmute::<NonNull<c_void>, GetRandomFn>(fptr) }; | |||
|
I think it's fine to keep minor code tweaks in this PR. Before releasing v0.3.2 I would like to also merge the |
Fixes #600