Add methods for atomic access to Bytes #115
Conversation
The "integer-atomics" feature can be replaced by using intrinsic `cfg` annotations (such as detecting different platforms). Also removed the implementation of `AtomicValued` for `AtomicBool` because we cannot safely reinterpret locations in guest memory as `bool` (values other than 0 and 1 may lead to undefined behaviour). Renamed `AtomicValued` to `AtomicInteger`, which seems like a more accurate description. Signed-off-by: Alexandru Agache <[email protected]>
alexandruag
force-pushed
the
atomic_access
branch
from
60fd29c to
c0fc95d
Compare
| @@ -12,7 +12,6 @@ autobenches = false | |||
|
|
|||
| [features] | |||
| default = [] | |||
| integer-atomics = [] | |||
There was a problem hiding this comment.
This seems like a breaking change, should we bump the minor version number for next release?
There was a problem hiding this comment.
Sounds good. This also reminds me I have to update the changelog.
| @@ -0,0 +1,86 @@ | |||
| use std::sync::atomic::Ordering; | |||
There was a problem hiding this comment.
Could you please help to add a license header?
src/atomic_integer.rs
Outdated
|
|
||
| /// Objects that implement this trait must consist exclusively of atomic types | ||
| /// from [`std::sync::atomic`](https://doc.rust-lang.org/std/sync/atomic/), except for | ||
| /// [`AtomicPtr<T>`](https://doc.rust-lang.org/std/sync/atomic/struct.AtomicPtr.html). |
There was a problem hiding this comment.
AtomicBool is dangerous in this context (i.e. we're essentially casting some memory from the guest to a type), because values other than 0 and 1 for a bool value may lead to undefined behaviour.
There was a problem hiding this comment.
So it may be better to refine the comments as "except for [AtomicPtr<T>] and AtomicBool".
There was a problem hiding this comment.
That's totally right. There was no mention of AtomicBool initially, but I've added except for AtomicPtr and AtomicBool ... as part of the commit which expands the AtomicInteger trait. The message from the commit that removes the AtomicInteger for AtomicBool impl also offers a bit more details abou the why.
Turn `AtomicInteger` into a trait that abstracts away atomic operations (strange that Rust does not have this already). We only include `load` and `store` for now, but others can be added later. Signed-off-by: Alexandru Agache <[email protected]>
Signed-off-by: Alexandru Agache <[email protected]>
alexandruag
force-pushed
the
atomic_access
branch
from
c0fc95d to
4cdc83f
Compare
Signed-off-by: Alexandru Agache <[email protected]>
|
Looks good to me! |
|
LGTM |
This PR adds the
loadandstoremethods to theBytesinterface, which can be used to perform atomic accesses based on a specifiedOrderingargument. It's a simplified variant of #104, which does not include the abstractions for aligned accesses.