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

Require T: NoUninit only for compare_exchange operation? #40

Open
hehaoqian opened this issue Nov 20, 2024 · 10 comments
Open

Require T: NoUninit only for compare_exchange operation? #40

hehaoqian opened this issue Nov 20, 2024 · 10 comments

Comments

@hehaoqian
Copy link

hehaoqian commented Nov 20, 2024

Require T: NoUninit is too strict in many cases,
as many types from 3rdparty crates does not implement this trait,
significantly limits the usablility of ths crate.

I don't think get_mut, into_inner, load, store, swap needs T: NoUninit,
T: Copy should be just enough.

@hehaoqian
Copy link
Author

For issues that fixed by v0.6 which introduces T: NoUninit

#36: compare_exchange issue
#35: Is this a real bug?
#23: Also compare_exhange issue

@hehaoqian
Copy link
Author

get_mut, into_inner do not need T: Copy either

@hehaoqian
Copy link
Author

hehaoqian commented Nov 20, 2024

Also, is the NoUninit from bytemuck is a proper choice?
T: NoUninit requires T: 'static,
but I don't think 'static is needed

I think compare_exchange operation for Atomic<&'a T> where 'a not 'static,
should be sound

@hehaoqian
Copy link
Author

Still, this is not easy to use.
One option is to introduce unsafe version of compare_exchange,
where the user needs to ensure there is no internal padding.

The other option is to just fully remove the T: NoUninit,
and other require T: Copy for compare_exchange.

I don't think failure of compare_exchange,
is any of undefined bahavior defined in
https://doc.rust-lang.org/reference/behavior-considered-undefined.html

@Amanieu
Copy link
Owner

Amanieu commented Dec 4, 2024

NoUninit is needed because we are transmuting values to integers before using them with atomics, and it's undefined behavior for there to be any uninitialized bytes in the resulting integer.

@hehaoqian
Copy link
Author

At least, inner_ptr, get_mut , into_inner do not require T: NoUninit, because no conversion to int occurs

@hehaoqian
Copy link
Author

I still don't think load, store, swap needs T: NoUninit,
T: Copy should be just enough.

@hehaoqian
Copy link
Author

For compare_exchange,
An unsafe version of compare_exchange_unsafe could be introduced,
that does not require T: NoUninit

@hehaoqian
Copy link
Author

Alternatively, remove all T: NoUninit requirement, but change Atomic::new() to unsafe function,
and add a safe version of Atomic::new() that requires T: NoUninit

@Amanieu
Copy link
Owner

Amanieu commented Dec 24, 2024

We could remove the T: NoUninit for methods that don't involve atomic operations, but then the type won't be very useful. What exactly are you trying to achieve with this?

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

No branches or pull requests

2 participants