Repository navigation
How should we expose atomic load/store on targets that don't support full atomics #99668
Description
Activity
Just wanted to add my $0.02, option two, "Remove support for atomic load/store on targets that don't support full atomics", would be a breaking change to (at least) one Tier 2 target:
thumbv6m-none-eabi.It would break a non-trivial number of embedded projects (there are lots of Cortex-M0+ cores out there, including the very popular RP2040), and might not be picked up by a crater run, as crate may not exercise specific targets during the run.
Also just as a note, we might want to check in that
armv5testill builds with these relevant changes. It has "native" load/stores, but no CAS operations. These are provided by an OS-level polyfill for the linux target.It's likely their load/stores will also be affected by any change here.
AFAIK
armv5teshould not be affected by this change, but we should double check with the upgrade to LLVM 15.Currently the core::ptr docs say:
All accesses performed by functions in this module are non-atomic in the sense of atomic operations used to synchronize between threads. This means it is undefined behavior to perform two concurrent accesses to the same location from different threads unless both accesses only read from memory. Notice that this explicitly includes read_volatile and write_volatile: Volatile accesses cannot be used for inter-thread synchronization.
One downside of requiring users on these platforms to use volatile ops instead is that (in Rust) volatile is a property of an access (so a static UnsafeCell/static mut could still be accessed with a non-volatile operation) while the Atomic types only have atomic operations (via AtomicU8, AtomicBool, etc), and I don't think there's any appetite for
VolatileU32or similar.There is the atomic-polyfill crate which is quite popular in embedded Rust and provides CAS operations on thumbv6 etc while directly forwarding
core::sync::atomicon other platforms. If option 2 was chosen, probably this crate could help smooth out a transition.Reacted by Ralf Jungthe atomic-polyfill crate
That is unsound on multi-core systems. (See tokio-rs/bytes#461 (comment) for an approach to handle such cases soundly.)
That is unsound on multi-core systems.
I thought it used the critical-section crate, which requires users to provide a critical section implementation for their system - so on a multi-core system you need to provide something with the appropriate guarantees, and it won't compile if no implementation is provided. It doesn't simply disable all interrupts on the CM0 core to obtain the CAS locks.
Edit: The currently-released critical-section 0.2.7 does by default provide a "disable all interrupts" critical section for thumbv* platforms, which is unsound on multi-core, though users on multi-core systems can still provide their own implementation which is sound. The next release of critical-section removes all the built-in implementations entirely, and it's expected most platform support crates will provide an implementation instead, which should be sound for their respective platform. In any event I don't think it gets in the way of having atomic-polyfill provide replacement Atomic* types that can do load/store on thumbv6 if need be.
WG-prioritization assigning priority (Zulip discussion).
@rustbot label -I-prioritize +P-critical +T-compiler
requires users to provide a critical section implementation for their system - so on a multi-core system you need to provide something with the appropriate guarantees, and it won't compile if no implementation is provided
Note that rust-embedded/critical-section@a3cdfd8 has not been released yet. The releases available on crates.io do not work this way.
And even if it were released, atomic-polyfill is still unsound on multi-core systems because it mixes native atomic load/store and critical-section.
FWIW, my portable-atomic crate implements the equivalent of the atomic types that was be removed in #99595 (by using inline assembly). It also provides equivalents on other platforms where atomic load/store is not provided (riscv without A-extension, msp430, avr).
Remove support for atomic load/store on targets that don't support full atomics. Users are expected to switch to using volatile load/store instead.
That seems in conflict with us telling people for years now that volatile operations are not atomic. And they aren't, so this strategy risks subtle miscompilations.
Reacted by Taiki Endo and Jubilee38 remaining items
T-compiler reviewed this as part of 2022 Q3 P-high review
What is current situation here? It seems like there are still unresolved questions about what we want to tell people to do;
but also, I see that @nikic added target changes on the LLVM side for ARM and RISC-V: https://reviews.llvm.org/D130480 and https://reviews.llvm.org/D130621
do we need to be leveraging the +atomic-32 and +forced-atomics features in some fashion within the rustc interface to LLVM in order to make progress here?We already use
+atomic-32for relevant ARM/Thumb targets.Once we update to LLVM 16, we can consider using
+forced-atomicson RISC-V (it would allow landing the change from #98333).I think at this point in time, the only thing that can be done here is adding upstream LLVM support for
+forced-atomicsto more targets, like MSP430 and AVR mentioned above.Reacted by Demo for summer'23@nikic I have no bandwidth at this time to add LLVM support for
+forced-atomicsor lowering atomics for MSP430, but I'm not against the idea and neither is the maintainer last I checked.If this is only simple thing that just drops some bits and lowers atomic stuff to normal loads / stores, then probably it could be fine
Right now, MSP430 doesn't lower atomic operations correctly in LLVM (long story)
This is still true.
powerpc (32-bit) is also affected.
Discussed at 2023 Q1 P-high review
My current understanding is that there is no one objecting to adding support to +forced-atomics for the targets that need it, but no one is taking up the task of actually doing so for all such targets.
I'm not clear on who would be best to take ownership of that, but maybe that doesn't matter yet; the first order of business is to collectively agree that is our strategy going forward.
@Amanieu @nikic am I right in inferring that you're both aligned on the plan of leveraging
+forced-atomicswhere we need it, and the main work items here are adding support for it upstream with LLVM? (And I guess maybe there are some efficiency issues with the current support, as flagged e.g. in #101300 ?)We have
+forced-atomicssupport for RISCV in LLVM 16, so we could give adding that to our target specs (and enabling atomic load/store) a try.I believe the only real concern is that it makes Rust's atomics ABI-incompatible with C's atomics (unless the C code also uses
+forced-atomics), but I think people value having atomics in Rust at all much more than being able to work with atomics across an ABI boundary.Reacted by Dario Nieuwenhuis, Scott Mabin, Matt Johnston and Demo for summer'23I believe the only real concern is that it makes Rust's atomics ABI-incompatible with C's atomics (unless the C code also uses +forced-atomics)
I've been thinking about this; does anyone have a concrete example of where calling C code using libatomic from Rust code using Rust atomics is unsound/memory-unsafe (single core and multi core case )? Or is it more a "precaution that interop will subtly break"?
I can't visualize an example after thinking about it, because I already can't think of a way to share Rust atomics and use as if they were C
_Atomic_s; Rust usesrepr(C)on a newtype, and C_Atomics are not guaranteed to be the same size as the underlying type (dogccandclangaccommodate this?).Reacted by JubileeWe have
+forced-atomicssupport for RISCV in LLVM 16, so we could give adding that to our target specs (and enabling atomic load/store) a try.FWIW, I'm planning to find some spare time to address some atomic weirdness on MSP430 in LLVM.
Reacted by JubileeVisited during the compiler team's P-high review. Based on the most recent discussion points, we believe this can relabeled P-medium as
+forced-atomicsupport has landed in LLVM for all relevant Tier 1 or Tier 2 targets and interoperability of passing pointers to atomic values between C and Rust in this context does not seem to be commonly used.Reacted by Demo for summer'23
Some embedded targets (
thumbv6m,riscv32i) don't support general atomic operations (compare_exchange,fetch_add, etc) but do support atomicloadandstoreoperations. We previously supported this onthumbv6m(but notriscv32i, see #98333) bycfging out most of the methods onAtomic*except forloadandstore. However recent changes to LLVM (#99595) cause it to emit calls to libatomic forloadandstore(it already does this for the other atomic operations).It seems that LLVM's support for lowering atomic loads and stores on ARM was an accident that is being reverted. However the reason for this revert is that the directly lowered load/store cannot interoperate correctly with the other atomic operations which are lowered to libcalls. This concern doesn't apply to Rust since we don't expose emulated (read: not lock free) atomics, so there is no interoperability concern (except maybe for FFI with C that uses atomics?).
I see 2 ways we can move forward: