Are `rwlock` and `spin_mutex` compatible with `portable_atomic`? about spin-rs HOT 11 CLOSED

For future reference: the reason for this added complexity is that enabling portable_atomic support is unsound on multi-core platforms (and arguably unsound outright in all cases, depending on the aliasing semantics that Rust finally settles on) so they want users to be really sure of what they're doing before enabling the feature.

That said, the error message here could definitely be improved. I'll have a look into this.

from spin-rs.

You need to enable cfg or the critical-section feature of the portable-atomic as described in the documentation.

https://github.com/mvdnes/spin-rs#feature-flags

portable_atomic enables usage of the portable-atomic crate
to support platforms without native atomic operations (Cortex-M0, etc.).
The portable_atomic_unsafe_assume_single_core cfg or critical-section feature
of portable-atomic crate must also be set by the final binary crate.

When using the cfg, this can be done by adapting the following snippet to the .cargo/config file:

 [target.<target>]
 rustflags = [ "--cfg", "portable_atomic_unsafe_assume_single_core" ]

Note that this cfg is unsafe by nature, and enabling it for multicore systems is unsound.

When using the critical-section feature, you need to implement the critical-section
implementation that sound for your system by implementing an unsafe trait.
See the documentation for the portable-atomic crate
for more information.

from spin-rs.

Okay thanks that fixed it!

from spin-rs.

Thanks for that additional context, definitely makes sense you'd want to have users explicitly do an unsafe behavior. 👍

from spin-rs.

enabling portable_atomic support is unsound on multi-core platforms

To be clear: This relates to the single-core cfg of portable_atomic, and it is possible to make it sound on multi-core systems by using the critical-section feature of portable_atomic. However, when using the critical-section feature, the appropriate critical-section implementation must be selected for the system, and there is no single way that will work correctly on all systems. Also, single-core cfg is more efficient in contexts where single-core cfg is sound.

and arguably unsound outright in all cases, depending on the aliasing semantics that Rust finally settles on

@zesterer Could you elaborate on this? I'm not aware of such a problem.

from spin-rs.

@zesterer Could you elaborate on this? I'm not aware of such a problem.

I don't believe Rust currently defines what 'parallelism' and 'thread safety' mean to a sufficient degree to conclusively rule out unsafety on single-core systems, since 'single-core' isn't a thing that has a specific relationship with Rust's compilation model, that I'm aware of.

I think, in practice, it's probably never going to result in unexpected behaviour unless you're doing strange things like accessing spinlocks in interrupt handlers and the like, but it's still worth considering that your system superficially not possessing multiple threads of execution doesn't guarantee safety.

from spin-rs.

In this commit I made some changes that result in the following error when using portable_atomic without the relevant cfg flag.

$ cargo build --no-default-features --features rwlock,portable_atomic --target thumbv6m-none-eabi
   Compiling spin v0.9.8 (/home/joshua/other/spin-rs)
error: The feature "portable_atomic" requires the "portable_atomic_unsafe_assume_single_core" cfg flag to be enabled. See https://docs.rs/portable-atomic/latest/portable_atomic/#optional-cfg.
  --> src/lib.rs:75:1
   |
75 | core::compile_error!("The feature \"portable_atomic\" requires the \"portable_atomic_unsafe_assume_single_core\" cfg flag to be enabled. See https://docs.rs/portable-atomic/latest/portable_atomic/#optional-cfg....
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: could not compile `spin` due to previous error

from spin-rs.

@zesterer

I don't believe Rust currently defines what 'parallelism' and 'thread safety' mean to a sufficient degree to conclusively rule out unsafety on single-core systems, since 'single-core' isn't a thing that has a specific relationship with Rust's compilation model, that I'm aware of.

I think, in practice, it's probably never going to result in unexpected behaviour unless you're doing strange things like accessing spinlocks in interrupt handlers and the like, but it's still worth considering that your system superficially not possessing multiple threads of execution doesn't guarantee safety.

portable_atomic's single-core cfg is implemented by disabling interrupts on all platforms where cfg is supported.
As docs says:

When this cfg is enabled, this crate provides atomic CAS for targets where atomic CAS is not available in the standard library by disabling interrupts.

On these platforms, if they are single-core and interrupts are disabled, there is no way to break atomicity except reordering memory accesses by the compiler (well, the compiler actually shouldn't be able to reorder memory accesses in such a way as to affect the results, though). And the inline assembly used to disable and restore the interrupt state implies a compiler fence.

So, the problem you mention does not exist in portable_atomic.

In this commit I made some changes that result in the following error when using portable_atomic without the relevant cfg flag.

Hmm, I'm concerned that this will prevent the use of the critical-section feature of the portable_atomic.

Instead, I think it would be better to add something like require-cas feature to the portable_atomic and make it a compile error on portable_atomic side if neither the cfg nor the feature is not set.

from spin-rs.

Instead, I think it would be better to add something like require-cas feature to the portable_atomic and make it a compile error on portable_atomic side if neither the cfg nor the feature is not set.

Filed taiki-e/portable-atomic#100 to implement this approach.

from spin-rs.

Filed taiki-e/portable-atomic#100 to implement this approach.

Would you suggest that we revert the change to spin-rs given that this now exists?

from spin-rs.

Yes, I would suggest reverting it and enabling portable-atomic's require-cas feature.

I opened #152 to do these.

from spin-rs.