Request: Add support for atomic-polyfill about spin-rs HOT 7 CLOSED

I would find it sufficient for this crate to state that enabling the atomic_polyfill feature when depending on spin in a library crate is very dangerous and should not be done. You can trigger cargo to merge the feature together for an underlying library crate by including spin in the toplevel crate with the atomic_polyfill feature.

Detecting the target to be multicore in Rust code is also impossible when you have heterogeneous cores that depend on cargo-microamp to be built.

Making Mutex / RwLock generic over the type of atomics seems unnecessarily complex to me. It would require all depending crates to expose it in their public API, always. This still requires crate authors to be disciplined in this regard.

from spin-rs.

Perhaps there is another, more sound solution, created by @taiki-e:
https://crates.io/crates/portable-atomic

This crate includes the following cfg flag:

--cfg portable_atomic_unsafe_assume_single_core
Assume that the target is single-core. When this cfg is enabled, this crate provides atomic CAS for targets where atomic CAS is not available in the standard library.

This ensures that only the last link in the dependency chain has control and unintentional activation should be impossible.

from spin-rs.

While I theoretically want to implement this (I've worked with Rust on the GBA in the past) I'm a little worried about the possible soundness implications. From the atomic-polyfill README:

Note: polyfill is based on critical sections using the critical-section crate. The default implementation is based on disabling all interrupts, so it's unsound on multi-core targets. It is possible to supply a custom critical section implementation, check the critical-section docs for details.

This seems to imply that enabling such a feature on a multi-core platform might accidentally result in spin becoming unsound. I'd ordinarily be fine with this: it's something the crate user opts into, so it's their fault if they don't check the target.

Except when they don't opt into it. Cargo's dependency resolver unifies feature sets, and this may lead to a sub-dependency accidentally relying on unsound behaviour because a crate higher up in the dependency tree enables the feature.

I don't really see a way out of this, cargo's feature system just isn't designed to deal with these sort of cases, and I'm not aware of a way to force the resolver not to unify when a specific feature flag is enabled.

from spin-rs.

What do you suggest? I've been told atomic-polyfill is the go-to for implementing atomics by means of disabling interrupts on single-threaded systems, but there's no crate I can find which builds on top of it to provide Mutex and RwLock APIs.

from spin-rs.

One option might be to make Mutex/RwLock generic over the type of the lock integer, which would allow swapping between real atomic locking and polyfill locking with something like this:

#[cfg(feature = "building_for_the_gba")]
type Mutex<T> = spin::Mutex<T, DisableIrq>;
#[cfg(not(feature = "building_for_the_gba"))]
type Mutex<T> = spin::Mutex<T, Atomic>;

Obviously, this won't work if you're depending on something that requires spin (like lazy-static).

from spin-rs.

This is a good idea and seems like it would resolve my concerns! @Wassasin: if this was a route you wanted to go down, I'd be happy to accept the PR.

from spin-rs.

This has now been implemented by #120, thanks to @fralalonde !

from spin-rs.