Are rwlock and spin_mutex compatible with portable_atomic?

[coreylowman]

Hello,

I'm trying to build a no_std crate for --target thumbv6m-none-eabi and running into some issues with rwlock and spin mutex. The error messages are below.

Here's spin in my Cargo.toml:

spin = { version = "0.9.8", default-features = false, features = ["spin_mutex", "rwlock", "portable_atomic"], optional = true }
num-traits = { version = "0.2.15", default-features = false, features = ["libm"] }

Here's my full cargo tree:

dfdx v0.11.1 (C:\Users\clowm\Documents\programming\dfdx)
├── no-std-compat v0.4.1
│   └── hashbrown v0.8.2
│       └── ahash v0.3.8
│       [build-dependencies]
│       └── autocfg v1.0.1
├── num-traits v0.2.15
│   └── libm v0.2.1
│   [build-dependencies]
│   └── autocfg v1.0.1
├── rand v0.8.5
│   ├── rand_chacha v0.3.1
│   │   ├── ppv-lite86 v0.2.10
│   │   └── rand_core v0.6.3
│   └── rand_core v0.6.3
├── rand_distr v0.4.3
│   ├── num-traits v0.2.15 (*)
│   └── rand v0.8.5 (*)
└── spin v0.9.8
    └── portable-atomic v1.2.0

And here are the error messages:

error[E0599]: no method named `compare_exchange_weak` found for struct `portable_atomic::AtomicBool` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\mutex\spin.rs:182:14
    |
182 |             .compare_exchange_weak(false, true, Ordering::Acquire, Ordering::Relaxed)
    |              ^^^^^^^^^^^^^^^^^^^^^ method not found in `portable_atomic::AtomicBool`

error[E0599]: no method named `compare_exchange` found for struct `portable_atomic::AtomicBool` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\mutex\spin.rs:242:14
    |
242 |             .compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed)
    |              ^^^^^^^^^^^^^^^^ method not found in `portable_atomic::AtomicBool`

error[E0599]: no method named `fetch_add` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:265:31
    |
265 |         let value = self.lock.fetch_add(READER, Ordering::Acquire);
    |                               ^^^^^^^^^ method not found in `portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_sub` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:268:23
    |
268 |             self.lock.fetch_sub(READER, Ordering::Relaxed);
    |                       ^^^^^^^^^ method not found in `portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_sub` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:305:23
    |
305 |             self.lock.fetch_sub(READER, Ordering::Release);
    |                       ^^^^^^^^^ method not found in `portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_sub` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:349:19
    |
349 |         self.lock.fetch_sub(READER, Ordering::Release);
    |                   ^^^^^^^^^ method not found in `portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_and` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:363:19
    |
363 |         self.lock.fetch_and(!(WRITER | UPGRADED), Ordering::Release);
    |                   ^^^^^^^^^ method not found in `portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_or` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:415:22
    |
415 |         if self.lock.fetch_or(UPGRADED, Ordering::Acquire) & (WRITER | UPGRADED) == 0 {
    |                      ^^^^^^^^ method not found in `portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_sub` found for reference `&'rwlock portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:760:19
    |
760 |         self.lock.fetch_sub(READER, Ordering::Release);
    |                   ^^^^^^^^^ method not found in `&'rwlock portable_atomic::AtomicUsize`

error[E0599]: no method named `fetch_sub` found for struct `portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:796:16
    |
796 |         atomic.compare_exchange(current, new, success, failure)
    |                ^^^^^^^^^^^^^^^^ method not found in `&portable_atomic::AtomicUsize`

error[E0599]: no method named `compare_exchange_weak` found for reference `&portable_atomic::AtomicUsize` in the current scope
   --> C:\Users\clowm\.cargo\registry\src\github.com-1ecc6299db9ec823\spin-0.9.8\src\rwlock.rs:798:16
    |
798 |         atomic.compare_exchange_weak(current, new, success, failure)
    |                ^^^^^^^^^^^^^^^^^^^^^ method not found in `&portable_atomic::AtomicUsize`

[taiki-e]

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.

[coreylowman]

Okay thanks that fixed it!

[zesterer]

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.

[coreylowman]

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

[taiki-e]

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.

[zesterer]

@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.

[zesterer]

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

[taiki-e]

@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.

[taiki-e]

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.

[zesterer]

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?

[taiki-e]

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

I opened #152 to do these.